[PATCH] scsi: libiscsi: validate the ITT reflected in a Reject PDU

[PATCH] 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
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
Measured on 7.2-rc5 with KASAN over a proxy that injects one Reject PDU.
Unpatched: the unused index oopses, the returned index warns and
underflows the refcount, and the in-flight read returns 1048576 with 0
bytes filled. With the patch the reject is refused in all three cases
and the connection is failed the way an unknown ITT already is. A
NOP-Out ping reflected in a Reject - the case this branch exists for -
completes normally on both kernels.
 drivers/scsi/libiscsi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 160f02f2f51d..d881f4dc47b4 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1147,8 +1147,11 @@ static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 			 * 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) {
+			task = NULL;
+			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] scsi: libiscsi: validate the ITT reflected in a Reject PDU

[sashiko-bot]

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

New issues:
- [High] The newly added task validation logic in `iscsi_handle_reject` introduces a Time-of-Check to Time-of-Use (TOCTOU) data race against concurrent task allocation.

Pre-existing issues:
- [Critical] The vulnerability preventing malicious target completion of in-flight SCSI commands or free tasks is only patched for `ISCSI_OP_REJECT`, leaving other management opcodes vulnerable to the exact same exploit.
- [Critical] Negative ITT values bypass bounds checking when using drivers like be2iscsi, leading to an out-of-bounds array access on the session cmds array.
- [High] The patch prevents spoofed Reject PDUs from erroneously completing SCSI tasks by checking `!task->sc`, but leaves non-SCSI management tasks (e.g., TMFs, Logout) vulnerable to a double-completion exploit via reallocation.
--

--- Patch [1]: [PATCH] scsi: libiscsi: validate the ITT reflected in a Reject PDU ---
commit bf6ef027cf2ac528cf92e9e76e0a750fc88ecc70
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Subject: scsi: libiscsi: validate the ITT reflected in a Reject PDU

This commit addresses an issue where an invalid ITT in a Reject PDU could
cause a null pointer dereference or complete an active task incorrectly.
It adds a check in iscsi_handle_reject to verify the ITT and ensure the
task is valid and is a SCSI command before cleaning it up.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 6bffd91b973a4..23ae038c54820 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1089,8 +1089,11 @@ static int iscsi_handle_reject(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>  			 * 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) {
> +			task = NULL;
> +			if (!iscsi_verify_itt(conn, rejected_pdu.itt))

[Severity: Critical]
This isn't a bug introduced by this patch, but does the new call to
iscsi_verify_itt() properly handle negative ITT values?

When using drivers like be2iscsi, the network ITT is cast to a signed
integer:

drivers/scsi/be2iscsi/be_main.c:beiscsi_parse_pdu() {
    ...
    *index = (int)itt;
    ...
}

In iscsi_verify_itt() and iscsi_itt_to_task(), the bounds check performs a
signed comparison:

    if (i >= session->cmds_max) {

If an ITT with the MSB set (e.g., 0x80000000) is provided, would this
evaluate to false for negative integers and completely bypass the bounds
check, allowing an out-of-bounds access on session->cmds[i]?

> +				task = iscsi_itt_to_task(conn, rejected_pdu.itt);
> +			if (!task || task->state == ISCSI_TASK_FREE ||
> +			    task->sc) {

[Severity: High]
Does this unprotected lockless read introduce a Time-of-Check to Time-of-Use
(TOCTOU) data race against concurrent task allocation?

Looking at iscsi_alloc_task(), which runs under frwd_lock, it assigns
task->state before setting task->sc without any memory barriers:

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

The RX softirq thread here in iscsi_handle_reject() checks task->state and
task->sc without holding the frwd_lock. If a Reject PDU arrives exactly as a
task is allocated, could the RX thread observe task->state ==
ISCSI_TASK_PENDING and task->sc == NULL, causing this check to evaluate to
false and prematurely complete the task via iscsi_nop_out_rsp()?

[Severity: High]
This is a pre-existing issue, but does this logic leave non-NOOP management
tasks vulnerable to a double-completion exploit?

Since TMF tasks are allocated from cmdpool but have task->sc == NULL, this
new check evaluates to false for an in-flight non-SCSI management task
(like an Abort TMF).

If a target sends a spoofed Reject PDU with the ITT of an in-flight TMF
task, could iscsi_nop_out_rsp() prematurely complete and free it?
Consequently, if the initiator reallocates this freed task for a legitimate
SCSI command and the target finally sends the actual response for the
original TMF, would the new SCSI command be prematurely completed?

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

[Severity: Critical]
This is a pre-existing issue, but is this vulnerability also present for
other management opcodes?

While this patch addresses ISCSI_OP_REJECT, __iscsi_complete_pdu() fetches
the task for other management response opcodes without validating if it is a
free task or an active SCSI command:

__iscsi_complete_pdu() {
    ...
    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 a target sends a management response containing the ITT of a free task or
an active SCSI command, could it completely bypass this patch's logic and
be erroneously passed to completion functions?

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