Re: [PATCH v3 84/88] scsi: qla2xxx: Zero-init bsg stack buffers to avoid info leak

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `qla24xx_iidma` BSG handler fails to validate `bsg_job->request_len`, leading to heap out-of-bounds read and write vulnerabilities when processing a short request.
- [High] Missing bounds check on user-supplied `i2c->length` in `qla2x00_read_i2c` allows kernel stack buffer overflow and DMA memory corruption.
--

--- Patch [84]: [PATCH v3 84/88] scsi: qla2xxx: Zero-init bsg stack buffers to avoid info leak ---
commit e991f43fbeb0a4f289a4242fcd01f28211667aba
Author: Nilesh Javali <njavali@marvell.com>
scsi: qla2xxx: Zero-init bsg stack buffers to avoid info leak

This patch zero-initializes the stack buffers in several qla2xxx bsg handlers
to prevent leaking stale kernel stack memory to user space on short requests.
It explicitly sets the bsg arrays to zero at declaration across the handlers.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 55142df30bda7..cb7227298b24b 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
[ ... ]
> @@ -2163,7 +2163,7 @@ qla2x00_read_i2c(struct bsg_job *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];
> +	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);

[Severity: High]
This is a pre-existing issue, but does this function lack a bounds check on
the user-supplied i2c->length?

In qla2x00_write_i2c(), there is a check to ensure i2c->length is not larger
than sizeof(i2c->buffer). However, in qla2x00_read_i2c(), the unvalidated
i2c->length is passed directly to qla2x00_read_sfp():

    rval = qla2x00_read_sfp(vha, sfp_dma, sfp,
        i2c->device, i2c->offset, i2c->length, i2c->option);

Could this instruct the firmware to perform a DMA transfer larger than the
allocated sfp buffer?

Following that, the unbounded length is used in memcpy():

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

Since i2c points to the local bsg stack array, could a length greater than
248 bytes cause a kernel stack buffer overflow?


While reviewing the BSG handlers in this file, I also noticed another
pre-existing issue in qla24xx_iidma().

[Severity: High]
This is a pre-existing issue, but does qla24xx_iidma() fail to validate
bsg_job->request_len?

In qla24xx_iidma(), port_param is derived by appending the size of the
request header to the user-supplied bsg_request:

    port_param = (void *)bsg_request + sizeof(struct fc_bsg_request);

If a user provides a short request where bsg_job->request_len is not
sufficiently large, port_param points past the end of the buffer.

The function then passes &port_param->speed to qla2x00_get_idma_speed():

    rval = qla2x00_get_idma_speed(vha, fcport->loop_id,
            &port_param->speed, mb);

Could this result in an out-of-bounds heap read and write when the speed
is read from or written to the out-of-bounds location?

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