Re: [PATCH v2 58/60] 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:
- [High] Functions using a stack-allocated `bsg[DMA_POOL_SIZE]` buffer leak uninitialized kernel stack memory to user space and the device hardware.
- [High] The CPU zero-initializes a DMA mapped buffer with `memset()` after ownership has been transferred to the device via `dma_map_single()`, which can silently overwrite device DMA data on non-coherent architectures.
- [High] The mapped DMA buffer `dd->buf` is embedded inside a `__packed` structure, forcing an unaligned address mapping that violates cacheline isolation and risks DMA hardware errors.
--

commit db26a0928e61a1d01c807ef436263bd6ea60d8b1
Author: Nilesh Javali <njavali@marvell.com>

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

The qla2x00_do_dport_diagnostics function previously allocated the response
buffer without zeroing it. If the user request payload was short, the unused
fields would leak uninitialized kernel heap contents to user space. This patch
fixes the info leak by switching to kzalloc_obj.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 7f4558beee2c3..b57c55964f9e2 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -2744,7 +2744,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 isn't a bug introduced by this patch, but does allocating this __packed
structure lead to unaligned DMA mappings?

Looking at struct qla_dport_diag in drivers/scsi/qla2xxx/qla_bsg.h, the
__packed attribute forces the buf array to an unaligned 2-byte offset:

struct qla_dport_diag {
    uint16_t options;
    uint32_t buf[16];
    uint8_t  unused[62];
} __packed;

When dd->buf is later passed to dma_map_single() inside
qla26xx_dport_diagnostics(), wouldn't this violate the DMA API alignment
constraints and risk cacheline sharing or hardware DMA failures?

>  	if (!dd) {
>  		ql_log(ql_log_warn, vha, 0x70db,
>  		    "Failed to allocate memory for dport.\n");
>  		return -ENOMEM;
>  	}
>  
>  	sg_copy_to_buffer(bsg_job->request_payload.sg_list,
>  	    bsg_job->request_payload.sg_cnt, dd, sizeof(*dd));
>  
>  	rval = qla26xx_dport_diagnostics(
>  	    vha, dd->buf, sizeof(dd->buf), dd->options);

[Severity: High]
This is a pre-existing issue, but does the hardware diagnostics function
clobber its own DMA buffers?

Inside qla26xx_dport_diagnostics() in drivers/scsi/qla2xxx/qla_mbx.c, the code
does this:

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

Since ownership has already been transferred to the device via dma_map_single(),
could this memset() overwrite the hardware's DMA data on non-coherent
architectures when the CPU cache lines are evicted or written back?

[Severity: High]
This is a pre-existing issue, but do other functions in this file suffer from
a similar stack-based info leak?

Functions like qla2x00_read_fru_status() allocate an uninitialized stack
buffer:

qla2x00_read_fru_status(...) {
    uint8_t bsg[DMA_POOL_SIZE];
    struct qla_status_reg *sr = (void *)bsg;
    ...
    sg_copy_to_buffer(..., sr, sizeof(*sr));
    ...
    sg_copy_from_buffer(..., sr, sizeof(*sr));
}

If the user provides a payload smaller than the structure size,
sg_copy_to_buffer() leaves the remainder of the buffer uninitialized.

When sg_copy_from_buffer() copies the entire structure size back to the
user's reply payload, couldn't this leak uninitialized kernel stack memory
to user space, much like the heap leak fixed in this patch?

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