Re: [PATCH v3 55/88] 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] Missing bounds check on user-supplied `i2c->length` in `qla2x00_read_i2c` and `qla2x00_write_i2c` leads to severe kernel stack and DMA pool heap buffer overflows.
- [High] Kernel stack memory is leaked to userspace and hardware due to uninitialized `bsg` stack buffers and missing payload length validation.
- [Medium] Preexisting leak of uninitialized kernel memory to user space via `bsg_reply->reply_payload_rcv_len`.
--

--- Patch [55]: [PATCH v3 55/88] scsi: qla2xxx: Bound image count in qla2x00_update_fru_versions() ---
commit 649b4e1d5ee505bfadef89861d13191eb9aaf100
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
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -1977,6 +1977,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;
> +	}
> +
>  	while (count--) {
>  		memcpy(sfp, &image->field_info, sizeof(image->field_info));
>  		rval = qla2x00_write_sfp(vha, sfp_dma, sfp,

[ ... ]

> @@ -2005,6 +2012,8 @@ qla2x00_update_fru_versions(struct bsg_job *bsg_job)
>  
>  done:
>  	bsg_job->reply_len = sizeof(struct fc_bsg_reply);
>  	bsg_reply->result = DID_OK << 16;
>  	bsg_job_done(bsg_job, bsg_reply->result,
>  		       bsg_reply->reply_payload_rcv_len);

[Severity: Medium]
This is a pre-existing issue, but is bsg_reply->reply_payload_rcv_len properly
initialized here? 

Both error paths (including the newly added length check) and the success path
jump to the done label, but bsg_reply->reply_payload_rcv_len is passed to
bsg_job_done() without ever being assigned a value.

Does this leak 4 bytes of uninitialized kernel memory to user space via the BSG
reply?

[ ... ]

> @@ -2012,6 +2021,21 @@ qla2x00_read_fru_status(struct bsg_job *bsg_job)
>  {
>  	struct fc_bsg_reply *bsg_reply = bsg_job->reply;
>  	struct Scsi_Host *host = fc_bsg_to_shost(bsg_job);
>  	scsi_qla_host_t *vha = shost_priv(host);
>  	struct qla_hw_data *ha = vha->hw;
>  	int rval = 0;
>  	uint8_t bsg[DMA_POOL_SIZE];
>  	struct qla_status_reg *sr = (void *)bsg;

[Severity: High]
This isn't a bug introduced by this patch, but does this uninitialized bsg array
cause a memory leak?

If a user issues a BSG command (e.g., QL_VND_READ_FRU_STATUS) with a truncated
payload length, sg_copy_to_buffer partially populates the uninitialized 256-byte
bsg stack array. 

The uninitialized data is then used in device commands and copied back to the
user reply via sg_copy_from_buffer. Can this leak up to 256 bytes of
uninitialized kernel stack memory to userspace and hardware?

[ ... ]

> @@ -2115,18 +2124,31 @@ qla2x00_write_i2c(struct bsg_job *bsg_job)
>  {
>  	struct fc_bsg_reply *bsg_reply = bsg_job->reply;
>  	struct Scsi_Host *host = fc_bsg_to_shost(bsg_job);
>  	scsi_qla_host_t *vha = shost_priv(host);
>  	struct qla_hw_data *ha = vha->hw;
>  	int rval = 0;
>  	uint8_t bsg[DMA_POOL_SIZE];
>  	struct qla_i2c_access *i2c = (void *)bsg;
>  	dma_addr_t sfp_dma;
>  	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
>  
>  	if (!sfp) {
>  		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
>  		    EXT_STATUS_NO_MEMORY;
>  		goto done;
>  	}
>  
>  	sg_copy_to_buffer(bsg_job->request_payload.sg_list,
>  	    bsg_job->request_payload.sg_cnt, i2c, sizeof(*i2c));
>  
>  	memcpy(sfp, i2c->buffer, i2c->length);

[Severity: High]
This is a pre-existing issue, but does this lack bounds checking on i2c->length?

A user can invoke the BSG interface with QL_VND_WRITE_I2C and provide an
i2c->length larger than the 256-byte DMA pool allocation (sfp).

Will this cause an unbounded read from the stack and an unbounded write into the
256-byte DMA pool buffer?

[ ... ]

> @@ -2157,20 +2179,35 @@ qla2x00_read_i2c(struct bsg_job *bsg_job)
>  {
>  	struct fc_bsg_reply *bsg_reply = bsg_job->reply;
>  	struct Scsi_Host *host = fc_bsg_to_shost(bsg_job);
>  	scsi_qla_host_t *vha = shost_priv(host);
>  	struct qla_hw_data *ha = vha->hw;
>  	int rval = 0;
>  	uint8_t bsg[DMA_POOL_SIZE];
>  	struct qla_i2c_access *i2c = (void *)bsg;

[Severity: High]
This is a pre-existing issue, but does this uninitialized bsg array
cause a memory leak?

Similar to qla2x00_read_fru_status, if a user issues a BSG command (e.g.,
QL_VND_READ_I2C) with a truncated payload length, sg_copy_to_buffer partially
populates the uninitialized 256-byte bsg stack array. 

Can this leak up to 256 bytes of uninitialized kernel stack memory to userspace
and hardware?

>  	dma_addr_t sfp_dma;
>  	uint8_t *sfp = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &sfp_dma);
>  
>  	if (!sfp) {
>  		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
>  		    EXT_STATUS_NO_MEMORY;
>  		goto done;
>  	}
>  
>  	sg_copy_to_buffer(bsg_job->request_payload.sg_list,
>  	    bsg_job->request_payload.sg_cnt, i2c, sizeof(*i2c));
>  
>  	rval = qla2x00_read_sfp(vha, sfp_dma, sfp,
>  		i2c->device, i2c->offset, i2c->length, i2c->option);
>  
>  	if (rval) {
>  		bsg_reply->reply_data.vendor_reply.vendor_rsp[0] =
>  		    EXT_STATUS_MAILBOX;
>  		goto dealloc;
>  	}
>  
>  	memcpy(i2c->buffer, sfp, i2c->length);

[Severity: High]
This is a pre-existing issue, but is there a missing bounds check on
i2c->length here as well?

If a user invokes the BSG interface with QL_VND_READ_I2C and provides an
i2c->length larger than the 64-byte stack buffer (i2c->buffer) inside the
256-byte bsg array, can this cause an unbounded copy into the fixed 64-byte
stack buffer?

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