Re: [PATCH] scsi: iscsi_tcp: Fix null-pointer dereference in iscsi_sw_tcp_conn_restore_callbacks

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The newly added NULL pointer check introduces a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability that fails to completely prevent the NULL pointer dereference.

Pre-existing issues:
- [Critical] The patch fails to address the root cause of the race condition, leaving a severe double free / file refcount underflow and use-after-free vulnerability.
--

--- Patch [1]: [PATCH] scsi: iscsi_tcp: Fix null-pointer dereference in iscsi_sw_tcp_conn_restore_callbacks ---
commit a6e54310fce25391e4b63d84129f6076f40ca758
Author: Jiayuan Liang <[email protected]>

scsi: iscsi_tcp: Fix null-pointer dereference in iscsi_sw_tcp_conn_restore_callbacks

The commit message states this fixes a null pointer dereference in
iscsi_sw_tcp_conn_restore_callbacks caused by a race condition leading to
concurrent invocations of iscsi_sw_tcp_release_conn. The patch adds a null
check for the socket pointer before accessing its internal fields.

> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 9260b1c9b0e05..0eabb8b59f46d 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -265,8 +265,12 @@ iscsi_sw_tcp_conn_restore_callbacks(struct iscsi_conn *conn)
>  {
>  	struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
>  	struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
> -	struct sock *sk = tcp_sw_conn->sock->sk;
> +	struct sock *sk;
>  
> +	if (!tcp_sw_conn->sock)
> +		return;
> +
> +	sk = tcp_sw_conn->sock->sk;

[Severity: High]
Does this new check fully prevent the null pointer dereference?

Because the check and the subsequent read are lockless and not protected
by READ_ONCE(), the compiler could fetch the tcp_sw_conn->sock pointer
twice. 

If another thread sets tcp_sw_conn->sock to null under sock_lock right after
the check, could the second fetch result in dereferencing null here?

[Severity: Critical]
This is a pre-existing issue, but does this patch fully address the root
cause of the race condition?

If two threads concurrently execute iscsi_sw_tcp_release_conn, they both
read tcp_sw_conn->sock into a local variable:

iscsi_sw_tcp_release_conn() {
	...
	struct socket *sock = tcp_sw_conn->sock;

	if (!sock)
		return;
	...

Since both threads can read the non-null pointer before either thread takes
the sock_lock to set it to null, both will bypass this check.

Later in the same function, both threads will call:

	...
	mutex_lock(&tcp_sw_conn->sock_lock);
	tcp_sw_conn->sock = NULL;
	mutex_unlock(&tcp_sw_conn->sock_lock);
	sockfd_put(sock);
}

Will this cause a double free of the socket file refcount, since both
concurrent threads call sockfd_put on the same locally cached sock pointer?

Similarly, in iscsi_sw_tcp_conn_stop, both threads might use the locally
cached sock pointer after it has already been released by the first thread:

iscsi_sw_tcp_conn_stop() {
	...
	struct socket *sock = tcp_sw_conn->sock;

	if (!sock)
		return;

	sock->sk->sk_err = EIO;
	...

If one thread finishes iscsi_sw_tcp_release_conn first, will the second
thread dereference a freed socket here?

>  	/* restore socket callbacks, see also: iscsi_sw_tcp_conn_set_callbacks() */
>  	write_lock_bh(&sk->sk_callback_lock);
>  	sk->sk_user_data    = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1