[PATCH] scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn()

[PATCH] scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn()

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

This's issue as follows:
 connection50052:0: detected conn error (1020)
BUG: kernel NULL pointer dereference, address: 0000000000000018
PGD 0 P4D 0
Oops: 0000 [#1] SMP NOPTI
CPU: 2 PID: 2696024 Comm: kworker/u8:0 Kdump: loaded Tainted: G        W  OE K   5.10.0-136.12.0.86.x86_64 #1
Workqueue: iscsi_conn_cleanup iscsi_cleanup_conn_work_fn [scsi_transport_iscsi]
RIP: 0010:iscsi_sw_tcp_release_conn+0x73/0x1d0
RAX: 0000000000000000 RBX: ffff9ae5ede7a4a0 RCX: 0000000000000002
RDX: 000000000027acac RSI: 0000000000000002 RDI: 0001ce6ceb593ebc
RBP: ffff9ae5c16c7400 R08: 0000000000000002 R09: 000000000027ac54
R10: ffffb8e4408ffc38 R11: ffffffffbb93b5c0 R12: ffff9ae5ede7a7d8
R13: ffff9ae5ede7a7d8 R14: ffff9ae5c3376c00 R15: ffff9ae5c3376c05
FS:  0000000000000000(0000) GS:ffff9ae5fad00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000018 CR3: 0000000131872003 CR4: 00000000003706e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 iscsi_sw_tcp_release_conn+0x73/0x1d0
 iscsi_sw_tcp_conn_stop+0x5e/0x78 [iscsi_tcp]
 iscsi_stop_conn+0x5f/0xb0 [scsi_transport_iscsi]
 iscsi_cleanup_conn_work_fn+0x87/0x100
 process_one_work+0x1b2/0x350
 worker_thread+0x49/0x310
 kthread+0xfb/0x140
 ret_from_fork+0x1f/0x30

Above issue may happen as follows:
            user                            kernel
iscsi_if_recv_msg
  iscsi_if_transport_conn(transport, nlh, rlen);
   iscsi_if_stop_conn(conn, ev->u.stop_conn.flag);
    if (flag == STOP_CONN_TERM)
     cancel_work_sync(&conn->cleanup_work); // work not queue yet.

                     iscsi_conn_error_event
                      switch (state)
                       case ISCSI_CONN_UP:
                         if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP,
                                               &conn->flags);
                           //queue work
                           queue_work(iscsi_conn_cleanup_workq,
                                    &conn->cleanup_work);
                    ...
                     // run work
                     iscsi_cleanup_conn_work_fn
                       iscsi_stop_conn(conn, STOP_CONN_RECOVER);
                        conn->transport->stop_conn(conn, flag);
                         iscsi_sw_tcp_conn_stop(conn, flag);
                           struct socket *sock = tcp_sw_conn->sock;
                           if (!sock)  // pass
                             return;

    iscsi_stop_conn(conn, flag);
      conn->transport->stop_conn(conn, flag);
        iscsi_sw_tcp_conn_stop(conn, flag);
          struct socket *sock = tcp_sw_conn->sock;
          mutex_lock(&tcp_sw_conn->sock_lock);
          tcp_sw_conn->sock = NULL; // clear sock
          mutex_unlock(&tcp_sw_conn->sock_lock);

                          iscsi_sw_tcp_conn_restore_callbacks(conn);
                            struct sock *sk = tcp_sw_conn->sock->sk;
                            *** trigger null ptr dereference ***

To solve above issue, when the user mode delivers the STOP_CONN_TERM,
the ISCSI_CLS_CONN_BIT_CLEANUP status needs to be set to prevent
concurrent invoking of iscsi_stop_conn().

