Re: [PATCH v5 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 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing bounds checks on user-provided lengths cause severe stack and heap buffer overflows, as well as out-of-bounds DMA transfers.
- [High] DMA API violation: modifying a buffer via memset() immediately after mapping it for DMA.
- [High] Uninitialized stack buffer leaks kernel stack memory to user space via BSG replies.
- [High] Unlocked read-modify-write of `vha->dport_status` across process and hardirq contexts leads to a data race.
- [Medium] A struct field at an unaligned offset is mapped for DMA, resulting in unsafe cache-line sharing with adjacent struct fields.
--

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

This commit changes kmalloc_obj to kzalloc_obj in
qla2x00_do_dport_diagnostics. This ensures the qla_dport_diag response
buffer is fully zeroed, preventing uninitialized fields from being
leaked to user space.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 9ee56ccd52b01..3bf3a7b96eb50 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -2858,7 +2858,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: Medium]
This isn't a bug introduced by this patch, but does mapping the buf field
of this struct for DMA cause unsafe cache-line sharing?

Looking at struct qla_dport_diag in qla_bsg.h, the __packed attribute
places the buf array at an unaligned 2-byte offset:

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

This same unaligned buffer is then mapped for DMA:

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

Because the struct is packed, the mapped buf array shares a cache line
with the preceding options field, which violates DMA mapping hardware
cache alignment rules. Can this lead to data corruption on architectures
with non-coherent DMA caches?

>  	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 is it safe to modify a buffer with
memset while it is actively mapped for DMA?

In the underlying diagnostic functions called after this allocation, the
CPU modifies the buffer via memset after mapping it:

qla_mbx.c:qla26xx_dport_diagnostics() {
    dd_dma = dma_map_single(&vha->hw->pdev->dev,
        dd_buf, size, DMA_FROM_DEVICE);
    if (dma_mapping_error(&vha->hw->pdev->dev, dd_dma)) {
        ql_log(ql_log_warn, vha, 0x1194, "Failed to map dma buffer.\n");
        return QLA_MEMORY_ALLOC_FAILED;
    }

    memset(dd_buf, 0, size);
}

A similar pattern exists in qla26xx_dport_diagnostics_v2(). Since the
DMA API strictly prohibits the CPU from accessing or modifying a buffer
while it is mapped for device access, could the CPU cache lines dirtied
by memset be evicted during the DMA operation and overwrite the
diagnostic test data written by the device?

[Severity: Critical]
This is a pre-existing issue, but does this code lack bounds checks on
user-provided lengths, leading to buffer overflows?

In several BSG handler paths, user-controlled lengths dictate memory
operations without bounds checking:

qla_bsg.c:qla2x00_read_i2c() {
    rval = qla2x00_read_sfp(vha, sfp_dma, sfp,
        i2c->device, i2c->offset, i2c->length, i2c->option);
    
    if (rval) {
        ...
    }

    memcpy(i2c->buffer, sfp, i2c->length);
}

Here, the user-controlled i2c->length is passed directly to the firmware
for DMA, potentially overflowing the 256-byte sfp allocation. It is then
used in a memcpy to overflow the 256-byte bsg array on the stack.

Similarly, in qla2x00_write_i2c():

qla_bsg.c:qla2x00_write_i2c() {
    memcpy(sfp, i2c->buffer, i2c->length);
    rval = qla2x00_write_sfp(vha, sfp_dma, sfp,
        i2c->device, i2c->offset, i2c->length, i2c->option);
}

This causes an out-of-bounds memcpy from the stack into the 256-byte
sfp heap chunk.

Finally, in qla2x00_update_fru_versions():

qla_bsg.c:qla2x00_update_fru_versions() {
    count = list->count;
    while (count--) {
        memcpy(sfp, &image->field_info, sizeof(image->field_info));
        ...
        image++;
    }
}

An unbounded count dictates unbounded traversal of the image pointer
beyond the stack buffer. Could these lead to out-of-bounds DMA transfers
and severe memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but does this code leak
uninitialized stack memory to user space?

There are uninitialized stack buffers populated by BSG requests that
are fully copied back to user space:

qla_bsg.c:qla2x00_read_fru_status() {
    uint8_t bsg[DMA_POOL_SIZE];
    struct qla_status_reg *sr = (void *)bsg;
    ...
    sg_copy_to_buffer(bsg_job->request_payload.sg_list,
        bsg_job->request_payload.sg_cnt, sr, sizeof(*sr));
    ...
    sg_copy_from_buffer(bsg_job->reply_payload.sg_list,
        bsg_job->reply_payload.sg_cnt, sr, sizeof(*sr));
}

A similar pattern exists in qla2x00_read_i2c(). If a user issues a BSG
request with a small or zero payload size, the rest of the array remains
uninitialized. Will the entire structure size then be copied back via
sg_copy_from_buffer(), leaking kernel stack frames to user space?

[Severity: High]
This is a pre-existing issue, but does this code have an unlocked data
race on vha->dport_status?

In process context:

qla_bsg.c:qla2x00_do_dport_diagnostics_v2() {
    if (options == QLA_START_DPORT_TEST_V2 ||
        options == QLA_RESTART_DPORT_TEST_V2) {
        dd->mbx1 = mcp->mb[0];
        dd->mbx2 = mcp->mb[1];
        vha->dport_status |=  DPORT_DIAG_IN_PROGRESS;
    }
}

In hardirq context:

qla_isr.c:qla2x00_async_event() {
    ...
    case MBA_DPORT_DIAGNOSTICS:
        vha->dport_status &= ~DPORT_DIAG_IN_PROGRESS;
}

Both contexts modify the status field asynchronously without locks.
If the async completion interrupt fires before or during the process
context's bitwise OR operation, could the clear operation performed
by the interrupt be overwritten, permanently sticking the flag in
DPORT_DIAG_IN_PROGRESS?

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