Re: [PATCH v4 39/56] scsi: qla2xxx: Adjust feature gating in BSG paths for 29xx support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing bounds check on user-supplied count leads to out-of-bounds stack read and information leak to hardware.
- [High] Uninitialized kernel heap memory is leaked to user space in qla2x00_do_dport_diagnostics.
- [High] Modifying a DMA buffer after mapping it for device use causes data corruption on non-coherent architectures.
--

--- Patch [39]: [PATCH v4 39/56] scsi: qla2xxx: Adjust feature gating in BSG paths for 29xx support ---
commit 3425e7689599b4f90cb066bb5b9b5a8c81315e6d
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Adjust feature gating in BSG paths for 29xx support

Extend qla2xxx BSG command handling to recognize QLA29xx adapters and
align feature availability with hardware capabilities. Allow QLA29xx in
paths previously restricted to QLA27xx/28xx and restrict unsupported
operations. Guard the active image layout logic with explicit checks.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 46eed5df7eefb..9ee56ccd52b01 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -2014,7 +2014,7 @@ static int qla2900_bsg_load_mpi(struct bsg_job *bsg_job)
>  	bsg_job_done(bsg_job, bsg_reply->result,
>  		     bsg_reply->reply_payload_rcv_len);
>  
> -	return rval;
> +	return 0;
>  }

[Severity: Critical]
This is a pre-existing issue, but could this lead to an out-of-bounds stack
read?

In the neighboring function qla2x00_update_fru_versions() in qla_bsg.c:

	uint8_t bsg[DMA_POOL_SIZE];
	struct qla_image_version_list *list = (void *)bsg;
	...
	image = list->version;
	count = list->count;

	while (count--) {
		memcpy(sfp, &image->field_info, sizeof(image->field_info));
		rval = qla2x00_write_sfp(vha, sfp_dma, sfp, ...);
		...
		image++;
	}

It reads the user-supplied list->count and uses it as a loop condition.
If the count is maliciously large, could this loop iterate past the end of the
bsg stack array, reading out-of-bounds stack memory and sending it to the
device via qla2x00_write_sfp()?

[ ... ]

> @@ -2855,7 +2855,7 @@ qla2x00_do_dport_diagnostics(struct bsg_job *bsg_job)
>  	struct qla_dport_diag *dd;
>  
>  	if (!IS_QLA83XX(vha->hw) && !IS_QLA27XX(vha->hw) &&
> -	    !IS_QLA28XX(vha->hw))
> +	    !IS_QLA28XX(vha->hw) && !IS_QLA29XX(vha->hw))
>  		return -EPERM;
>  
>  	dd = kmalloc_obj(*dd);

[Severity: High]
This isn't a bug introduced by this patch, but does this allocation leak
uninitialized kernel heap memory?

If a user triggers this BSG request with a payload smaller than
sizeof(struct qla_dport_diag), kmalloc_obj(*dd) leaves the buffer
uninitialized, and sg_copy_to_buffer() only partially overwrites it.

Then qla26xx_dport_diagnostics() memsets only dd->buf. The remaining 62 bytes
in dd->unused remain uninitialized before being copied back to userspace via
sg_copy_from_buffer():

	if (rval == QLA_SUCCESS) {
		sg_copy_from_buffer(bsg_job->reply_payload.sg_list,
		    bsg_job->reply_payload.sg_cnt, dd, sizeof(*dd));
	}

Could we use kzalloc_obj() instead?

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index cec308811d9eb..ce845afe3e16a 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -6566,7 +6566,7 @@ qla26xx_dport_diagnostics(scsi_qla_host_t *vha,
>  	dma_addr_t dd_dma;
>  
>  	if (!IS_QLA83XX(vha->hw) && !IS_QLA27XX(vha->hw) &&
> -	    !IS_QLA28XX(vha->hw))
> +	    !IS_QLA28XX(vha->hw) && !IS_QLA29XX(vha->hw))
>  		return QLA_FUNCTION_FAILED;
>  
>  	ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x119f,

[Severity: High]
This is a pre-existing issue, but does this modify a DMA buffer after mapping
it for device use?

Looking at qla26xx_dport_diagnostics():

	dd_dma = dma_map_single(&vha->hw->pdev->dev,
	    dd_buf, size, DMA_FROM_DEVICE);
	...
	memset(dd_buf, 0, size);

Modifying the buffer after mapping it with DMA_FROM_DEVICE violates the DMA
API, which can cause data corruption on non-coherent architectures when the
CPU writes dirty cache lines over the device's DMA writes.

Should the memset() occur before the dma_map_single() call?

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