[PATCH -next 0/2] scsi: iscsi: Fix races in connection cleanup path

[PATCH -next 0/2] scsi: iscsi: Fix races in connection cleanup path

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

This series fixes two races in the iSCSI connection cleanup path that
were introduced by commit 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel
conn failure handling").

Patch 1 fixes a NULL pointer dereference in iscsi_sw_tcp_release_conn()
that occurs when userspace delivers STOP_CONN_TERM while the kernel has
already queued cleanup_work for the same connection. The two paths race
on tcp_sw_conn->sock: the userspace path clears it under sock_lock while
the workqueue path dereferences it locklessly in
iscsi_sw_tcp_conn_restore_callbacks(), leading to a NULL dereference.
The fix sets ISCSI_CLS_CONN_BIT_CLEANUP when userspace delivers
STOP_CONN_TERM to prevent concurrent invocation of iscsi_stop_conn().

Patch 2 fixes a use-after-free in iscsi_conn_release().
iscsi_conn_error_event() queues conn->cleanup_work without taking a
reference to the connection, so if the connection is freed (e.g. via
iscsi_remove_session() -> iscsi_iter_destroy_conn_fn()) while the work
is still pending or running, iscsi_cleanup_conn_work_fn() dereferences
freed memory. Unlike the iscsi_if_destroy_conn() and iscsi_if_stop_conn()
paths, the session teardown path does not handle the pending
cleanup_work before dropping the connection reference. The fix calls
cancel_work_sync(&conn->cleanup_work) in iscsi_conn_release() before
freeing the connection.

Ye Bin (2):
  scsi: iscsi: fix NULL pointer dereference in
    iscsi_sw_tcp_release_conn()
  scsi: iscsi: Fix use-after-free in iscsi_conn_release() for
    cleanup_work

 drivers/scsi/scsi_transport_iscsi.c | 50 +++++++++++++++++------------
 1 file changed, 30 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH -next 1/2] 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 -next 1/2] 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:
- [High] The patch introduces an endpoint (`struct iscsi_endpoint`) reference leak by unconditionally clearing `ISCSI_CLS_CONN_BIT_CLEANUP` in `iscsi_if_stop_conn()` during a `STOP_CONN_TERM`, causing the subsequent endpoint disconnect to incorrectly skip releasing the workqueue's reference.
--

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

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

This commit addresses a NULL pointer dereference in iscsi_sw_tcp_release_conn()
by ensuring the ISCSI_CLS_CONN_BIT_CLEANUP bit is set during STOP_CONN_TERM.
This prevents concurrent execution of iscsi_stop_conn().

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 8aa76f813bcdb..aea319c1e72fe 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2278,35 +2280,36 @@ static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)

[ ... ]

> -		/*
> -		 * 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);

[Severity: High]
Does this unconditionally clear ISCSI_CLS_CONN_BIT_CLEANUP during
STOP_CONN_TERM?

The original code avoided clearing this bit during termination, as the removed
comment explicitly noted.

If a connection error queues iscsi_cleanup_conn_work_fn(), which unbinds the
endpoint (setting conn->ep = NULL), and then userspace initiates termination
via iscsi_if_stop_conn(conn, STOP_CONN_TERM), iscsi_if_disconnect_bound_ep()
won't be called here because conn->ep is NULL.

When this bit is subsequently cleared unconditionally, and userspace later issues
an event to disconnect the endpoint, will iscsi_if_disconnect_bound_ep() skip
releasing the workqueue's reference?

drivers/scsi/scsi_transport_iscsi.c:iscsi_if_disconnect_bound_ep() {
    ...
	if (!test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
		spin_unlock_irq(&conn->lock);
		iscsi_ep_disconnect(conn, is_active);
	} else {
		...
		iscsi_put_endpoint(ep);
    ...
}

Could this cause a resource leak of struct iscsi_endpoint over time?

> +
>  	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop done.\n");
>  	return 0;
>  }

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

[PATCH -next 2/2] scsi: iscsi: Fix use-after-free in iscsi_conn_release() for cleanup_work

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

iscsi_conn_error_event() queues conn->cleanup_work without taking a
reference to the connection. If the connection's refcount drops to zero
while the work is still pending or running, iscsi_conn_release() frees
the connection memory without canceling the work, leading to a
use-after-free when iscsi_cleanup_conn_work_fn() later dereferences the
freed conn struct.

This can happen when a session is torn down via iscsi_remove_session(),
which calls iscsi_iter_destroy_conn_fn() to remove and drop the final
reference on each remaining connection. Unlike iscsi_if_destroy_conn()
(which flushes the work) and iscsi_if_stop_conn() (which cancels or
flushes the work), the iscsi_iter_destroy_conn_fn() path does not handle
the pending cleanup_work before dropping the connection reference.

Trigger flow:

  CPU 0 (error path)               CPU 1 (session teardown)
  -----------------------          --------------------------
  iscsi_conn_error_event(conn)
    queue_work(cleanup_work)
    [no conn reference taken]      iscsi_remove_session()
                                     iscsi_iter_destroy_conn_fn()
                                       iscsi_remove_conn(conn)
                                       iscsi_put_conn(conn)
                                         refcount == 0
                                         iscsi_conn_release()
                                           kfree(conn)

  [workqueue picks up work]
  iscsi_cleanup_conn_work_fn()
    container_of(work, ...)
    mutex_lock(&conn->ep_mutex)     *** USE-AFTER-FREE ***

Fix this by calling cancel_work_sync(&conn->cleanup_work) in
iscsi_conn_release() before freeing the connection. This is safe
because:
  - iscsi_conn_release() is always called in process context
  - iscsi_cleanup_conn_work_fn() never drops a conn reference, so
    there is no deadlock risk
  - If the work is running, cancel_work_sync() waits for it to
    complete before kfree(conn), keeping conn valid during execution
  - If the work is pending, it is simply canceled, which is harmless
    since the connection is being destroyed

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

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index aea319c1e72f..cdc337b0fc26 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2128,6 +2128,13 @@ static void iscsi_conn_release(struct device *dev)
 	struct device *parent = conn->dev.parent;
 
 	ISCSI_DBG_TRANS_CONN(conn, "Releasing conn\n");
+	/*
+	 * iscsi_conn_error_event() may have queued conn->cleanup_work without
+	 * holding a reference to the connection. Cancel any pending or
+	 * running work before freeing the connection to prevent a
+	 * use-after-free.
+	 */
+	cancel_work_sync(&conn->cleanup_work);
 	kfree(conn);
 	put_device(parent);
 }