Fixes: 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 43 +++++++++++++++--------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 8aa76f813bcd..aea319c1e72f 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2266,6 +2266,8 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
 
 static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
 {
+	bool cleanup;
+
 	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop.\n");
 	/*
 	 * For offload, iscsid may not know about the ep like when iscsid is
@@ -2278,35 +2280,36 @@ static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
 	mutex_unlock(&conn->ep_mutex);
 
 	/*
-	 * If this is a termination we have to call stop_conn with that flag
-	 * so the correct states get set. If we haven't run the work yet try to
-	 * avoid the extra run.
+	 * Figure out if it was the kernel or userspace initiating this.
 	 */
-	if (flag == STOP_CONN_TERM) {
-		cancel_work_sync(&conn->cleanup_work);
-		iscsi_stop_conn(conn, flag);
-	} else {
+	spin_lock_irq(&conn->lock);
+	cleanup = test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
+	spin_unlock_irq(&conn->lock);
+
+	if (cleanup) {
 		/*
-		 * Figure out if it was the kernel or userspace initiating this.
+		 * If this is a termination we have to call stop_conn with
+		 * that flag so the correct states get set. If we haven't
+		 * run the work yet try to avoid the extra run.
 		 */
-		spin_lock_irq(&conn->lock);
-		if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
-			spin_unlock_irq(&conn->lock);
+		if (flag == STOP_CONN_TERM) {
+			ISCSI_DBG_TRANS_CONN(conn,
+					"cancel kernel conn cleanup.\n");
+			cancel_work_sync(&conn->cleanup_work);
 			iscsi_stop_conn(conn, flag);
 		} else {
-			spin_unlock_irq(&conn->lock);
 			ISCSI_DBG_TRANS_CONN(conn,
-					     "flush kernel conn cleanup.\n");
+					"flush kernel conn cleanup.\n");
 			flush_work(&conn->cleanup_work);
 		}
-		/*
-		 * Only clear for recovery to avoid extra cleanup runs during
-		 * termination.
-		 */
-		spin_lock_irq(&conn->lock);
-		clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
-		spin_unlock_irq(&conn->lock);
+	} else {
+		iscsi_stop_conn(conn, flag);
 	}
+
+	spin_lock_irq(&conn->lock);
+	clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
+	spin_unlock_irq(&conn->lock);
+
 	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop done.\n");
 	return 0;
 }
-- 
2.34.1

Re: [PATCH] scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn()

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-after-free of `iscsi_cls_conn` due to missing cancellation of `cleanup_work` on release.
--

--- Patch [1]: [PATCH] scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn() ---
commit bdfb05c9a6c008e559f81d0c3e5ffb712699e0cc
Author: Ye Bin <yebin10@huawei.com>

scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn()

This commit fixes a NULL pointer dereference by setting the
ISCSI_CLS_CONN_BIT_CLEANUP bit when userspace delivers STOP_CONN_TERM.
This prevents concurrent execution of iscsi_stop_conn() and cancels or
flushes the kernel connection cleanup work as needed.

Fixes: 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")

> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2266,6 +2266,8 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
>  
>  static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
>  {
> +	bool cleanup;
> +
>  	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop.\n");
>  	/*
>  	 * For offload, iscsid may not know about the ep like when iscsid is
[ ... ]
> -	if (flag == STOP_CONN_TERM) {
> -		cancel_work_sync(&conn->cleanup_work);
> -		iscsi_stop_conn(conn, flag);
> -	} else {
> +	spin_lock_irq(&conn->lock);
> +	cleanup = test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> +	spin_unlock_irq(&conn->lock);
> +
> +	if (cleanup) {
>  		/*
> -		 * Figure out if it was the kernel or userspace initiating this.
> +		 * If this is a termination we have to call stop_conn with
> +		 * that flag so the correct states get set. If we haven't
> +		 * run the work yet try to avoid the extra run.
>  		 */
> -		spin_lock_irq(&conn->lock);
> -		if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> -			spin_unlock_irq(&conn->lock);
> +		if (flag == STOP_CONN_TERM) {
> +			ISCSI_DBG_TRANS_CONN(conn,
> +					"cancel kernel conn cleanup.\n");
> +			cancel_work_sync(&conn->cleanup_work);

[Severity: High]
This is a pre-existing issue, but since we are modifying cleanup_work
handling here, is there a risk of a use-after-free if the connection is
destroyed before the work executes?

Looking at iscsi_conn_error_event(), it queues conn->cleanup_work without
taking a reference to the connection. If a session is torn down before the
work executes (e.g. via device removal calling iscsi_remove_session()),
iscsi_iter_destroy_conn_fn() drops the references:

drivers/scsi/scsi_transport_iscsi.c:iscsi_iter_destroy_conn_fn()
    ...
    iscsi_remove_conn(iscsi_dev_to_conn(dev));
    iscsi_put_conn(iscsi_dev_to_conn(dev));

This final reference drop triggers iscsi_conn_release(), which frees the
connection memory:

drivers/scsi/scsi_transport_iscsi.c:iscsi_conn_release()
    ...
    kfree(conn);

Since iscsi_conn_release() does not call cancel_work_sync(&conn->cleanup_work),
could iscsi_cleanup_conn_work_fn() execute and dereference the already freed
conn struct? Should we be cancelling or flushing the work during
iscsi_conn_release() to prevent this?

>  			iscsi_stop_conn(conn, flag);
>  		} else {
> -			spin_unlock_irq(&conn->lock);
>  			ISCSI_DBG_TRANS_CONN(conn,
> -					     "flush kernel conn cleanup.\n");
> +					"flush kernel conn cleanup.\n");
>  			flush_work(&conn->cleanup_work);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820020321.1537641-1-yebin@huaweicloud.com?part=1