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

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

[Yehyeong Lee]

v1 was a single patch for the Reject path. The Sashiko review of it
pointed at two more places where a target-supplied ITT reaches a task
without being checked, and both are now in the series.

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 v1 unchanged: 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.

v1: 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 | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v2 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
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
Not reproduced: I have no be2iscsi hardware. The reachability argument
is in the commit message.
 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 160f02f2f51d..7a74bc697d23 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 v2 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
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 7a74bc697d23..774ed4739891 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

[PATCH v2 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
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
Measured on 7.2-rc5 with KASAN over a proxy that injects one PDU. An
unsolicited NOP-In naming an unused index oopses in 5 of 5 runs and none
of 5 with the patch; a Text Response carrying the same ITT oopses in 5
of 5 and none of 5. A Reject reflecting the ITT of an in-flight abort
TMF warns and underflows the refcount unpatched and does neither with
the series. Normal I/O and a NOP-Out ping rejected by the target are
unchanged.
 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 774ed4739891..d62d6c5ef8d6 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1287,7 +1287,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 v2 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:

Pre-existing issues:
- [High] Incomplete fix for ITT validation allowing remote denial of service and silent data corruption.
--

--- Patch [2]: [PATCH v2 2/3] scsi: libiscsi: validate the ITT reflected in a Reject PDU ---
commit b86c9905a91963794fe78b0e96f89ab99adb9cb4
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

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

This commit fixes an issue where an iSCSI target could cause a kernel panic
or silent data corruption by sending a Reject PDU with an invalid or
inappropriate Initiator Task Tag (ITT). By explicitly validating the ITT
and ensuring the task is active and not a SCSI command, it prevents these
out-of-bounds manipulations.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index ba3f971279c64..f6e7facf296b2 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c

[ ... ]

> @@ -1045,8 +1045,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) {

[Severity: High]
This is a pre-existing issue, but does this same validation need to be
applied to other management responses in __iscsi_complete_pdu()?

Looking at __iscsi_complete_pdu(), when handling opcodes like
ISCSI_OP_NOOP_IN or ISCSI_OP_TEXT_RSP, it retrieves the task blindly:

drivers/scsi/libiscsi.c:__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 (!task)
            return ISCSI_ERR_BAD_ITT;
        break;
    ...
}

If a target sends one of these opcodes with an ITT pointing to an unused
task, task->conn will be NULL. This can lead to a NULL pointer dereference
when accessing conn->cls_conn in iscsi_nop_out_rsp() or 
iscsi_complete_task().

Similarly, if the ITT points to an active SCSI task, iscsi_complete_task()
could incorrectly force its state to ISCSI_TASK_COMPLETED. Could this allow
an in-flight SCSI read to complete without transferring data?

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

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

Re: [PATCH v2 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:

Pre-existing issues:
- [High] iscsi_complete_pdu() acquires session->lock with spin_lock(), but can be called from process context, leading to a deadlock against softirqs.
--

--- Patch [3]: [PATCH v2 3/3] scsi: libiscsi: validate the task named by a management response ---
commit 5135af06e5540d3c52932cc2310608b06242c034
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 fixes an issue
where task->conn could be NULL by requiring the task to be in flight and
not a SCSI command.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index f6e7facf296b2..b686cd2aa4dd2 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1180,7 +1180,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]
This is a pre-existing issue, but could the lock usage in the caller lead
to a deadlock?

Looking at iscsi_complete_pdu() (which calls __iscsi_complete_pdu()), it uses
spin_lock(&conn->session->lock) instead of spin_lock_bh().

If qla4xxx_task_work() in drivers/scsi/qla4xxx/ql4_os.c is called from
a workqueue (process context), it calls iscsi_complete_pdu() without disabling
softirqs:

drivers/scsi/qla4xxx/ql4_os.c:qla4xxx_task_work() {
    ...
		iscsi_complete_pdu(conn, hdr, data, data_len);
    ...
}

If a timer interrupt fires on the same CPU while the lock is held, the timer
softirq callback iscsi_check_transport_timeouts() will attempt to acquire the
same lock, resulting in a lock inversion deadlock:

drivers/scsi/libiscsi.c:iscsi_check_transport_timeouts() {
    ...
	spin_lock(&session->lock);
    ...
}

Should iscsi_complete_pdu() use spin_lock_bh() to prevent this?

>  			return ISCSI_ERR_BAD_ITT;
>  		break;
>  	default:

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

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

[Yehyeong Lee]

Two notes on the Sashiko reviews of 2/3 and 3/3, both marked pre-existing.

On 2/3, the same validation is indeed needed for the other management
responses in __iscsi_complete_pdu() - that is 3/3 in this series. The
second question there, whether an in-flight SCSI read can complete
without transferring data, is the third case in 2/3's changelog: a 1 MiB
read returned 1048576 with none of its buffer written. The task->sc test
is what blocks it.

On 3/3 the reported lock inversion does not hold. iscsi_complete_pdu()
takes back_lock (libiscsi.c:1358), iscsi_check_transport_timeouts() takes
frwd_lock (libiscsi.c:2249). session->lock does not appear in libiscsi.c;
the only mention left is a stale comment at libiscsi.h:131.