-- 
2.34.1

Re: [PATCH -next 2/2] scsi: iscsi: Fix use-after-free in iscsi_conn_release() for cleanup_work

[sashiko-bot]

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

New issues:
- [High] Adding `cancel_work_sync(&conn->cleanup_work)` inside `iscsi_conn_release()` introduces a guaranteed AB-BA deadlock with `session->eh_mutex` when the final connection reference is dropped by the SCSI error handler.

Pre-existing issues:
- [Critical] ep->conn is never cleared when a connection is destroyed, leading to a Use-After-Free in iscsi_if_ep_disconnect().
- [High] LLD-specific background tasks (e.g., `transport_timer`, `xmitwork`) are not canceled before `iscsi_conn_release()` frees the connection memory, leading to a Use-After-Free.
--

--- Patch [2]: [PATCH -next 2/2] scsi: iscsi: Fix use-after-free in iscsi_conn_release() for cleanup_work ---
commit 16664476ef28f32b2b58204098a289184054f45c
Author: Ye Bin <yebin10@huawei.com>

scsi: iscsi: Fix use-after-free in iscsi_conn_release() for cleanup_work

This patch attempts to resolve a use-after-free by canceling the pending or
running cleanup_work task before freeing the connection memory in
iscsi_conn_release().

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index aea319c1e72fe..cdc337b0fc26a 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2128,6 +2128,13 @@ static void iscsi_conn_release(struct device *dev)
>  	struct device *parent = conn->dev.parent;
>  
>  	ISCSI_DBG_TRANS_CONN(conn, "Releasing conn\n");
> +	/*
> +	 * iscsi_conn_error_event() may have queued conn->cleanup_work without
> +	 * holding a reference to the connection. Cancel any pending or
> +	 * running work before freeing the connection to prevent a
> +	 * use-after-free.
> +	 */
> +	cancel_work_sync(&conn->cleanup_work);

[Severity: High]
Could this cancel_work_sync() introduce an AB-BA deadlock with
session->eh_mutex?

If a concurrent session teardown drops the main connection references, the
SCSI error handler might drop the final reference while still holding the
eh_mutex:

drivers/scsi/libiscsi.c:iscsi_eh_abort() {
    mutex_lock(&session->eh_mutex);
    ...
    iscsi_put_conn(conn->cls_conn);
    mutex_unlock(&session->eh_mutex);
}

When iscsi_put_conn() triggers iscsi_conn_release(), it will block here
waiting for the workqueue to finish. However, if the worker thread is
already running, it attempts to acquire the exact same mutex:

drivers/scsi/libiscsi.c:iscsi_conn_stop() {
    mutex_lock(&session->eh_mutex);
    ...
}

Would this cause the error handler and the workqueue to wait on each other
indefinitely?

> +	kfree(conn);

[Severity: High]
This is a pre-existing issue, but does freeing the connection here without
canceling LLD-specific background tasks leave a use-after-free risk?

When a session is abruptly torn down, the teardown iterates over active
connections and drops the final reference:

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));
}

Because this bypasses the transport's destroy_conn callback (such as
iscsi_conn_teardown), asynchronous tasks like conn->transport_timer and
conn->xmitwork are never canceled.

Could this regression allow these timers or work tasks to execute later and
dereference the freed connection memory?

[Severity: Critical]
This is a pre-existing issue, but is there another use-after-free possible
when an endpoint disconnects after a connection is destroyed?

If a session is forcefully destroyed via iscsi_remove_session(), the
connection is freed in iscsi_conn_release() as seen above. However,
ep->conn does not appear to be cleared when this happens.

If userspace then issues an ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT command,
the kernel reads the dangling ep->conn pointer:

drivers/scsi/scsi_transport_iscsi.c:iscsi_if_ep_disconnect() {
    conn = ep->conn;
    if (!conn) {
        ...
    }
    
    mutex_lock(&conn->ep_mutex);
}

Could this regression allow a userspace netlink command to trigger a
use-after-free by acquiring the mutex on the freed connection memory?

>  	put_device(parent);
>  }

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