Re: [PATCH v3 53/88] scsi: qla2xxx: Zero dport diagnostics buffer to avoid info leak

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Uninitialized kernel stack memory leak and severe stack buffer overflow in multiple BSG handlers.
- [High] Calling `memset()` on a buffer after `dma_map_single()` and using unaligned structs causes DMA cacheline sharing and data corruption.
- [High] Out-of-bounds heap read due to missing `bsg_job->request_len` validation.
--

--- Patch [53]: [PATCH v3 53/88] scsi: qla2xxx: Zero dport diagnostics buffer to avoid info leak ---
commit 4b26f2e53fe6f8ddd499850e1bc9b9280efa8bd9
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Zero dport diagnostics buffer to avoid info leak

This commit fixes an information leak in qla2x00_do_dport_diagnostics().
It changes the allocation of the qla_dport_diag response buffer from
kmalloc_obj() to kzalloc_obj() to ensure the buffer is fully zeroed
before it is copied to user space.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index fade3638d31cb..e7739cead967a 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -2792,7 +2792,7 @@ qla2x00_do_dport_diagnostics(struct bsg_job *bsg_job)
>  	    !IS_QLA28XX(vha->hw) && !IS_QLA29XX(vha->hw))
>  		return -EPERM;
>  
> -	dd = kmalloc_obj(*dd);
> +	dd = kzalloc_obj(*dd);
>  	if (!dd) {
>  		ql_log(ql_log_warn, vha, 0x70db,
>  		    "Failed to allocate memory for dport.\n");

[Severity: High]
This is a pre-existing issue, but does calling memset() on a buffer after
dma_map_single() corrupt the DMA data?

In drivers/scsi/qla2xxx/qla_mbx.c:qla26xx_dport_diagnostics(), the code does:

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

The dma_map_single() call invalidates the CPU cache for the buffer on
non-coherent architectures. The immediate memset() dirties the CPU cache lines,
which could overwrite the diagnostic data written by the device via DMA if
they are evicted to main memory.

Additionally, the mapped buffer dd_buf starts at an offset that may not be
aligned to ARCH_DMA_MINALIGN. Mapping unaligned buffers can corrupt adjacent
struct fields sharing the same cache line. The same DMA cacheline sharing and
memset corruption pattern occurs in qla26xx_dport_diagnostics_v2().


[Severity: Critical]
This is a pre-existing issue, but is there a risk of an uninitialized kernel
stack memory leak and stack buffer overflow in multiple BSG handlers?

In drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_read_i2c():

	uint8_t bsg[DMA_POOL_SIZE];
	struct qla_i2c_access *i2c = (void *)bsg;
...
	sg_copy_to_buffer(bsg_job->request_payload.sg_list,
	    bsg_job->request_payload.sg_cnt, i2c, sizeof(*i2c));

	rval = qla2x00_read_sfp(vha, sfp_dma, sfp,
		i2c->device, i2c->offset, i2c->length, i2c->option);
...
	memcpy(i2c->buffer, sfp, i2c->length);
	sg_copy_from_buffer(bsg_job->reply_payload.sg_list,
	    bsg_job->reply_payload.sg_cnt, i2c, sizeof(*i2c));

If the user provides a short payload, i2c->length remains uninitialized. It is
then used as the size for memcpy() without bounds checking, which could cause a
stack buffer overflow (writing past the bsg array) and a heap out-of-bounds
read from the sfp DMA pool buffer.

Passing this unbounded length to qla2x00_read_sfp() can also cause a DMA
buffer overflow into the kernel heap. Finally, the bsg buffer is copied back
to user space, potentially leaking uninitialized kernel stack memory. A similar
leak pattern exists in qla2x00_read_fru_status().


[Severity: High]
This is a pre-existing issue, but does this code read out-of-bounds heap memory
if the user provides a short request?

In drivers/scsi/qla2xxx/qla_bsg.c:qla84xx_mgmt_cmd(), the handler expects a
vendor-specific structure appended to the generic request:

	ql84_mgmt = (void *)bsg_request + sizeof(struct fc_bsg_request);
	switch (ql84_mgmt->mgmt.cmd) {

While the block layer validates that bsg_job->request_len covers the generic
struct fc_bsg_request, there does not appear to be validation that it covers
the appended qla_bsg_a84_mgmt structure.

If a user issues a BSG command with a request_len of exactly
sizeof(struct fc_bsg_request), this function will read out of bounds from the
allocated heap buffer, which can cause a kernel panic if the read crosses a
page boundary. A similar out-of-bounds heap read exists in qla24xx_iidma() due
to a lack of request_len validation for the appended qla_port_param structure.

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