Re: [PATCH v4 04/56] scsi: qla2xxx: Add 29xx support in queue initialisation path

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Base queue DMA rings (Queue 0) are undersized for 29xx hardware, which will lead to firmware reading and writing out-of-bounds memory.
- [Critical] Response queue lockless synchronization signatures are improperly initialized for 29xx, leaving half the ring buffer zero-initialized and vulnerable to phantom processing.
- [High] Mismatched queue pointer arithmetic for QLA29XX introduces hardware synchronization failures, queue stalls, and breaks bisectability.

Pre-existing issues:
- [High] The queue cleanup paths blindly pass a potentially NULL pointer to `dma_free_coherent()`, triggering a kernel crash if the initial queue allocation fails.
- [High] Inconsistent locking between `vport_lock` and `mq_lock` when managing dynamic queue IDs and queue maps.
- [High] Uninitialized kernel stack memory is written to hardware mailbox registers.
--

--- Patch [4]: [PATCH v4 04/56] scsi: qla2xxx: Add 29xx support in queue initialisation path ---
commit 13a40ca7120445e92ae33446755917919989aa14
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add 29xx support in queue initialisation path

This patch extends queue initialization and mailbox commands to support the
QLA29XX hardware. It introduces 128-byte request and response structures and
doubles the DMA ring memory allocated for multi-queues.

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 2d052f870b2bc..2b232b225c900 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4584,7 +4584,8 @@ qla25xx_init_req_que(struct scsi_qla_host *vha, struct req_que *req)
>  	mcp->mb[12] = req->qos;
>  	mcp->mb[11] = req->vp_idx;
>  	mcp->mb[13] = req->rid;
> -	if (IS_QLA83XX(ha) || IS_QLA27XX(ha) || IS_QLA28XX(ha))
> +	if (IS_QLA83XX(ha) || IS_QLA27XX(ha) || IS_QLA28XX(ha) ||
> +	    IS_QLA29XX(ha))
>  		mcp->mb[15] = 0;

[Severity: High]
This is a pre-existing issue, but does this leak uninitialized kernel stack
memory to the hardware mailbox?

The mbx_cmd_t structure is allocated on the stack but not zero-initialized:

mbx_cmd_t mc;
mbx_cmd_t *mcp = &mc;

Several mailbox fields (like mcp->mb[14]) are left unwritten, yet their
corresponding validity bits (MBX_14) are set in the mcp->out_mb bitmask:

mcp->out_mb = MBX_14|MBX_13|MBX_12|MBX_11|MBX_10|...

When qla2x00_mailbox_command() iterates over the out_mb mask, does it read
these uninitialized stack values and issue them directly to the hardware via
wrt_reg_word()?

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index c563133f751e8..e75b7ae22bc52 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -574,9 +574,13 @@ qla25xx_free_req_que(struct scsi_qla_host *vha, struct req_que *req)
>  {
>  	struct qla_hw_data *ha = vha->hw;
>  	uint16_t que_id = req->id;
> +	uint16_t reqsz;
> +
> +	reqsz = IS_QLA29XX(ha) ? sizeof(struct request_ext) :
> +				 sizeof(request_t);
>  
>  	dma_free_coherent(&ha->pdev->dev, (req->length + 1) *
> -		sizeof(request_t), req->ring, req->dma);
> +			  reqsz, req->ring, req->dma);

[Severity: High]
This is a pre-existing issue, but can this trigger a kernel panic if the
initial allocation fails?

If memory pressure causes dma_alloc_coherent() to return NULL during queue
creation, the error path executes this cleanup and passes the NULL req->ring
pointer directly into dma_free_coherent().

[Severity: High]
This is a pre-existing issue, but is there a data race in the queue ID
cleanup later in this function?

Dynamic queue ID allocation in qla25xx_create_req_que() protects the
ha->req_qid_map read-modify-write cycle using mutex_lock(&ha->mq_lock).
However, the cleanup path clears the map globally without holding mq_lock
and clears the bitmap using vport_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);

Could this corrupt the queue tracking structures when racing against queue
creation?

