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

[sashiko-bot@kernel.org]

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