Re: [PATCH v5 33/56] scsi: qla2xxx: Add abort command handling for 29xx series

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition in asynchronous aborts (e.g., ELS DCMD timeouts) leads to a Use-After-Free of the original command SRB (`orig_sp`) and potential memory corruption of newly allocated NVMe commands.
--

--- Patch [33]: [PATCH v5 33/56] scsi: qla2xxx: Add abort command handling for 29xx series ---
commit d0967e5f98d09c6d227bb75d1093a54e5b842954
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Add abort command handling for 29xx series

Extend the abort-IOCB code path to support the 29xx extended
abort_entry_24xx_ext structure alongside the existing
abort_entry_24xx.

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
[ ... ]
> @@ -3973,7 +3973,14 @@ qla24xx_abort_iocb_entry(scsi_qla_host_t *vha, struct req_que *req,
>  		return;
>  
>  	abt = &sp->u.iocb_cmd;
> -	abt->u.abt.comp_status = pkt->comp_status;
> +	/*
> +	 * abort_entry_24xx_ext overlays abort_entry_24xx through the
> +	 * nport_handle/comp_status union at offset 8, so reading
> +	 * comp_status is stride-agnostic and goes through the 24xx view.
> +	 */
> +	abt->u.abt.comp_status =
> +	    ((struct abort_entry_24xx *)pkt)->comp_status;
> +
>  	orig_sp = sp->cmd_sp;
>  	/* Need to pass original sp */
>  	if (orig_sp)

[Severity: High]
This is a pre-existing issue, but could this race condition cause a
use-after-free on orig_sp?

If an SRB_ELS_DCMD or SRB_LOGIN_CMD times out, it triggers an async abort. 
Since wait is false, the timeout handler returns immediately without waiting 
for the abort to complete.

If the firmware completes the original command (e.g., with CS_ABORTED), the 
driver processes the completion, calls the done() callback, and frees the SRB 
back to srb_mempool.

When the firmware later completes the ABT_IOCB, this code reads the now-freed 
orig_sp:

qla24xx_abort_iocb_entry() {
    orig_sp = sp->cmd_sp;
    if (orig_sp)
        qla_nvme_abort_process_comp_status(pkt, orig_sp);
}

If that freed memory was reallocated for an active NVMe command, 
qla_nvme_abort_process_comp_status() dereferences it, and 
qla_wait_nvme_release_cmd_kref() drops the cmd_kref:

qla_wait_nvme_release_cmd_kref() {
    if (!(ql2xabts_wait_nvme && QLA_ABTS_WAIT_ENABLED(orig_sp)))
        return;
    kref_put(&orig_sp->cmd_kref, orig_sp->put_fn);
}

Wouldn't this prematurely free an unrelated active NVMe command and cause 
memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717152904.3155730-1-njavali@marvell.com?part=33