Re: [PATCH] scsi: libiscsi: only complete a task that has been sent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] scsi: libiscsi: premature ISCSI_TASK_RUNNING store before xmit
  ref acquisition
- [High] scsi: libiscsi: RX completion of RESERVED_ITT tasks marked
  ISCSI_TASK_RUNNING
--

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

scsi: libiscsi: only complete a task that has been sent

This commit pairs smp_store_release() when marking a task
ISCSI_TASK_RUNNING with smp_load_acquire() in the RX task lookups so
management responses and Reject PDUs only complete tasks that have been
prepared for transmission.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 2d04448432a77..83d726043d3e8 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c

[ ... ]

> @@ -702,7 +703,8 @@ static int iscsi_prep_mgmt_task(struct iscsi_conn *conn,
>  	if ((hdr->opcode & ISCSI_OPCODE_MASK) == ISCSI_OP_LOGOUT)
>  		session->state = ISCSI_STATE_LOGGING_OUT;
>  
> -	task->state = ISCSI_TASK_RUNNING;
> +	/* Pairs with the acquire in the RX task lookups. */
> +	smp_store_release(&task->state, ISCSI_TASK_RUNNING);
>  	ISCSI_DBG_SESSION(session, "mgmtpdu [op 0x%x hdr->itt 0x%x "
>  			  "datalen %d]\n", hdr->opcode & ISCSI_OPCODE_MASK,
>  			  hdr->itt, task->data_count);

[Severity: High]
Can the RX path still complete and free task after smp_store_release() here
before the transmit path takes its reference?

A management task dequeued in iscsi_data_xmit() has refcount 1 under
session->frwd_lock and only acquires its transmit reference later in
iscsi_xmit_task() via iscsi_get_task():

CPU0 (kworker)                    CPU1 (softirq)
iscsi_data_xmit()
  spin_lock_bh(&session->frwd_lock)
  iscsi_prep_mgmt_task()
    smp_store_release(&task->state,
                      ISCSI_TASK_RUNNING)
                                  __iscsi_complete_pdu()
                                    spin_lock(&session->back_lock)
                                    smp_load_acquire(&task->state)
                                    iscsi_complete_task(task, ...)
                                      __iscsi_put_task()
                                        iscsi_free_task()
  iscsi_xmit_task()
    iscsi_get_task(task)
      WARN_ON_ONCE(1)             <-- refcount already 0

On transports without ihost->workq, iscsi_send_mgmt_task() calls
iscsi_prep_mgmt_task() and session->tt->xmit_task(task) with refcount 1, so
an RX completion in that window frees task during xmit_task() and causes
the caller's error-path iscsi_put_task() to underflow refcount and double
free the slot.

> @@ -1174,7 +1176,8 @@ static int iscsi_handle_reject(struct iscsi_session *session,
>  			if (!iscsi_verify_itt(conn, rejected_pdu.itt))
>  				task = iscsi_itt_to_task(session, conn,
>  							 rejected_pdu.itt);
> -			if (!task || task->state == ISCSI_TASK_FREE ||
> +			/* Pairs with the release when the task is prepared. */
> +			if (!task || smp_load_acquire(&task->state) != ISCSI_TASK_RUNNING ||
>  			    task->sc) {
>  				iscsi_conn_printk(KERN_ERR, conn,
>  						 "Invalid pdu reject. Could "
> @@ -1314,7 +1317,9 @@ int __iscsi_complete_pdu(struct iscsi_session *session, struct iscsi_conn *conn,
>  	case ISCSI_OP_SCSI_TMFUNC_RSP:
>  	case ISCSI_OP_NOOP_IN:
>  		task = iscsi_itt_to_task(session, conn, hdr->itt);
> -		if (!task || task->state == ISCSI_TASK_FREE || task->sc)
> +		/* Pairs with the release when the task is prepared. */
> +		if (!task || smp_load_acquire(&task->state) != ISCSI_TASK_RUNNING ||
> +		    task->sc)
>  			return ISCSI_ERR_BAD_ITT;
>  		break;
>  	default:

[Severity: High]
Can an incoming management response or Reject PDU complete a management
task that was sent with task->hdr->itt == RESERVED_ITT?

When replying to a target NOP-In, iscsi_send_nopout() allocates a task from
session->cmdpool with hdr->itt = RESERVED_ITT, and iscsi_prep_mgmt_task()
still sets task->state to ISCSI_TASK_RUNNING.  Transports free RESERVED_ITT
tasks on transmit because no response is expected:

drivers/scsi/libiscsi_tcp.c:iscsi_tcp_task_xmit() {
    ...
	/* mgmt command */
	if (!task->sc) {
		if (task->hdr->itt == RESERVED_ITT)
			iscsi_put_task(task);
		return 0;
	}
    ...
}

Because iscsi_itt_to_task() indexes session->cmds[] by the pool index in the
received PDU's ITT without checking task->hdr->itt, a PDU naming that pool
index while iscsi_xmit_task() holds its transmit reference (or while the
task is requeued after -EAGAIN) will pass the ISCSI_TASK_RUNNING check and
call iscsi_complete_task(), dropping a reference that the transport also
drops and causing a refcount underflow and use-after-free.

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