Re: [PATCH v4 55/56] scsi: qla2xxx: Bound image count in qla2x00_update_fru_versions()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on user-controlled `i2c->length` field causes stack and heap buffer overflows.
- [High] Kernel stack memory disclosure to userspace via short payloads.
- [Medium] Uninitialized stack memory sent to device due to unverified payload length.
- [Medium] Uninitialized DMA pool memory leaked to device via length override.
--

--- Patch [55]: [PATCH v4 55/56] scsi: qla2xxx: Bound image count in qla2x00_update_fru_versions() ---
commit 57c2acf420006763090ae734727bc12044491697
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Bound image count in qla2x00_update_fru_versions()

qla2x00_update_fru_versions() copies the user-supplied BSG request into
a fixed 256-byte stack buffer (bsg[DMA_POOL_SIZE]) and then iterates
list->count times over the qla_image_version array embedded in that
buffer, advancing the image pointer each iteration. count is taken
directly from user input with no upper bound, while only
(DMA_POOL_SIZE - sizeof(list->count)) / sizeof(struct qla_image_version)
= 6 entries actually fit. A larger count walks the image pointer off the
end of the stack buffer, reading adjacent kernel stack memory and
sending it to the device via qla2x00_write_sfp().

Reject requests whose declared count does not fit in the buffer.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 2f73c9af418d7..95cfdb020319d 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -2043,6 +2043,13 @@ qla2x00_update_fru_versions(struct bsg_job *bsg_job)
>  
>  	image = list->version;
>  	count = list->count;
> +
> +	if (struct_size(list, version, count) > sizeof(bsg)) {

[Severity: Medium]
This isn't a bug introduced by this patch, but does this bounds check verify
the actual user-provided payload length?

sg_copy_to_buffer() only copies the bytes provided by the user payload. While
this check verifies count against the bsg buffer capacity, if a short payload
is provided, the rest of the bsg array remains uninitialized.

> +		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
> +		    EXT_STATUS_INVALID_PARAM;
> +		goto dealloc;
> +	}
> +
>  	while (count--) {
>  		memcpy(sfp, &image->field_info, sizeof(image->field_info));

[Severity: Medium]
This isn't a bug introduced by this patch, but can this loop copy uninitialized
portions of the bsg stack array?

If a short request is provided, the bsg array contains uninitialized data
which is then copied into sfp and passed to the device.

[Severity: Medium]
This isn't a bug introduced by this patch, but does the dma_pool_alloc() call
earlier in qla2x00_update_fru_versions() leak uninitialized DMA pool memory?

The sfp buffer is allocated via dma_pool_alloc() without GFP_ZERO. Only 36
bytes of user data are copied into this 256-byte buffer.

In qla2x00_write_sfp(), if the user sets BIT_0 in image->field_address.option:

drivers/scsi/qla2xxx/qla_mbx.c:qla2x00_write_sfp()
    if (opt & BIT_0)
        len = *sfp;

This overrides the write length with the first byte of user data. A user could
set this to 255, causing the firmware to DMA-read up to 219 bytes of
uninitialized kernel DMA pool memory and sending it to the device.

[Severity: High]
This isn't a bug introduced by this patch, but could an unchecked i2c->length
field cause a buffer overflow in qla2x00_write_i2c() and qla2x00_read_i2c()?

In qla2x00_write_i2c():

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

The i2c->length field is extracted from the user payload without bounds
checking and executes unconditionally before calling the firmware. Since sfp
is a 256-byte allocation and bsg is a 256-byte stack array, a length > 256
could cause a kernel stack out-of-bounds read and a heap out-of-bounds write.

Similarly, qla2x00_read_i2c() might suffer a stack out-of-bounds write when
calling memcpy(i2c->buffer, sfp, i2c->length).

[Severity: High]
This isn't a bug introduced by this patch, but can short payloads cause
qla2x00_read_fru_status() and qla2x00_read_i2c() to disclose kernel stack
memory to userspace?

These functions allocate a 256-byte uninitialized stack array and use
sg_copy_to_buffer() to populate it from the user request payload. If the
payload is short, the rest of the struct is left uninitialized.

Upon success, sg_copy_from_buffer() copies the full sizeof(*sr) or
sizeof(*i2c) to the reply payload:

drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_read_fru_status()
    sg_copy_from_buffer(bsg_job->reply_payload.sg_list,
        bsg_job->reply_payload.sg_cnt, sr, sizeof(*sr));

Could this directly leak uninitialized kernel stack data to userspace?

>  		rval = qla2x00_write_sfp(vha, sfp_dma, sfp,

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