[PATCH v3 0/3] scsi: libiscsi: validate task lookups driven by target-supplied ITTs

[PATCH v3 0/3] scsi: libiscsi: validate task lookups driven by target-supplied ITTs

[Yehyeong Lee]

Three places where an ITT taken from a target-supplied PDU reaches a task
without being checked. Each is reachable from the wire with one injected
PDU and each is independently useful, but they share one lookup helper so
they are sent together.

1/3 bounds the index from below. A transport that implements
parse_pdu_itt can produce a negative one; be2iscsi does, and forwards an
unsolicited NOP-In from the hardware with the target's ITT intact. I
have no be2iscsi hardware, so this one is argued from source.

2/3 is the ITT reflected in a Reject PDU.

3/3 is the same defect at the lookup that five management responses
share. An unsolicited NOP-In naming an unused index dereferences a NULL
task->conn; a Text Response with the same ITT crashes a little later in
iscsi_complete_task().

Measured on 7.2-rc5 with KASAN over a proxy that injects one PDU. Five
attack shapes oops or warn unpatched and none of them do with the
series. Normal I/O, an abort TMF, a rejected NOP-Out ping, a userspace
nop sent over netlink and an iscsid-driven session are unchanged.

v3: 2/3 - initialise task at its declaration per Mike Christie's review.
    No functional change: the .text of libiscsi.o is byte-identical
    before and after. Picked up Mike Christie's Reviewed-by on all three
    patches.
v2: https://lore.kernel.org/all/20260804040546.2264137-1-yhlee@isslab.korea.ac.kr/
v1: https://lore.kernel.org/all/20260803104149.2114414-1-yhlee@isslab.korea.ac.kr/

Yehyeong Lee (3):
  scsi: libiscsi: reject a negative task index from parse_pdu_itt
  scsi: libiscsi: validate the ITT reflected in a Reject PDU
  scsi: libiscsi: validate the task named by a management response

 drivers/scsi/libiscsi.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)


base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
-- 
2.43.0

[PATCH v3 1/3] scsi: libiscsi: reject a negative task index from parse_pdu_itt

[Yehyeong Lee]

A transport that implements parse_pdu_itt hands libiscsi an index taken
from the PDU, and both lookups bound it from above only:

      if (i >= session->cmds_max)

i and cmds_max are both int, so a negative index passes and
session->cmds[i] is read from before the array.

be2iscsi produces one. beiscsi_parse_pdu() assigns the raw tag,

      *index = (int)itt;

and beiscsi_complete_pdu() forwards an unsolicited NOP-In from the
hardware async ring without replacing its ITT, so the value is the
target's. It also reports the session's own age rather than the one in
the tag, which leaves the age comparison in iscsi_verify_itt() with
nothing to reject.

Bound the index from below in both lookups.

Fixes: bfead3b2cb46 ("[SCSI] be2iscsi: Adding msix and mcc_rings V3")
Cc: stable@vger.kernel.org
Reviewed-by: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/libiscsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 160f02f2f51d1..7a74bc697d23b 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1191,7 +1191,7 @@ struct iscsi_task *iscsi_itt_to_task(struct iscsi_conn *conn, itt_t itt)
 		session->tt->parse_pdu_itt(conn, itt, &i, NULL);
 	else
 		i = get_itt(itt);
-	if (i >= session->cmds_max)
+	if (i < 0 || i >= session->cmds_max)
 		return NULL;
 
 	return session->cmds[i];
@@ -1384,7 +1384,7 @@ int iscsi_verify_itt(struct iscsi_conn *conn, itt_t itt)
 		return ISCSI_ERR_BAD_ITT;
 	}
 
