Re: [PATCH v4 22/56] scsi: qla2xxx: Enable qla2x00_shutdown for 29xx

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Mismatched allocator and deallocator for crc_ctx in the command queuing error paths.
- [High] Logic inversion and Use-After-Free in qla2x00_abort_srb when waiting for firmware abort completion.
--

--- Patch [22]: [PATCH v4 22/56] scsi: qla2xxx: Enable qla2x00_shutdown for 29xx ---
commit f4293733252c862c9a670e30cbe9ec9b3965ea0f
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Enable qla2x00_shutdown for 29xx

Enable qla2x00_shutdown for 29xx adapter by adding IS_QLA29XX check
to the shutdown path that performs firmware abort cleanup.

> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 146701445485f..5450c40259bf9 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -3840,7 +3840,7 @@ qla2x00_shutdown(struct pci_dev *pdev)
>  		qla2x00_disable_eft_trace(vha);
>  
>  	if (IS_QLA25XX(ha) ||  IS_QLA2031(ha) || IS_QLA27XX(ha) ||
> -	    IS_QLA28XX(ha)) {
> +	    IS_QLA28XX(ha) || IS_QLA29XX(ha)) {
>  		if (ha->flags.fw_started)
>  			qla2x00_abort_isp_cleanup(vha);
>  	} else {

[Severity: High]
This is a pre-existing issue, but is it safe to free a DMA pool allocation
using mempool_free()?

In the queuing_error paths of qla24xx_start_scsi() and
qla2xxx_dif_start_scsi_mq() when I/O submission fails, the dma_pool_zalloc'd
crc_ctx is freed using mempool_free():

drivers/scsi/qla2xxx/qla_iocb.c:qla24xx_start_scsi() {
    ...
	mempool_free(sp->u.scmd.crc_ctx, ha->ctx_mempool);
    ...
}

drivers/scsi/qla2xxx/qla_iocb.c:qla2xxx_dif_start_scsi_mq() {
    ...
	mempool_free(sp->u.scmd.crc_ctx, ha->ctx_mempool);
    ...
}

Could this lead to memory corruption or allocator crashes? Furthermore, if
ha->ctx_mempool is null (which happens when dif is enabled but security is
not), could this cause a null pointer dereference?

[Severity: High]
This isn't a bug introduced by this patch, but does this logic inversion
cause a use-after-free and potential stack corruption?

Looking at qla2x00_abort_srb() when waiting for firmware abort completion:

drivers/scsi/qla2xxx/qla_os.c:qla2x00_abort_srb() {
    ...
	if (wait_for_completion_timeout(&comp, ratov_j)) {
		ql_dbg(ql_dbg_taskm, vha, 0xffff,
		    "%s: Abort wait timer (4 * R_A_TOV[%d]) expired\n",
		    __func__, ha->r_a_tov/10);
		ret_cmd = true;
	}
    ...
}

Since wait_for_completion_timeout() returns > 0 on successful completion,
is a success treated as a timeout here due to a missing logical NOT (!)?

Because successful completion means the interrupt handler already freed the
srb (sp), will the function then dereference sp->type and call
sp->done(sp, res), resulting in a use-after-free?

Conversely, if a genuine timeout occurs and wait_for_completion_timeout()
returns 0, will it skip this block entirely? Would this leave sp->comp
pointing to the stack-allocated comp, causing stack corruption when the
firmware eventually responds?

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