Re: [PATCH v5 07/56] scsi: qla2xxx: Add flash block read/write BSG support for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Uninitialized usage of `bsg_reply->reply_payload_rcv_len` and partial initialization of `bsg_reply` leads to an information leak and potential kernel warning.
- [Medium] Incorrect BSG request length validation forces user space to over-allocate the payload buffer by 8 unused bytes and rejects exactly-sized valid requests.
- [Medium] Passing a 0-length payload to flash block read/write commands triggers a kernel WARN_ON_ONCE via `vzalloc(0)`.
--

--- Patch [7]: [PATCH v5 07/56] scsi: qla2xxx: Add flash block read/write BSG support for 29xx ---
commit a2bf87754d2c500ea1dbb98bcd9b4f70437eb23f
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add flash block read/write BSG support for 29xx

Introduce QL_VND_READ_FLASH_BLOCK and QL_VND_WRITE_FLASH_BLOCK
BSG vendor commands so that userspace tools can perform flash
block-level operations on 29xx adapters via the isp_ops interface.

> 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
[ ... ]
> @@ -1431,42 +1422,79 @@ qla2x00_optrom_setup(struct bsg_job *bsg_job, scsi_qla_host_t *vha,
[ ... ]
> +		if (is_update) {
> +			ha->optrom_region_size = start +
> +			    bsg_job->request_payload.payload_len >
> +			    ha->optrom_size ?
> +			    ha->optrom_size - start :
> +			    bsg_job->request_payload.payload_len;
> +			ha->optrom_state = QLA_SWRITING;
> +		} else {
> +			ha->optrom_region_size = start +
> +			    bsg_job->reply_payload.payload_len >
> +			    ha->optrom_size ?
> +			    ha->optrom_size - start :
> +			    bsg_job->reply_payload.payload_len;
> +			ha->optrom_state = QLA_SREADING;
> +		}
[ ... ]
>  	ha->optrom_buffer = vzalloc(ha->optrom_region_size);
>  	if (!ha->optrom_buffer) {

[Severity: Medium]
If a user space tool sends a flash block read or write command with a 
payload length of 0, ha->optrom_region_size will become 0.

Won't calling vzalloc(0) here trigger a WARN_ON_ONCE(!size) in the kernel 
memory allocator? 

Should there be an explicit check to reject 0-length payloads early to 
prevent this warning and potential system instability if panic_on_warn 
is enabled?

[ ... ]
> @@ -1554,6 +1592,234 @@ qla2x00_update_optrom(struct bsg_job *bsg_job)
[ ... ]
> +static int qla29xx_bsg_flash_block_read(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	if (bsg_job->request_len < sizeof(struct fc_bsg_request) +
> +	    2 * sizeof(uint32_t) + sizeof(struct qla_block_rw))
> +		return -EINVAL;

[Severity: Medium]
Does this length calculation double-count the first two elements of the 
vendor command array?

Since sizeof(struct fc_bsg_request) is 20 bytes and the vendor_cmd array 
starts at offset 12, vendor_cmd[2] is at offset 20. Adding 
2 * sizeof(uint32_t) requires an extra 8 bytes that are already accounted 
for in the struct size.

Will this reject perfectly sized valid requests and force userspace to 
over-allocate the payload buffer by 8 unused bytes?

[ ... ]
> +static int qla29xx_bsg_flash_block_write(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	if (bsg_job->request_len < sizeof(struct fc_bsg_request) +
> +	    2 * sizeof(uint32_t) + sizeof(struct qla_block_rw))
> +		return -EINVAL;

[Severity: Medium]
Is this subject to the same length calculation issue as the flash block 
read path above, where it over-estimates the required request size?

[ ... ]
> +	vfree(ha->optrom_buffer);
> +	ha->optrom_buffer = NULL;
> +	ha->optrom_state = QLA_SWAITING;
> +	mutex_unlock(&ha->optrom_mutex);
> +	bsg_job->reply_len = sizeof(struct fc_bsg_reply);
> +	bsg_job_done(bsg_job, bsg_reply->result,
> +			bsg_reply->reply_payload_rcv_len);
> +	return 0;
> +}

[Severity: High]
Because bsg_reply->reply_payload_rcv_len is never initialized in 
qla29xx_bsg_flash_block_write(), could this pass a stale value from a 
previous IO back to bsg_job_done()?

If the block layer reuses the kzalloc'd job->reply buffer from initialization, 
setting bsg_job->reply_len to the full size of struct fc_bsg_reply might 
copy this partially uninitialized buffer to userspace, leaking kernel memory 
and potentially triggering a WARN_ON in the BSG layer if the stale value 
is too large.

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