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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Multiple BSG handlers use uninitialized stack buffers for request/reply structures, causing kernel memory leaks, arbitrary hardware writes, and severe buffer overflows when users provide a short payload.
- [High] Modifying a DMA buffer with memset() after it has been mapped for DMA_FROM_DEVICE violates DMA ownership rules and causes cache coherency data corruption.
- [High] DMA mapping a misaligned buffer inside a __packed struct risks cache line sharing corruption and hardware DMA faults.
- [Medium] Unvalidated reply_payload_rcv_len can trigger a kernel WARN_ON in the block layer.
--

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

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

This patch changes the allocation of the qla_dport_diag response buffer
from kmalloc_obj() to kzalloc_obj() in qla2x00_do_dport_diagnostics().
The change ensures the buffer is zero-initialized, preventing kernel heap
contents from leaking to user space when copying the structure back.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index f9f9687316b58..4a9cf8da67a6b 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -2806,7 +2806,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);

[Severity: High]
This is a pre-existing issue, but are we risking cache line sharing
corruption and hardware DMA faults by DMA mapping the misaligned buffer
inside this structure?

Looking at the structure definition:

drivers/scsi/qla2xxx/qla_bsg.h:struct qla_dport_diag {
	uint16_t options;
	uint32_t buf[16];
	uint8_t  unused[62];
} __packed;

Because the struct is __packed, the buf array is forced to an unaligned
2-byte offset.

When buf is subsequently mapped for DMA in qla26xx_dport_diagnostics():

drivers/scsi/qla2xxx/qla_mbx.c:qla26xx_dport_diagnostics() {
    ...
	dd_dma = dma_map_single(&vha->hw->pdev->dev, dd_buf, size,
				DMA_FROM_DEVICE);
    ...
}

Could unmapping or syncing this misaligned buffer on non-coherent
architectures invalidate the entire cache line, thereby corrupting the
adjacent options and unused fields? Furthermore, could providing a 2-byte
aligned address to the HBA cause hardware DMA faults?


[Severity: High]
This isn't a bug introduced by this patch, but does the memset() in the
diagnostics functions violate DMA ownership rules?

In qla26xx_dport_diagnostics():

drivers/scsi/qla2xxx/qla_mbx.c:qla26xx_dport_diagnostics() {
    ...
	dd_dma = dma_map_single(&vha->hw->pdev->dev, dd_buf, size,
				DMA_FROM_DEVICE);
    ...
	memset(dd_buf, 0, size);
    ...
}

A similar pattern exists in qla26xx_dport_diagnostics_v2().

The DMA API forbids the CPU from modifying a buffer while it is mapped for
device DMA. On non-cache-coherent architectures, does the CPU write bring
the cache line into the CPU cache and mark it as dirty, meaning that if it
is evicted to RAM after the device completes its DMA writes, the CPU's
zeroes might silently overwrite the device's diagnostic data?

Since the allocation in qla2x00_do_dport_diagnostics() is now using
kzalloc_obj(), is this memset() still necessary, or can it be removed?


[Severity: Medium]
This is a pre-existing issue, but does setting the reply length
unconditionally risk triggering a block layer WARN_ON if the user provides
a short payload?

Later in qla2x00_do_dport_diagnostics():

drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_do_dport_diagnostics() {
    ...
	bsg_reply->reply_payload_rcv_len = sizeof(*dd);
    ...
}

If a user submits a BSG job with a reply payload smaller than sizeof(*dd),
will reply_payload_rcv_len exceed the allocated response length, triggering
a WARN_ON(job->reply_payload_rcv_len > rsp_len) in the block layer?


[Severity: Critical]
This is a pre-existing issue, but do other BSG handlers have similar
uninitialized stack buffer vulnerabilities?

Several handlers allocate structures on the stack without initializing them,
and sg_copy_to_buffer() only populates the bytes provided by the user
payload. If a user provides a short payload, variables might remain
uninitialized stack garbage.

For example, in qla2x00_write_i2c():

drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_write_i2c() {
    ...
	memcpy(sfp, i2c->buffer, i2c->length);
    ...
}

Could an uninitialized i2c->length cause an out-of-bounds read from the
stack and an out-of-bounds write to the DMA pool allocation?

Similar issues appear to exist in:
- qla2x00_read_i2c() (memcpy overflowing the fixed-size stack array bsg)
- qla2x00_update_fru_versions() (uninitialized list->count dictating loop
  iterations)
- qla2x00_read_fru_status() (uninitialized stack buffer copied to user space)
- qla_edif_app_start() (uninitialized pad and reserved fields copied to
  user space)

Could a user issuing a BSG job with a truncated request payload trigger
these out-of-bounds accesses or memory leaks?

>  	if (!dd) {
>  		ql_log(ql_log_warn, vha, 0x70db,
>  		    "Failed to allocate memory for dport.\n");

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