Re: [PATCH v4 40/56] scsi: qla2xxx: Fix queue teardown NULL dma_free and bitmap locking

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cancellation of `qpair->q_work` during queue pair teardown leads to Use-After-Free of `qpair`.
- [High] DMA memory for queues is freed before safely clearing the queue pointers, leading to a race condition and Use-After-Free by concurrent readers like firmware dump.
- [High] Hardware-supplied queue indices are used to index `ha->req_q_map` prior to bounds validation, leading to Out-of-Bounds Read.
- [Medium] Host memory and DMA mappings are leaked during teardown if queues were allocated but not started in hardware.
--

--- Patch [40]: [PATCH v4 40/56] scsi: qla2xxx: Fix queue teardown NULL dma_free and bitmap locking ---
commit 00cbb695fac148d9ec27ea62ef3a784eac6e57fb
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Fix queue teardown NULL dma_free and bitmap locking

This patch fixes a potential panic when freeing uninitialized DMA memory on
error paths. It also adds locking to prevent concurrent queue ID allocation
races.

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index bb2521c052bf8..7072af5b42178 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -576,16 +576,17 @@ qla25xx_free_req_que(struct scsi_qla_host *vha, struct req_que *req)
>  	uint16_t que_id = req->id;
>  	size_t req_entry_size = qla_req_entry_size(ha);
>  
> -	dma_free_coherent(&ha->pdev->dev,
> -			  (req->length + 1) * req_entry_size,
> -			  req->ring, req->dma);
> +	if (req->ring)
> +		dma_free_coherent(&ha->pdev->dev,
> +				  (req->length + 1) * req_entry_size,
> +				  req->ring, req->dma);

[Severity: High]
This is a pre-existing issue, but does freeing the DMA memory here before
clearing the queue pointer create a race condition?

If a firmware dump occurs concurrently (e.g., triggered by a mailbox timeout),
it executes qla27xx_fwdt_entry_t263() under hardware_lock and reads
from ha->req_q_map[i]. It could then copy from req->ring via
qla27xx_insertbuf() after the memory has been freed here.

Would it be safer to nullify ha->req_q_map[que_id] before calling
dma_free_coherent()?

>  	req->ring = NULL;
>  	req->dma = 0;
>  	if (que_id) {
> +		mutex_lock(&ha->mq_lock);
>  		ha->req_q_map[que_id] = NULL;
> -		mutex_lock(&ha->vport_lock);
>  		clear_bit(que_id, ha->req_qid_map);
> -		mutex_unlock(&ha->vport_lock);
> +		mutex_unlock(&ha->mq_lock);
>  	}
>  	kfree(req->outstanding_cmds);
>  	kfree(req);

[ ... ]

> @@ -605,16 +606,17 @@ qla25xx_free_rsp_que(struct scsi_qla_host *vha, struct rsp_que *rsp)
>  		rsp->msix->handle = NULL;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a pending
work item to execute after the qpair is destroyed?

The interrupt handler qla2xxx_msix_rsp_q() queues qpair->q_work on the
workqueue. During teardown, we free the IRQ here, but the already-queued
work doesn't appear to be flushed or cancelled before qla2xxx_delete_qpair()
frees the qpair structure.

When the work eventually executes, qla_do_work() resolves the qpair:

qla_do_work() {
    struct qla_qpair *qpair = container_of(work, struct qla_qpair, q_work);
    ...
}

Could this lead to a use-after-free if the work runs after the qpair is
freed?

> -	dma_free_coherent(&ha->pdev->dev,
> -			  (rsp->length + 1) * rsp_entry_size,
> -			  rsp->ring, rsp->dma);
> +	if (rsp->ring)
> +		dma_free_coherent(&ha->pdev->dev,
> +				  (rsp->length + 1) * rsp_entry_size,
> +				  rsp->ring, rsp->dma);

[ ... ]

While reviewing the queue teardown paths, I also noticed a few other
pre-existing issues in the driver:

[Severity: Medium]
This is a pre-existing issue, but does qla25xx_delete_req_que() leak memory
if the queues were allocated but never started?

In qla25xx_delete_req_que():

qla25xx_delete_req_que() {
    ...
    if (req && vha->flags.qpairs_req_created) {
        req->options |= BIT_0;
        ret = qla25xx_init_req_que(vha, req);
        if (ret != QLA_SUCCESS)
            return QLA_FUNCTION_FAILED;

        qla25xx_free_req_que(vha, req);
    }
}

If qpairs_req_created is false (such as during a driver initialization
failure or if startqp was false), the check fails and we never call
qla25xx_free_req_que(), which appears to leak the req structure and its DMA
mappings.

[Severity: High]
This is a pre-existing issue, but is it safe to use the hardware-supplied
queue index before validating it?

In qla2x00_status_entry(), the hardware returns sts->handle which is
used to look up the queue:

qla2x00_status_entry() {
    ...
    que = MSW(sts->handle);
    req = ha->req_q_map[que];

    /* Check for invalid queue pointer */
    if (req == NULL ||
        que >= find_first_zero_bit(ha->req_qid_map, ha->max_req_queues)) {
        ...
}

The index que is used to access ha->req_q_map before checking if it is
within bounds. A similar pattern exists in qlafx00_status_entry() where
req is dereferenced before validation. Could a corrupted handle from
the firmware cause an out-of-bounds read here?

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