[PATCH v2 0/3] scsi: target: iscsi: fix session use-after-free on non-leading login

[PATCH v2 0/3] scsi: target: iscsi: fix session use-after-free on non-leading login

[Daniel Wade]

A non-leading login can attach a connection to a session that
iscsit_close_connection() has already committed to freeing.  The
connection ends up in full feature phase with conn->sess dangling and
its NOP-In timer walking freed memory.  The same window leaves an armed
Time2Retain timer on a freed session when the login fails instead.

The bug dates to the driver's original merge in v3.1, but the fix keys
off ->session_close, which arrived in v5.7 with commit 57c46e9f33da
("scsi: target: fix hang when multiple threads try to destroy the same
iscsi session"), so a backport further back needs adjusting.

Tested with KASAN and lockdep on mainline against a target with
authentication=0 and one explicit ACL.  Compile tested on x86_64.

A reproducer is available on request.

v2:
- 2/3: re-test every condition iscsi_login_non_zero_tsih_s2() tests, not
  just ->session_close.  The Time2Retain timeout and the multi connection
  ERL=0 path both reach iscsit_close_session() without setting it.
- 2/3: publish conn_state inside the attach critical sections.  Checking
  after it was already LOGGED_IN let iscsi_target_tx_thread() enter
  iscsit_close_connection() and wait on a conn_usage_count that the
  refusal path then deadlocked against with kthread_stop().
- Both were reported by Sashiko AI review on v1.

v1: https://lore.kernel.org/linux-scsi/cover.1794732723.git.danjwade95@gmail.com/

Daniel Wade (3):
  scsi: target: iscsi: factor out iscsit_stop_kthreads()
  scsi: target: iscsi: don't attach a connection to a closing session
  scsi: target: iscsi: don't re-arm Time2Retain on a closing session

 drivers/target/iscsi/iscsi_target.c       |  6 ++
 drivers/target/iscsi/iscsi_target_login.c | 81 +++++++++++++++++++++--
 drivers/target/iscsi/iscsi_target_login.h |  1 +
 drivers/target/iscsi/iscsi_target_nego.c  | 17 +----
 4 files changed, 86 insertions(+), 19 deletions(-)

-- 
2.39.0.windows.2

[PATCH v2 1/3] scsi: target: iscsi: factor out iscsit_stop_kthreads()

[Daniel Wade]

iscsi_target_do_tx_login_io() stops a connection's RX and TX kthreads
and releases its thread-set bitmap slot inline on its error path. The
next patch needs that same teardown from iscsi_post_login_handler(), so
move it to a helper beside iscsit_start_kthreads().

No functional change.

Cc: stable@vger.kernel.org
Signed-off-by: Daniel Wade <danjwade95@gmail.com>
---
 drivers/target/iscsi/iscsi_target_login.c | 17 +++++++++++++++++
 drivers/target/iscsi/iscsi_target_login.h |  1 +
 drivers/target/iscsi/iscsi_target_nego.c  | 17 ++---------------
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index c282b6a7029..f4df5518171 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -643,6 +643,23 @@ int iscsit_start_kthreads(struct iscsit_conn *conn)
 	return ret;
 }
 
