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

Pre-existing issues:
- [Critical] Severe heap and stack buffer overflows in adjacent I2C BSG handlers due to unvalidated length parameters.
- [High] Systemic uninitialized kernel stack memory disclosure due to lack of request payload length validation.
--

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

scsi: qla2xxx: Bound image count in qla2x00_update_fru_versions()

This commit introduces a bounds check in qla2x00_update_fru_versions() to
ensure the user-supplied count does not exceed the size of the local stack
buffer. It rejects requests where the declared count would cause an
out-of-bounds read.

> 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: High]
This isn't a bug introduced by this patch, but does this bounds check
fully protect against reading uninitialized memory?

The check limits the loop count against the fixed sizeof(bsg) (256 bytes),
but it doesn't appear to check bsg_job->request_payload.payload_len.

If a user issues a BSG request with a short payload, sg_copy_to_buffer()
will only populate a portion of the 256-byte array, leaving the remainder
uninitialized:

qla2x00_update_fru_versions() {
    ...
    sg_copy_to_buffer(bsg_job->request_payload.sg_list,
        bsg_job->request_payload.sg_cnt, list, sizeof(bsg));
    ...
    while (count--) {
        memcpy(sfp, &image->field_info, sizeof(image->field_info));
        rval = qla2x00_write_sfp(...);
}

Could a short payload cause the loop to read uninitialized kernel stack
memory and send it to the device?

Similarly, in adjacent functions like qla2x00_read_fru_status() and
qla2x00_read_i2c(), if the initial request payload is short, could
sg_copy_from_buffer() copy uninitialized stack memory back to user space?

[Severity: Critical]
This is a pre-existing issue, but do adjacent I2C BSG handlers lack length
validation?

In qla2x00_write_i2c(), i2c->length comes from user input and appears to be
unconstrained. Could this cause a massive out-of-bounds read from the
stack into the sfp DMA pool allocation?

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

Likewise, in qla2x00_read_i2c(), could an unvalidated i2c->length allow the
device to DMA up to 65535 bytes into the 256-byte sfp heap buffer, followed
by a stack buffer overflow?

qla2x00_read_i2c() {
    ...
    rval = qla2x00_read_sfp(vha, sfp_dma, sfp,
        i2c->device, i2c->offset, i2c->length, i2c->option);
    ...
    memcpy(i2c->buffer, sfp, i2c->length);
    ...
}

> +		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));
>  		rval = qla2x00_write_sfp(vha, sfp_dma, sfp,

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