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

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

[Jiayuan Liang]

A null-pointer dereference can occur in
iscsi_sw_tcp_conn_restore_callbacks() due to a race condition
leading to concurrent/re-entrant invocations of
iscsi_sw_tcp_release_conn().

Specifically, the re-entrancy can be triggered under the
following scenario:

1. The iSCSI client initiates a logout, actively stopping the
   connection via:
   iscsi_if_stop_conn()
     -> iscsi_stop_conn(..., STOP_CONN_TERM)
          -> cancel_work_sync(&conn->cleanup_work)
          -> iscsi_sw_tcp_release_conn()

2. Simultaneously, a server disconnect triggers a heartbeat
   timeout on the client side, executing the timeout path:
   iscsi_check_transport_timeouts()
     -> iscsi_conn_failure()
          -> iscsi_conn_error_event()
               -> queue_work(..., &conn->cleanup_work)

   This schedules iscsi_cleanup_conn_work_fn(), which calls:
   iscsi_cleanup_conn_work_fn()
     -> iscsi_stop_conn(..., STOP_CONN_RECOVER)
          -> iscsi_sw_tcp_release_conn()

If these two paths execute concurrently, iscsi_sw_tcp_release_conn()
is re-entered. Since the first invocation releases the socket and
sets tcp_sw_conn->sock to NULL, the subsequent re-entrant
invocation in iscsi_sw_tcp_conn_restore_callbacks() attempts to
dereference the NULL pointer at `tcp_sw_conn->sock->sk`, resulting
in a kernel panic (Oops):

BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
Oops: 0000 [#1] SMP PTI
Workqueue: iscsi_conn_cleanup iscsi_cleanup_conn_work_fn [scsi_transport_iscsi]
RIP: 0010:iscsi_sw_tcp_release_conn+0x54/0x110 [iscsi_tcp]
Call Trace:
 iscsi_sw_tcp_conn_stop+0x5d/0x80 [iscsi_tcp]
 iscsi_stop_conn+0x66/0xc0 [scsi_transport_iscsi]
 iscsi_cleanup_conn_work_fn+0x6e/0xb0 [scsi_transport_iscsi]
 process_one_work+0x1a7/0x360
 worker_thread+0x30/0x390
 kthread+0x10a/0x120
 ret_from_fork+0x35/0x40

Fix this by adding a NULL check for `tcp_sw_conn->sock` in
iscsi_sw_tcp_conn_restore_callbacks() before attempting to access
the socket's internal fields.

Signed-off-by: Jiayuan Liang <ljykernel@gmail.com>
---
 drivers/scsi/iscsi_tcp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 9260b1c9b0e0..0eabb8b59f46 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;
 	/* restore socket callbacks, see also: iscsi_sw_tcp_conn_set_callbacks() */
 	write_lock_bh(&sk->sk_callback_lock);
 	sk->sk_user_data    = NULL;
@@ -277,6 +281,7 @@ iscsi_sw_tcp_conn_restore_callbacks(struct iscsi_conn *conn)
 	write_unlock_bh(&sk->sk_callback_lock);
 }
 
+
 /**
  * iscsi_sw_tcp_xmit_segment - transmit segment
  * @tcp_conn: the iSCSI TCP connection
-- 
2.43.0

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

[sashiko-bot]

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 <ljykernel@gmail.com>

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/20260805065326.1952-1-ljykernel@gmail.com?part=1

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

[Mike Christie]

On 8/5/26 1:53 AM, Jiayuan Liang wrote:
> A null-pointer dereference can occur in iscsi_sw_tcp_conn_restore_callbacks() due to a race condition leading to concurrent/re-entrant invocations of iscsi_sw_tcp_release_conn(). Specifically, the re-entrancy can be triggered under the following
> 
> 
> A null-pointer dereference can occur in
> iscsi_sw_tcp_conn_restore_callbacks() due to a race condition
> leading to concurrent/re-entrant invocations of
> iscsi_sw_tcp_release_conn().
> 
> Specifically, the re-entrancy can be triggered under the
> following scenario:
> 
> 1. The iSCSI client initiates a logout, actively stopping the
>    connection via:
>    iscsi_if_stop_conn()
>      -> iscsi_stop_conn(..., STOP_CONN_TERM)
>           -> cancel_work_sync(&conn->cleanup_work)
>           -> iscsi_sw_tcp_release_conn()
> 
> 2. Simultaneously, a server disconnect triggers a heartbeat
>    timeout on the client side, executing the timeout path:
>    iscsi_check_transport_timeouts()
>      -> iscsi_conn_failure()
>           -> iscsi_conn_error_event()
>                -> queue_work(..., &conn->cleanup_work)
> 
>    This schedules iscsi_cleanup_conn_work_fn(), which calls:
>    iscsi_cleanup_conn_work_fn()
>      -> iscsi_stop_conn(..., STOP_CONN_RECOVER)
>           -> iscsi_sw_tcp_release_conn()
> 
> If these two paths execute concurrently, iscsi_sw_tcp_release_conn()
> is re-entered. Since the first invocation releases the socket and
> sets tcp_sw_conn->sock to NULL, the subsequent re-entrant
> invocation in iscsi_sw_tcp_conn_restore_callbacks() attempts to
> dereference the NULL pointer at `tcp_sw_conn->sock->sk`, resulting
> in a kernel panic (Oops):
> 
We don't want to allow iscsi_cleanup_conn_work_fn to run after a
termination and we don't want to allow iscsi_cleanup_conn_work_fn
and iscsi_if_stop_conn to run concurrently.

Can we have iscsi_if_stop_conn hold the conn->ep_mutex when calling
iscsi_stop_conn. iscsi_cleanup_conn_work_fn would then have a check
for for if the conn->state was ISCSI_CONN_DOWN and if so not call
iscsi_stop_conn.

I think iscsi_if_stop_conn could also call cancel_work_sync
after calling iscsi_stop_conn for both the STOP_CONN_TERM and
STOP_CONN_RECOVER cases instead of calling it for the
STOP_CONN_TERM before calling iscsi_stop_conn.