[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

[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.