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

[PATCH 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 on mainline against a target with authentication=0
and one explicit ACL.  Compile tested on x86_64.

A reproducer is available on request.

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 | 66 ++++++++++++++++++++++-
 drivers/target/iscsi/iscsi_target_login.h |  1 +
 drivers/target/iscsi/iscsi_target_nego.c  | 17 +-----
 4 files changed, 74 insertions(+), 16 deletions(-)

-- 
2.39.0.windows.2

[PATCH 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 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 and
refuse the login when it is set, dropping only the reference the login
took so iscsit_close_session() can finish the free it committed to.

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

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

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 | 41 +++++++++++++++++++++++
 2 files changed, 47 insertions(+)

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..de0cd95aff0 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)
+{
+	/*
+	 * iscsi_target_tx_thread() only leaves cleanup to this context while
+	 * conn_state is TARG_CONN_STATE_IN_LOGIN; otherwise it would reach
+	 * iscsit_close_connection() and walk the session being refused.
+	 */
+	conn->conn_state = TARG_CONN_STATE_IN_LOGIN;
+	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,
@@ -690,6 +717,20 @@ void iscsi_post_login_handler(
 		conn->param_list = NULL;
 
 		spin_lock_bh(&sess->conn_lock);
+		/*
+		 * The skip filter in iscsi_login_non_zero_tsih_s2() sampled this
+		 * before the negotiation.  Re-test under the lock that serialises
+		 * against the ->nconn test in iscsit_close_connection().
+		 */
+		if (atomic_read(&sess->session_close)) {
+			spin_unlock_bh(&sess->conn_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"
-- 
2.39.0.windows.2

Re: [PATCH 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 2 potential
issues to consider:
- [High] scsi: target: iscsi: incomplete teardown check in
  iscsi_post_login_handler()
- [Medium] scsi: target: iscsi: premature LOGGED_IN state in
  iscsi_post_login_handler()
--

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

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

This commit sets sess->session_close under sess->conn_lock when the last
connection closes in the ERL=0 fallback path and re-checks
sess->session_close in iscsi_post_login_handler() before attaching a
non-leading connection to the session.

> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index f4df551817116..de0cd95aff017 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)
> +{
> +	/*
> +	 * iscsi_target_tx_thread() only leaves cleanup to this context while
> +	 * conn_state is TARG_CONN_STATE_IN_LOGIN; otherwise it would reach
> +	 * iscsit_close_connection() and walk the session being refused.
> +	 */
> +	conn->conn_state = TARG_CONN_STATE_IN_LOGIN;
> +	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);
> +}

[Severity: Medium]
Can setting conn->conn_state = TARG_CONN_STATE_LOGGED_IN at the start of
iscsi_post_login_handler() and only resetting it back to
TARG_CONN_STATE_IN_LOGIN here deadlock with conn->tx_thread?

iscsi_post_login_handler() increments conn->conn_usage_count and sets
conn->conn_state = TARG_CONN_STATE_LOGGED_IN before taking sess->conn_lock
to check sess->session_close:

drivers/target/iscsi/iscsi_target_login.c:iscsi_post_login_handler() {
	iscsit_inc_conn_usage_count(conn);

	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
	 */
	if (!zero_tsih) {
		iscsi_set_session_parameters(sess->sess_ops,
				conn->param_list, 0);
}

If an asynchronous transport disconnect sends SIGINT to conn->tx_thread
during that window (for example via isert_disconnected_handler() ->
iscsit_cause_connection_reinstatement()), tx_thread sees conn_state !=
TARG_CONN_STATE_IN_LOGIN and enters iscsit_close_connection() on the
unattached connection:

CPU0 (login worker)               CPU1 (conn->tx_thread)
iscsi_post_login_handler()
  iscsit_inc_conn_usage_count()
  conn->conn_state =
    TARG_CONN_STATE_LOGGED_IN
                                  iscsi_target_tx_thread()
                                    if (conn_state !=
                                        TARG_CONN_STATE_IN_LOGIN)
                                      iscsit_take_action_...()
                                        iscsit_close_connection()
                                          iscsit_check_conn_usage_count()
                                            wait_for_completion()
  iscsi_post_login_fail_conn()
    conn->conn_state =
      TARG_CONN_STATE_IN_LOGIN
    iscsit_stop_kthreads()
      kthread_stop(tx_thread)  <-- deadlocks waiting for tx_thread
    iscsit_dec_conn_usage_count()

In addition, iscsi_set_session_parameters() mutates sess->sess_ops and
iscsit_collect_login_stats() records an accepted login before the session
is checked under sess->conn_lock.  Checking whether the session is closing
before transitioning conn->conn_state or mutating session state would
avoid this window.

>  void iscsi_post_login_handler(
>  	struct iscsi_np *np,
>  	struct iscsit_conn *conn,
> @@ -690,6 +717,20 @@ void iscsi_post_login_handler(
>  		conn->param_list = NULL;
>  
>  		spin_lock_bh(&sess->conn_lock);
> +		/*
> +		 * The skip filter in iscsi_login_non_zero_tsih_s2() sampled this
> +		 * before the negotiation.  Re-test under the lock that serialises
> +		 * against the ->nconn test in iscsit_close_connection().
> +		 */
> +		if (atomic_read(&sess->session_close)) {
> +			spin_unlock_bh(&sess->conn_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"

[Severity: High]
Does checking only sess->session_close here still allow a non-leading
connection to attach to a session that is already tearing down?

iscsi_login_non_zero_tsih_s2() skips sessions that have any of
session_fall_back_to_erl0, session_logout, session_close or
ISCSI_TF_EXPIRED set, but iscsi_post_login_handler() only re-checks
session_close, and iscsit_close_connection() only sets session_close when
sess->nconn reaches 0 in the ERL=0 fallback branch.

For example, if a session has one active connection (conn1) when conn2
passes iscsi_login_non_zero_tsih_s2(), sess->session_state is
TARG_SESS_STATE_LOGGED_IN, so sess->session_continuation remains 0 while
conn2 negotiates holding session_usage_count.  If conn1 then closes via a
connection logout (or ERL=2 failure) and the Time2Retain timer expires,
iscsit_handle_time2retain_timeout() sets ISCSI_TF_EXPIRED and calls
iscsit_close_session(sess, false) without setting sess->session_close:

CPU0 (conn1 / timer)              CPU1 (conn2 login)
iscsit_close_connection(conn1)
  sess->session_state =
    TARG_SESS_STATE_FAILED
  iscsit_start_time2retain_...()
<softirq>
  iscsit_handle_time2retain_...()
    sess->time2retain_timer_flags |=
      ISCSI_TF_EXPIRED
    iscsit_close_session(sess, false)
                                  iscsi_post_login_handler(conn2)
                                    spin_lock_bh(&sess->conn_lock)
                                    atomic_read(&sess->session_close) == 0
                                    list_add_tail(&conn2->conn_list, ...)
                                    atomic_inc(&sess->nconn)
                                    spin_unlock_bh(&sess->conn_lock)
      if (atomic_read(&sess->nconn))
        BUG()  <-- kernel panic (or kfree(sess) if past the nconn check)

Similarly, in a multi-connection session (nconn == 2) falling back to
ERL=0, when the first connection closes it sets session_fall_back_to_erl0
and session_stop_active under sess->conn_lock without setting
session_close, and calls iscsit_stop_session().  If conn2 attaches in
iscsi_post_login_handler() before the second connection takes
sess->conn_lock in iscsit_close_connection(), the second connection
decrements nconn from 2 to 1, sees session_stop_active is already 1 and
returns early:

drivers/target/iscsi/iscsi_target.c:iscsit_close_connection() {
	if (atomic_read(&sess->nconn)) {
		if (!atomic_read(&sess->session_reinstatement) &&
		    !atomic_read(&sess->session_fall_back_to_erl0)) {
			spin_unlock_bh(&sess->conn_lock);
			return 0;
		}
		if (!atomic_read(&sess->session_stop_active)) {
			atomic_set(&sess->session_stop_active, 1);
			spin_unlock_bh(&sess->conn_lock);
			iscsit_stop_session(sess, 0, 0);
			return 0;
		}
		spin_unlock_bh(&sess->conn_lock);
		return 0;
	}
}

That leaves conn2 stranded on a session with session_fall_back_to_erl0
set, which iscsit_release_sessions_for_tpg() and lio_tpg_close_session()
both skip during TPG teardown.  A CLOSESESSION logout also sets
sess->session_logout under sess->conn_lock in iscsit_logout_closesession()
before sess->session_close is set in
iscsit_logout_post_handler_closesession().

Re-checking the full teardown condition (session_close,
session_fall_back_to_erl0, session_logout, and ISCSI_TF_EXPIRED under
se_tpg->session_lock) in iscsi_post_login_handler() would close these
windows as well.

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

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

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 de0cd95aff0..1f41e5274f2 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1168,7 +1168,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