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

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 <[email protected]>

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/[email protected]?part=2