-	if (i >= session->cmds_max) {
+	if (i < 0 || i >= session->cmds_max) {
 		iscsi_conn_printk(KERN_ERR, conn,
 				  "received invalid itt index %u (max cmds "
 				   "%u.\n", i, session->cmds_max);
-- 
2.43.0

[PATCH v3 2/3] scsi: libiscsi: validate the ITT reflected in a Reject PDU

[Yehyeong Lee]

A Reject PDU carries a copy of the header it rejects, and
iscsi_handle_reject() takes the ITT out of that copy to find the task to
clean up:

      memcpy(&rejected_pdu, data, sizeof(struct iscsi_hdr));
      ...
      task = iscsi_itt_to_task(conn, rejected_pdu.itt);

That value is whatever the target put there. iscsi_itt_to_task() bounds
the index against cmds_max and checks nothing else, so any task in the
pool can be named, and iscsi_nop_out_rsp() then completes it.

An index that has never been used gives a NULL task->conn: the pool is
zeroed at session setup and conn is assigned only when a task is
allocated. If the task was used and returned, iscsi_complete_task()
hits its WARN_ON_ONCE(task->state == ISCSI_TASK_FREE) and the refcount
underflows. An in-flight SCSI command is completed as successful - a
1 MiB read returned 1048576 with none of its buffer written and no
warning.

Validate the reflected ITT the way iscsi_itt_to_ctask() validates a
command ITT, and require the task to be in flight and not a SCSI
command.

[    6.248477] Oops: general protection fault, probably for non-canonical address 0xdffffc000000000c: 0000 [#1] SMP KASAN NOPTI
[    6.249357] KASAN: null-ptr-deref in range [0x0000000000000060-0x0000000000000067]
[    6.249951] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Not tainted 7.2.0-rc5-ISCSI1-gf5098b6bae76 #1 PREEMPT(lazy)
[    6.250718] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[    6.251616] RIP: 0010:iscsi_nop_out_rsp.constprop.0+0x46/0x160
[    6.252043] Code: c1 ea 03 48 83 ec 08 80 3c 02 00 0f 85 f5 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 58 48 8d 7d 60 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 00 00 00 48 8b 45 60 48 39 c3 74 50 48 b8 00
[    6.252813] RSP: 0018:ffff88806c907b80 EFLAGS: 00010206
[    6.253042] RAX: dffffc0000000000 RBX: ffff88800607e000 RCX: ffffffff8d571a25
[    6.253345] RDX: 000000000000000c RSI: ffff88806c907c38 RDI: 0000000000000060
[    6.253642] RBP: 0000000000000000 R08: 0000000000000000 R09: fffffbfff235a504
[    6.253947] R10: 0000000000000003 R11: 7463656e6e6f6320 R12: 0000000000000000
[    6.254251] R13: 0000000000000000 R14: ffff88806c907c38 R15: 0000000000000000
[    6.254549] FS:  0000000000000000(0000) GS:ffff8880d95bc000(0000) knlGS:0000000000000000
[    6.254887] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    6.255140] CR2: 00007fc393e8ffc0 CR3: 0000000005252006 CR4: 0000000000770ef0
[    6.255439] PKRU: 55555554
[    6.255564] Call Trace:
[    6.255674]  <IRQ>
[    6.255768]  __iscsi_complete_pdu+0x18bf/0x22a0
[    6.256733]  iscsi_complete_pdu+0x54/0xa0
[    6.256905]  iscsi_tcp_data_recv_done+0xf4/0x250
[    6.257103]  iscsi_tcp_recv_skb+0x31e/0xec0
[    6.257680]  iscsi_sw_tcp_recv+0x12f/0x390
[    6.258061]  __tcp_read_sock+0x1ab/0x810
[    6.258853]  iscsi_sw_tcp_data_ready+0x18b/0x510
[    6.259808]  tcp_data_queue+0x1f13/0x4cd0
[    6.260567]  tcp_rcv_established+0x8a5/0x3a00
[    6.261931]  tcp_v4_do_rcv+0x449/0x960
[    6.262269]  tcp_v4_rcv+0x2245/0x3bc0

Fixes: 8afa1439fcff ("[SCSI] libiscsi: handle immediate command rejections")
Cc: stable@vger.kernel.org
Reviewed-by: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/libiscsi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 7a74bc697d23b..ffd6446d07b28 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1142,13 +1142,15 @@ static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 			spin_unlock(&conn->session->frwd_lock);
 			spin_lock(&conn->session->back_lock);
 		} else {
-			struct iscsi_task *task;
+			struct iscsi_task *task = NULL;
 			/*
 			 * Our nop as ping got dropped. We know the target
 			 * and transport are ok so just clean up
 			 */
-			task = iscsi_itt_to_task(conn, rejected_pdu.itt);
-			if (!task) {
+			if (!iscsi_verify_itt(conn, rejected_pdu.itt))
+				task = iscsi_itt_to_task(conn, rejected_pdu.itt);
+			if (!task || task->state == ISCSI_TASK_FREE ||
+			    task->sc) {
 				iscsi_conn_printk(KERN_ERR, conn,
 						 "Invalid pdu reject. Could "
 						 "not lookup rejected task.\n");
-- 
2.43.0

Re: [PATCH v3 2/3] scsi: libiscsi: validate the ITT reflected in a Reject PDU

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: libiscsi: TOCTOU race allows malicious target to corrupt SCSI task via Reject PDU
--

commit 0da359490f3ff756111324f1c5b68673b764be99
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: libiscsi: validate the ITT reflected in a Reject PDU

This commit adds validation to the ITT reflected in a Reject PDU to prevent
naming an unused or free task from the pool. It introduces checks to ensure
the task state is not ISCSI_TASK_FREE and that task->sc is NULL.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 7a74bc697d23b..ffd6446d07b28 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1142,13 +1142,15 @@ static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>  			spin_unlock(&conn->session->frwd_lock);
>  			spin_lock(&conn->session->back_lock);
>  		} else {
> -			struct iscsi_task *task;
> +			struct iscsi_task *task = NULL;
>  			/*
>  			 * Our nop as ping got dropped. We know the target
>  			 * and transport are ok so just clean up
>  			 */
> -			task = iscsi_itt_to_task(conn, rejected_pdu.itt);
> -			if (!task) {
> +			if (!iscsi_verify_itt(conn, rejected_pdu.itt))
> +				task = iscsi_itt_to_task(conn, rejected_pdu.itt);
> +			if (!task || task->state == ISCSI_TASK_FREE ||
> +			    task->sc) {

[Severity: High]
Does this introduce a Time-of-Check to Time-of-Use (TOCTOU) race with task
allocation?

The RX path executing iscsi_handle_reject() holds the session->back_lock,
while the TX path executing iscsi_alloc_task() holds the session->frwd_lock.

During task allocation, iscsi_alloc_task() performs these updates
sequentially:

iscsi_alloc_task() {
    ...
    task->state = ISCSI_TASK_PENDING;
    task->conn = conn;
    task->sc = sc;
    ...
}

Because these paths hold different locks, the RX path can observe an
incomplete task allocation where task->state is ISCSI_TASK_PENDING but
task->sc has not yet been assigned and is still NULL.

If a malicious target sends an ISCSI_OP_REJECT PDU matching the ITT of a task
currently being allocated, couldn't this check be completely bypassed?

This would falsely identify an in-flight SCSI task as a management task,
leading to premature completion via iscsi_nop_out_rsp().

This could eventually cause a refcount underflow and use-after-free when the
task actually completes.

>  				iscsi_conn_printk(KERN_ERR, conn,
>  						 "Invalid pdu reject. Could "
>  						 "not lookup rejected task.\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007015842.634893-1-yhlee@isslab.korea.ac.kr?part=2

Re: [PATCH v3 2/3] scsi: libiscsi: validate the ITT reflected in a Reject PDU

[Yehyeong Lee]

Thanks for looking at this.

The reflected ITT here is resolved with iscsi_itt_to_task(), which
returns a fixed slot in the static command pool (session->cmds[i])
and is non-NULL for any in-range index. The previous check was
"if (!task)", so a valid-range ITT was never rejected: before this
change a Reject PDU could already drive any pool slot to completion,
in any state, including a fully set-up in-flight SCSI command. This
patch rejects slots that are free or carry an active SCSI command,
which strictly reduces that set rather than widening it.

The window you point at is real but predates this change: in
iscsi_alloc_task() a slot is briefly ISCSI_TASK_PENDING with task->sc
not yet assigned (freed slots are cleared to sc == NULL), and the
allocation runs under frwd_lock while this check runs under back_lock.
That window is unchanged by this patch and is independent of the NULL
task->conn dereference fixed here.

Closing it is a separate change -- e.g. assigning task->state last in
iscsi_alloc_task(), after task->sc, so a findable slot is never
PENDING with a NULL sc. Happy to send that as a follow-up.

[PATCH v3 3/3] scsi: libiscsi: validate the task named by a management response

[Yehyeong Lee]

__iscsi_complete_pdu() fetches the task for five response types from one
place and checks only that the index resolved:

      case ISCSI_OP_LOGOUT_RSP:
      case ISCSI_OP_LOGIN_RSP:
      case ISCSI_OP_TEXT_RSP:
      case ISCSI_OP_SCSI_TMFUNC_RSP:
      case ISCSI_OP_NOOP_IN:
              task = iscsi_itt_to_task(conn, hdr->itt);
              if (!task)
                      return ISCSI_ERR_BAD_ITT;

iscsi_itt_to_task() bounds the index against cmds_max and looks at
nothing else, so a target can name any slot in the pool, including one
that has never been used. task->conn is then NULL, and both
iscsi_nop_out_rsp() and iscsi_complete_task() dereference it. One
unsolicited NOP-In is enough; a Text Response carrying the same ITT
crashes in iscsi_complete_task() instead.

Require the task to be in flight and not a SCSI command, the way
iscsi_itt_to_ctask() does for the command opcodes.

[    6.298634] Oops: general protection fault, probably for non-canonical address 0xdffffc000000000c: 0000 [#1] SMP KASAN NOPTI
[    6.298642] KASAN: null-ptr-deref in range [0x0000000000000060-0x0000000000000067]
[    6.298652] CPU: 1 UID: 0 PID: 111 Comm: iscsistart Not tainted 7.2.0-rc5-ISCSI1-gf5098b6bae76 #1 PREEMPT(lazy)
[    6.298654] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[    6.298656] RIP: 0010:iscsi_nop_out_rsp.constprop.0+0x46/0x160
[    6.298691] Code: c1 ea 03 48 83 ec 08 80 3c 02 00 0f 85 f5 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 58 48 8d 7d 60 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c0 00 00 00 48 8b 45 60 48 39 c3 74 50 48 b8 00
[    6.298698] RSP: 0018:ffff88806c907be8 EFLAGS: 00010206
[    6.298701] RAX: dffffc0000000000 RBX: ffff888006116800 RCX: 0000000000000020
[    6.298702] RDX: 000000000000000c RSI: ffff88800525d578 RDI: 0000000000000060
[    6.298703] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[    6.298706] R10: 0000000000000020 R11: 0000000000000000 R12: 0000000000000000
[    6.298706] R13: 000000000000002e R14: ffff8880056eb440 R15: ffff888006116800
[    6.298707] FS:  00007fc02d1c2740(0000) GS:ffff8880b29bc000(0000) knlGS:0000000000000000
[    6.298710] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    6.298711] CR2: 000055fe3b56ddf8 CR3: 00000000053b9004 CR4: 0000000000770ef0
[    6.298712] PKRU: 55555554
[    6.298713] Call Trace:
[    6.298714]  <IRQ>
[    6.298714]  __iscsi_complete_pdu+0x13a8/0x22a0
[    6.298758]  iscsi_complete_pdu+0x54/0xa0
[    6.298759]  iscsi_tcp_hdr_recv_done+0x870/0x2c80
[    6.298778]  iscsi_tcp_recv_skb+0x31e/0xec0
[    6.298800]  iscsi_sw_tcp_recv+0x12f/0x390
[    6.298804]  __tcp_read_sock+0x1ab/0x810
[    6.298812]  iscsi_sw_tcp_data_ready+0x18b/0x510
[    6.298814]  tcp_rcv_established+0x1f56/0x3a00
[    6.298842]  tcp_v4_do_rcv+0x449/0x960
[    6.298847]  tcp_v4_rcv+0x2245/0x3bc0

Fixes: 7996a778ff8c ("[SCSI] iscsi: add libiscsi")
Cc: stable@vger.kernel.org
Reviewed-by: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/libiscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index ffd6446d07b28..4329967b54260 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1286,7 +1286,7 @@ int __iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 	case ISCSI_OP_SCSI_TMFUNC_RSP:
 	case ISCSI_OP_NOOP_IN:
 		task = iscsi_itt_to_task(conn, hdr->itt);
-		if (!task)
+		if (!task || task->state == ISCSI_TASK_FREE || task->sc)
 			return ISCSI_ERR_BAD_ITT;
 		break;
 	default:
-- 
2.43.0

Re: [PATCH v3 3/3] scsi: libiscsi: validate the task named by a management response

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: libiscsi: early completion of pending task causes UAF on transmit queue
--

commit b24b8599f28cfc6b3085efa4f6f74df1f0a7f232
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: libiscsi: validate the task named by a management response

__iscsi_complete_pdu() fetches the task for five response types from one
place and checks only that the index resolved. This commit requires the
task to be in flight and not a SCSI command.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index ffd6446d07b28..4329967b54260 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1286,7 +1286,7 @@ int __iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>  	case ISCSI_OP_SCSI_TMFUNC_RSP:
>  	case ISCSI_OP_NOOP_IN:
>  		task = iscsi_itt_to_task(conn, hdr->itt);
> -		if (!task)
> +		if (!task || task->state == ISCSI_TASK_FREE || task->sc)

[Severity: High]
Does this allow an early completion of a pending task? By checking explicitly
for ISCSI_TASK_FREE, tasks in the ISCSI_TASK_PENDING state are accepted. If a
target sends an early management response matching an ITT that was just
allocated, the task gets completed.

If the task is completed while pending, iscsi_complete_task() will drop its
refcount to 0, returning it to the pool:

iscsi_complete_task() {
    ...
	/* release get from queueing */
	__iscsi_put_task(task);
}

But it hasn't been unlinked from the transmit queue. When the transmit thread
attempts to process it, couldn't this result in a use-after-free or list
corruption?

iscsi_data_xmit() {
    ...
	/* process pending command queue */
	while (!list_empty(&conn->cmdqueue)) {
		task = list_entry(conn->cmdqueue.next, struct iscsi_task,
				  running);
		list_del_init(&task->running);
    ...
}

Additionally, is there a race condition here? iscsi_alloc_task() updates the
task state and sc pointer sequentially under frwd_lock:

iscsi_alloc_task() {
    ...
	task->state = ISCSI_TASK_PENDING;
	task->conn = conn;
	task->sc = sc;
    ...
}

Since __iscsi_complete_pdu() performs its validation under back_lock, could
the RX thread observe an intermediate state where task->state is
ISCSI_TASK_PENDING but task->sc is still NULL, thus bypassing the validation
check even for SCSI tasks?

>  			return ISCSI_ERR_BAD_ITT;
>  		break;
>  	default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007015842.634893-1-yhlee@isslab.korea.ac.kr?part=3

Re: [PATCH v3 3/3] scsi: libiscsi: validate the task named by a management response

[Yehyeong Lee]

Thanks for looking at this.

The management-response ITT is resolved with iscsi_itt_to_task(),
which returns a fixed slot in the static command pool
(session->cmds[i]) and is non-NULL for any in-range index. The
previous check was "if (!task)", so a valid-range ITT was never
rejected: before this change a management response could already
drive any pool slot to completion, in any state. This patch rejects
slots that are free or carry an active SCSI command, which strictly
reduces that set.

On the two paths you raise:

- The allocation window -- a slot briefly ISCSI_TASK_PENDING with
  task->sc not yet assigned in iscsi_alloc_task() (freed slots are
  cleared to sc == NULL), allocation under frwd_lock and this check
  under back_lock -- is unchanged by this patch; it pre-exists and
  sits outside the NULL task->conn dereference and the
  ISCSI_TASK_FREE refcount underflow fixed here.
- Completing a still-queued PENDING task was likewise already
  reachable under the old "if (!task)"; this patch does not widen it.

Fully closing the allocation window is a separate change -- e.g.
assigning task->state last in iscsi_alloc_task(), after task->sc, so a
findable slot is never PENDING with a NULL sc. Happy to send that as a
follow-up.