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

Pre-existing issues:
- [High] Heap and stack buffer overflows due to unvalidated length in I2C BSG handlers.
- [Medium] Missing payload length check against user input in FRU update/read/write functions.
- [Medium] Uninitialized heap memory leak to device in SFP write operations.
--

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

scsi: qla2xxx: Bound image count in qla2x00_update_fru_versions()

This commit rejects BSG requests in qla2x00_update_fru_versions() where the 
declared count does not fit in the 256-byte stack buffer. This prevents a 
stack out-of-bounds read that could be sent to the device.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 76c57e574c220..f9b693af8db10 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -1991,6 +1991,13 @@ qla2x00_update_fru_versions(struct bsg_job *bsg_job)
>  
>  	image = list->version;
>  	count = list->count;
> +
> +	if (struct_size(list, version, count) > sizeof(bsg)) {
> +		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
> +		    EXT_STATUS_INVALID_PARAM;
> +		goto dealloc;
> +	}
> +

[Severity: Medium]
This is a pre-existing issue, but does this function also need to validate
the actual request payload length?

If a local user issues a request with a payload smaller than the required
struct sizes, sg_copy_to_buffer() only copies the provided bytes, leaving
the rest of the stack buffer bsg uninitialized:

drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_update_fru_versions() {
    ...
    sg_copy_to_buffer(bsg_job->request_payload.sg_list,
        bsg_job->request_payload.sg_cnt, list, sizeof(bsg));
    ...
}

The code then reads from this uninitialized memory (e.g. image->field_info)
and passes it to mailbox commands, leaking kernel stack memory to the
hardware device.

Can similar payload length validation issues occur in
qla2x00_read_fru_status() and qla2x00_write_fru_status() where
sg_copy_to_buffer() is also used?

>  	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 lead to an
uninitialized heap memory leak?

Earlier in qla2x00_update_fru_versions(), sfp is allocated via
dma_pool_alloc() from a 256-byte pool without zeroing it. Only 36 bytes of
user data are copied into sfp.

If the user sets image->field_address.option to include BIT_0 and sets the
first byte of field_info to 255, this length is overridden in
qla2x00_write_sfp():

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

Will the hardware then read 255 bytes from sfp, leaking 219 bytes of
uninitialized kernel heap memory to the device?

>  		rval = qla2x00_write_sfp(vha, sfp_dma, sfp,

[ ... ]

[Severity: High]
This is a pre-existing issue, but do the I2C BSG handlers have unbounded
length checks leading to overflows?

In qla2x00_write_i2c(), an unbounded user-controlled i2c->length is used in
a memcpy:

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

If i2c->length is set to a large value (up to 65535), can this overflow the
256-byte heap allocation sfp and read out-of-bounds from the kernel stack?

Similarly, in qla2x00_read_i2c(), does passing an unvalidated length to
memcpy() overflow the 256-byte stack array:

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

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