+void iscsit_stop_kthreads(struct iscsit_conn *conn)
+{
+	if (conn->rx_thread && conn->rx_thread_active) {
+		send_sig(SIGINT, conn->rx_thread, 1);
+		complete(&conn->rx_login_comp);
+		kthread_stop(conn->rx_thread);
+	}
+	if (conn->tx_thread && conn->tx_thread_active) {
+		send_sig(SIGINT, conn->tx_thread, 1);
+		kthread_stop(conn->tx_thread);
+	}
+	spin_lock(&iscsit_global->ts_bitmap_lock);
+	bitmap_release_region(iscsit_global->ts_bitmap, conn->bitmap_id,
+			      get_order(1));
+	spin_unlock(&iscsit_global->ts_bitmap_lock);
+}
+
 void iscsi_post_login_handler(
 	struct iscsi_np *np,
 	struct iscsit_conn *conn,
diff --git a/drivers/target/iscsi/iscsi_target_login.h b/drivers/target/iscsi/iscsi_target_login.h
index 03c7d695d58..02c5f6ecbec 100644
--- a/drivers/target/iscsi/iscsi_target_login.h
+++ b/drivers/target/iscsi/iscsi_target_login.h
@@ -20,6 +20,7 @@ extern int iscsit_get_login_rx(struct iscsit_conn *, struct iscsi_login *);
 extern int iscsit_put_login_tx(struct iscsit_conn *, struct iscsi_login *, u32);
 extern void iscsit_free_conn(struct iscsit_conn *);
 extern int iscsit_start_kthreads(struct iscsit_conn *);
+extern void iscsit_stop_kthreads(struct iscsit_conn *);
 extern void iscsi_post_login_handler(struct iscsi_np *, struct iscsit_conn *, u8);
 extern void iscsi_target_login_sess_out(struct iscsit_conn *, bool, bool);
 extern int iscsi_target_login_thread(void *);
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index b03ed154ca3..d56b28446d3 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -362,21 +362,8 @@ static int iscsi_target_do_tx_login_io(struct iscsit_conn *conn, struct iscsi_lo
 	return 0;
 
 err:
-	if (login->login_complete) {
-		if (conn->rx_thread && conn->rx_thread_active) {
-			send_sig(SIGINT, conn->rx_thread, 1);
-			complete(&conn->rx_login_comp);
-			kthread_stop(conn->rx_thread);
-		}
-		if (conn->tx_thread && conn->tx_thread_active) {
-			send_sig(SIGINT, conn->tx_thread, 1);
-			kthread_stop(conn->tx_thread);
-		}
-		spin_lock(&iscsit_global->ts_bitmap_lock);
-		bitmap_release_region(iscsit_global->ts_bitmap, conn->bitmap_id,
-				      get_order(1));
-		spin_unlock(&iscsit_global->ts_bitmap_lock);
-	}
+	if (login->login_complete)
+		iscsit_stop_kthreads(conn);
 	return -1;
 }
 
-- 
2.39.0.windows.2

[PATCH v2 2/3] scsi: target: iscsi: don't attach a connection to a closing session

[Daniel Wade]

iscsi_login_non_zero_tsih_s2() finds a session on tpg_sess_list, skips it
if it is already marked for teardown, and takes a usage count it holds
across the whole login negotiation.  That skip is sampled once, before
the negotiation, and never re-evaluated.

If the session's last connection goes away while the login is still
negotiating, iscsit_close_connection() sees ->nconn reach zero, commits
to the free under ->conn_lock and calls iscsit_close_session(), which
sleeps in iscsit_check_session_usage_count() on the reference the login
holds.  iscsi_post_login_handler() then attaches the new connection,
arms its NOP-In timer and drops that reference, which is what releases the
free.  conn->sess is left dangling and the timer walks the freed session:

  BUG: KASAN: slab-use-after-free in iscsit_allocate_cmd+0x6de/0x780
   iscsit_handle_nopin_timeout <- call_timer_fn <- run_timer_softirq
  Freed by task ...: kfree <- iscsit_close_session
                     <- iscsit_close_connection
                     <- iscsit_take_action_for_connection_exit

MaxConnections does not prevent this.
iscsi_login_post_auth_non_zero_tsih() tests
(atomic_read(&sess->nconn) + 1) > MaxConnections after the reference is
taken, and ->nconn is 0 here, so (0 + 1) > 1 is false even at the
default of 1.

iscsit_close_connection() tests ->nconn under sess->conn_lock and
iscsi_post_login_handler() attaches under the same lock, so publishing the
close decision in that critical section and re-testing it in the one that
attaches makes the two mutually exclusive.  ->session_close is already
that flag: only ever set, never cleared, and the logout and explicit close
branches already require it.  Set it in the ERL=0 fall-back branch too.

Re-test there everything iscsi_login_non_zero_tsih_s2() tested, not just
->session_close: iscsit_handle_time2retain_timeout() reaches
iscsit_close_session() with only ISCSI_TF_EXPIRED set, and the multi
connection ERL=0 path with only ->session_fall_back_to_erl0.  The timer
flag needs se_tpg->session_lock, which the zero_tsih path in this function
already holds across its own attach.

conn_state is published inside both attach critical sections instead of
up front.  At the gate the connection is therefore still
TARG_CONN_STATE_IN_LOGIN, so iscsi_target_tx_thread() leaves the cleanup
to this context rather than entering iscsit_close_connection() to wait on
a conn_usage_count the refusal is about to drop.  Every reader of
conn_state that walks sess_conn_list holds sess->conn_lock, so none
observes an attached connection that is not yet LOGGED_IN.

The refused connection is torn down here because its kthreads were started
before the final login response went out.

Tested on mainline with KASAN and lockdep: unpatched, the reproducer
produces the report above and then a general protection fault; with the
series applied the login is refused, it no longer triggers, lockdep is
silent and the target stays usable.

Fixes: e48354ce078c ("iscsi-target: Add iSCSI fabric support for target v4.1")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Wade <danjwade95@gmail.com>
---
 drivers/target/iscsi/iscsi_target.c       |  6 +++
 drivers/target/iscsi/iscsi_target_login.c | 56 +++++++++++++++++++++--
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 124ff269b8e..ee7df7f2723 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4431,6 +4431,12 @@ int iscsit_close_connection(
 	 */
 	if (!atomic_read(&sess->session_reinstatement) &&
 	     atomic_read(&sess->session_fall_back_to_erl0)) {
+		/*
+		 * Publish the close decision under ->conn_lock so a non-leading
+		 * login attaching in iscsi_post_login_handler() sees it.  The
+		 * branches below already require ->session_close.
+		 */
+		atomic_set(&sess->session_close, 1);
 		spin_unlock_bh(&sess->conn_lock);
 		complete_all(&sess->session_wait_comp);
 		iscsit_close_session(sess, true);
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index f4df5518171..556ae5d2d50 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -660,6 +660,33 @@ void iscsit_stop_kthreads(struct iscsit_conn *conn)
 	spin_unlock(&iscsit_global->ts_bitmap_lock);
 }
 
+/*
+ * Drop a connection whose kthreads are already running but that must not enter
+ * full feature phase.  conn->sess must not be touched once this returns.
+ */
+static void iscsi_post_login_fail_conn(struct iscsit_conn *conn)
+{
+	/*
+	 * The login is refused before conn_state leaves
+	 * TARG_CONN_STATE_IN_LOGIN, so iscsi_target_tx_thread() still leaves
+	 * the cleanup to this context and cannot be inside
+	 * iscsit_close_connection() waiting on conn_usage_count.
+	 */
+	iscsit_stop_kthreads(conn);
+	iscsit_dec_conn_usage_count(conn);
+
+	if (conn->sock) {
+		sock_release(conn->sock);
+		conn->sock = NULL;
+	}
+	if (conn->conn_transport->iscsit_wait_conn)
+		conn->conn_transport->iscsit_wait_conn(conn);
+	if (conn->conn_transport->iscsit_free_conn)
+		conn->conn_transport->iscsit_free_conn(conn);
+
+	iscsit_free_conn(conn);
+}
+
 void iscsi_post_login_handler(
 	struct iscsi_np *np,
 	struct iscsit_conn *conn,
@@ -676,9 +703,6 @@ void iscsi_post_login_handler(
 	iscsit_collect_login_stats(conn, ISCSI_STATUS_CLS_SUCCESS,
 			ISCSI_LOGIN_STATUS_ACCEPT);
 
-	pr_debug("Moving to TARG_CONN_STATE_LOGGED_IN.\n");
-	conn->conn_state = TARG_CONN_STATE_LOGGED_IN;
-
 	iscsi_set_connection_parameters(conn->conn_ops, conn->param_list);
 	/*
 	 * SCSI Initiator -> SCSI Target Port Mapping
@@ -689,7 +713,28 @@ void iscsi_post_login_handler(
 		iscsi_release_param_list(conn->param_list);
 		conn->param_list = NULL;
 
+		spin_lock_bh(&se_tpg->session_lock);
 		spin_lock_bh(&sess->conn_lock);
+		/*
+		 * iscsi_login_non_zero_tsih_s2() sampled these before the
+		 * negotiation and never looked again.  Re-test all of them under
+		 * the lock that serialises against the ->nconn test in
+		 * iscsit_close_connection(), and before this connection reaches
+		 * sess_conn_list or TARG_CONN_STATE_LOGGED_IN.
+		 */
+		if (atomic_read(&sess->session_close) ||
+		    atomic_read(&sess->session_fall_back_to_erl0) ||
+		    atomic_read(&sess->session_logout) ||
+		    (sess->time2retain_timer_flags & ISCSI_TF_EXPIRED)) {
+			spin_unlock_bh(&sess->conn_lock);
+			spin_unlock_bh(&se_tpg->session_lock);
+			pr_err("iSCSI session is closing, failing login on CID: %u\n",
+			       conn->cid);
+			iscsi_post_login_fail_conn(conn);
+			/* Last access to @sess: this lets close_session() free it. */
+			iscsit_dec_session_usage_count(sess);
+			return;
+		}
 		atomic_set(&sess->session_continuation, 0);
 		if (sess->session_state == TARG_SESS_STATE_FAILED) {
 			pr_debug("Moving to"
@@ -707,7 +752,10 @@ void iscsi_post_login_handler(
 		pr_debug("Incremented iSCSI Connection count to %d"
 			" from node: %s\n", atomic_read(&sess->nconn),
 			sess->sess_ops->InitiatorName);
+		pr_debug("Moving to TARG_CONN_STATE_LOGGED_IN.\n");
+		conn->conn_state = TARG_CONN_STATE_LOGGED_IN;
 		spin_unlock_bh(&sess->conn_lock);
+		spin_unlock_bh(&se_tpg->session_lock);
 
 		iscsi_post_login_start_timers(conn);
 		/*
@@ -755,6 +803,8 @@ void iscsi_post_login_handler(
 	pr_debug("Incremented iSCSI Connection count to %d from node:"
 		" %s\n", atomic_read(&sess->nconn),
 		sess->sess_ops->InitiatorName);
+	pr_debug("Moving to TARG_CONN_STATE_LOGGED_IN.\n");
+	conn->conn_state = TARG_CONN_STATE_LOGGED_IN;
 	spin_unlock_bh(&sess->conn_lock);
 
 	sess->sid = tpg->sid++;
-- 
2.39.0.windows.2

[PATCH v2 3/3] scsi: target: iscsi: don't re-arm Time2Retain on a closing session

[Daniel Wade]

iscsi_target_login_sess_out() re-arms the Time2Retain timer of the
session whose login just failed.  In the window closed by the previous
patch that session may already be on its way out: iscsit_close_session()
stops that timer, then sleeps in iscsit_check_session_usage_count() on
the reference this path is about to drop, and frees the session as soon
as it is dropped.  A re-arm therefore lands between the stop and the
free, and leaves an armed timer on freed memory.

->session_close is set before the free is committed to, so test it here
too and skip the re-arm.

Unlike the previous patch this does not need the other teardown flags.
Every iscsit_close_session() that reaches the free is the can_sleep=true
call from iscsit_close_connection() and has ->session_close set, while
iscsit_handle_time2retain_timeout() passes false and re-arms the timer
instead of freeing when a usage count is held.

Fixes: e48354ce078c ("iscsi-target: Add iSCSI fabric support for target v4.1")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Wade <danjwade95@gmail.com>
---
 drivers/target/iscsi/iscsi_target_login.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 556ae5d2d50..124c9903fac 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1177,7 +1177,13 @@ void iscsi_target_login_sess_out(struct iscsit_conn *conn,
 	 */
 	if (!zero_tsih && conn->sess) {
 		spin_lock_bh(&conn->sess->conn_lock);
-		if (conn->sess->session_state == TARG_SESS_STATE_FAILED) {
+		/*
+		 * iscsit_close_session() stopped this timer before sleeping on
+		 * the usage count this path drops, so re-arming it would leave
+		 * the timer armed on freed memory.
+		 */
+		if (conn->sess->session_state == TARG_SESS_STATE_FAILED &&
+		    !atomic_read(&conn->sess->session_close)) {
 			struct se_portal_group *se_tpg =
 					&conn->tpg->tpg_se_tpg;
 
-- 
2.39.0.windows.2

Re: [PATCH v2 2/3] scsi: target: iscsi: don't attach a connection to a closing session

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] scsi: target: iscsi: session leak when login fails on
  ISCSI_TF_EXPIRED
--

commit 2ec69a848adc6f1dace5f53e352fe2730d42d6f2
Author: Daniel Wade <danjwade95@gmail.com>

scsi: target: iscsi: don't attach a connection to a closing session

This commit prevents a non-leading iSCSI login from attaching a connection
to a session that began closing while the login was negotiating.

> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index f4df551817116..556ae5d2d501b 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c

[ ... ]

> @@ -689,7 +713,28 @@ void iscsi_post_login_handler(
>  		iscsi_release_param_list(conn->param_list);
>  		conn->param_list = NULL;
>  
> +		spin_lock_bh(&se_tpg->session_lock);
>  		spin_lock_bh(&sess->conn_lock);
> +		/*
> +		 * iscsi_login_non_zero_tsih_s2() sampled these before the
> +		 * negotiation and never looked again.  Re-test all of them under
> +		 * the lock that serialises against the ->nconn test in
> +		 * iscsit_close_connection(), and before this connection reaches
> +		 * sess_conn_list or TARG_CONN_STATE_LOGGED_IN.
> +		 */
> +		if (atomic_read(&sess->session_close) ||
> +		    atomic_read(&sess->session_fall_back_to_erl0) ||
> +		    atomic_read(&sess->session_logout) ||
> +		    (sess->time2retain_timer_flags & ISCSI_TF_EXPIRED)) {
> +			spin_unlock_bh(&sess->conn_lock);
> +			spin_unlock_bh(&se_tpg->session_lock);
> +			pr_err("iSCSI session is closing, failing login on CID: %u\n",
> +			       conn->cid);
> +			iscsi_post_login_fail_conn(conn);
> +			/* Last access to @sess: this lets close_session() free it. */
> +			iscsit_dec_session_usage_count(sess);
> +			return;
> +		}

[Severity: Medium]
Does this leak sess when the login is refused because ISCSI_TF_EXPIRED is
set?

When the Time2Retain timer expires while the negotiating login holds
sess->session_usage_count, iscsit_handle_time2retain_timeout() sets
ISCSI_TF_EXPIRED without clearing ISCSI_TF_RUNNING and calls
iscsit_close_session(sess, false) in timer context.  Because can_sleep is
false, iscsit_check_session_usage_count() does not wait for the login to
drop its reference:

drivers/target/iscsi/iscsi_target.c:iscsit_close_session() {
    ...
	if (iscsit_check_session_usage_count(sess, can_sleep)) {
		atomic_set(&sess->session_logout, 0);
		iscsit_start_time2retain_handler(sess);
		return 0;
	}
    ...
}

iscsit_start_time2retain_handler() then returns early without re-arming
the timer because ISCSI_TF_RUNNING is still set
(iscsit_stop_time2retain_timer() bails out when ISCSI_TF_EXPIRED is set),
and sess->session_reinstatement also remains set.  When
iscsi_post_login_handler() later sees ISCSI_TF_EXPIRED and drops
sess->session_usage_count, iscsit_close_session() has already returned and
nothing ever closes or frees sess.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794739041.git.danjwade95@gmail.com?part=2