>  	req->ring = NULL;
>  	req->dma = 0;
>  	if (que_id) {

[ ... ]

> @@ -594,6 +598,10 @@ qla25xx_free_rsp_que(struct scsi_qla_host *vha, struct rsp_que *rsp)
>  {
>  	struct qla_hw_data *ha = vha->hw;
>  	uint16_t que_id = rsp->id;
> +	uint16_t rspsz;
> +
> +	rspsz = IS_QLA29XX(ha) ? sizeof(struct response_ext) :
> +				 sizeof(response_t);
>  
>  	if (rsp->msix && rsp->msix->have_irq) {
>  		free_irq(rsp->msix->vector, rsp->msix->handle);
> @@ -601,8 +609,9 @@ qla25xx_free_rsp_que(struct scsi_qla_host *vha, struct rsp_que *rsp)
>  		rsp->msix->in_use = 0;
>  		rsp->msix->handle = NULL;
>  	}
> +
>  	dma_free_coherent(&ha->pdev->dev, (rsp->length + 1) *
> -		sizeof(response_t), rsp->ring, rsp->dma);
> +			  rspsz, rsp->ring, rsp->dma);

[Severity: High]
This is a pre-existing issue, but passing a potentially NULL rsp->ring pointer
into dma_free_coherent() can also trigger a wild pointer dereference here if
queue creation fails.

>  	rsp->ring = NULL;
>  	rsp->dma = 0;
>  	if (que_id) {

[ ... ]

> @@ -706,6 +715,7 @@ qla25xx_create_req_que(struct qla_hw_data *ha, uint16_t options,
>  	uint16_t que_id = 0;
>  	device_reg_t *reg;
>  	uint32_t cnt;
> +	uint16_t reqsz;
>  
>  	req = kzalloc_obj(struct req_que);
>  	if (req == NULL) {
> @@ -714,9 +724,12 @@ qla25xx_create_req_que(struct qla_hw_data *ha, uint16_t options,
>  		goto failed;
>  	}
>  
> +	reqsz = IS_QLA29XX(ha) ? sizeof(struct request_ext) :
> +				 sizeof(request_t);
> +
>  	req->length = REQUEST_ENTRY_CNT_24XX;
>  	req->ring = dma_alloc_coherent(&ha->pdev->dev,
> -			(req->length + 1) * sizeof(request_t),
> +			(req->length + 1) * reqsz,
>  			&req->dma, GFP_KERNEL);

[Severity: Critical]
Does this leave the base DMA rings (Queue 0) undersized for 29xx hardware?

While this patch updates MQ allocations here to use 128-byte strides (reqsz),
the base queue allocations in qla_os.c:qla2x00_mem_alloc() appear to still be
hardcoded to sizeof(request_t) (64 bytes).

Since 29xx hardware expects 128-byte entries across all rings, won't the
firmware perform out-of-bounds DMA read/writes into a buffer sized for
64-byte entries, potentially corrupting adjacent physical memory?

[Severity: High]
Does deferring shadow pointer calculation updates to later patches break
bisectability and cause hardware synchronization failures?

The commit message notes pointer arithmetic is handled by later patches.
However, since req->ring is a request_t * (64 bytes), calculations later in
this initialization sequence like:

req->out_ptr = (uint16_t *)(req->ring + req->length);

will use 64-byte pointer arithmetic. For 29xx, which expects a 128-byte
stride, this pointer lands in the middle of the active ring buffer instead
of the trailing slot. Is it safe to allocate 128-byte strides here without
simultaneously updating the shadow pointer calculations?

>  	if (req->ring == NULL) {
>  		ql_log(ql_log_fatal, base_vha, 0x00da,

[ ... ]

> @@ -833,6 +846,7 @@ qla25xx_create_rsp_que(struct qla_hw_data *ha, uint16_t options,
>  	struct scsi_qla_host *vha = pci_get_drvdata(ha->pdev);
>  	uint16_t que_id = 0;
>  	device_reg_t *reg;
> +	uint16_t rspsz;
>  
>  	rsp = kzalloc_obj(struct rsp_que);
>  	if (rsp == NULL) {
> @@ -841,9 +855,12 @@ qla25xx_create_rsp_que(struct qla_hw_data *ha, uint16_t options,
>  		goto failed;
>  	}
>  
> +	rspsz = IS_QLA29XX(ha) ? sizeof(struct response_ext) :
> +				 sizeof(response_t);
> +
>  	rsp->length = RESPONSE_ENTRY_CNT_MQ;
>  	rsp->ring = dma_alloc_coherent(&ha->pdev->dev,
> -			(rsp->length + 1) * sizeof(response_t),
> +			(rsp->length + 1) * rspsz,
>  			&rsp->dma, GFP_KERNEL);

[Severity: Critical]
Does this cause phantom processing of zero-initialized memory due to
mismatched buffer strides?

This patch provisions 128-byte strides (rspsz) for QLA29XX, but
qla2x00_init_response_q_entries() populates the RESPONSE_PROCESSED signature
by stepping linearly using pkt++ on a 64-byte response_t * pointer:

for (cnt = 0; cnt < rsp->length; cnt++) {
    pkt->signature = RESPONSE_PROCESSED;
    pkt++;
}

Won't this only write the signatures into the first half of the 128-byte
structures (hitting the reserved space) and leave the second half zeroed?

Since 0 != RESPONSE_PROCESSED, will the driver's interrupt handler falsely
interpret these zeroes as valid incoming firmware responses?

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