Re: [PATCH v5 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] Interrupt handler drops valid interrupts if any lower-numbered queue is deleted, due to misuse of find_first_zero_bit().
- [High] Sysfs firmware dump can concurrently read freed `req->ring` or `rsp->ring` memory.
- [High] Race condition during qpair deletion causes UAF of `req->outstanding_cmds` in the interrupt handler.
- [High] Orphaned request/response queues when deleting queue pairs created with `startqp = false`.
--

--- Patch [40]: [PATCH v5 40/56] scsi: qla2xxx: Fix queue teardown NULL dma_free and bitmap locking ---
commit 9e994d85d8469000c7bf32a3a2ebd3db56b893a4
Author: Nilesh Javali <njavali@marvell.com>
Date:   Fri Jul 17 15:28:48 2026 +0000

    scsi: qla2xxx: Fix queue teardown NULL dma_free and bitmap locking
    
    qla25xx_free_req_que() and qla25xx_free_rsp_que() have two pre-existing
    bugs exposed on the error path of qla25xx_create_{req,rsp}_que():
    
[ ... ]

> 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);
>  	req->ring = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does this create a use-after-free
or NULL pointer dereference during sysfs firmware dumps?

If a firmware dump is triggered concurrently with a queue teardown,
qla25xx_copy_mqueues() copies queue memory into the dump buffer under
hardware_lock. However, here we free the DMA memory and set req->ring to NULL
before the queue is removed from req_q_map, and without holding hardware_lock:

drivers/scsi/qla2xxx/qla_dbg.c:qla25xx_copy_mqueues() {
        ...
        memcpy(ptr, req->ring, req->length * req_entry_size);
        ...
}

>  	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);

[Severity: High]
This is a pre-existing issue, but does clearing bits here expose a flaw with
how interrupts are validated?

If multiple queue pairs are created and a lower-numbered queue is deleted,
this clear_bit creates a hole in the bitmask. When an interrupt arrives for
a higher-numbered queue, qla2x00_status_entry() validates it using:

drivers/scsi/qla2xxx/qla_isr.c:qla2x00_status_entry() {
        ...
        if (req == NULL ||
            que >= find_first_zero_bit(ha->req_qid_map, ha->max_req_queues)) {
        ...
}

The find_first_zero_bit call returns the index of the deleted queue, causing
the check to incorrectly drop valid interrupts for the higher-numbered queues.

> -		mutex_unlock(&ha->vport_lock);
> +		mutex_unlock(&ha->mq_lock);
>  	}
>  	kfree(req->outstanding_cmds);

[Severity: High]
This is a pre-existing issue, but could this cause a use-after-free of
req->outstanding_cmds in the interrupt handler?

During queue pair deletion in qla2xxx_delete_qpair(), the request queue is
deleted and freed here before the response queue is deleted. The response
queue's MSI-X interrupt remains active during this window.

If an interrupt fires (or q_work is executing), it reads req without mq_lock.
If it fetches req just before it is nullified but dereferences
req->outstanding_cmds after it is freed here, a use-after-free occurs.
Additionally, the qpair->q_work work item appears to never be canceled.

>  	kfree(req);
>  }

[Severity: High]
This is a pre-existing issue, but do we leak these request and response
queues when they are created with startqp = false?

If a queue pair is created with startqp = false, vha->flags.qpairs_req_created
is not set. Later, qla25xx_delete_req_que() skips calling qla25xx_free_req_que()
completely because of this check:

drivers/scsi/qla2xxx/qla_mid.c:qla25xx_delete_req_que() {
        ...
        if (req && vha->flags.qpairs_req_created) {
                ...
                qla25xx_free_req_que(vha, req);
        }
        return ret;
}

The same bypass happens in qla25xx_delete_rsp_que(). This seems to leak the
DMA rings, struct req/rsp, and the map indexes forever.

[ ... ]

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