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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Silent truncation of the 32-bit `region` parameter to 16 bits in BSG flash read/write handlers.
--

commit 42123b5c17ee809c301462b168b534861a97e40d
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
> index 5e910b5ca670d..0baf486e8fb89 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
[ ... ]
> +static int qla29xx_bsg_flash_block_read(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	buf = ha->isp_ops->read_optrom_region(vha, brcmd->region, opts,
> +				ha->optrom_buffer, ha->optrom_region_start,
> +				ha->optrom_region_size);

[Severity: Medium]
Does this code silently truncate brcmd->region? 

The qla_block_rw structure defines region as a uint32_t, but the
read_optrom_region() function pointer takes a uint16_t for the reg_code
parameter.

If userspace passes a region ID greater than 0xFFFF, this will silently
truncate the ID instead of returning -EINVAL. Could this lead to accidental
firmware corruption if an invalid region ID wraps around to a valid one?

[ ... ]
> +static int qla29xx_bsg_flash_block_write(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	rval = ha->isp_ops->write_optrom_region(vha, bwcmd->region, opts,
> +				ha->optrom_buffer, ha->optrom_region_start,
> +				ha->optrom_region_size);

[Severity: Medium]
Similarly, does this silently truncate bwcmd->region from uint32_t to
uint16_t when passing it to write_optrom_region()?

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.h b/drivers/scsi/qla2xxx/qla_bsg.h
> index a920c8e482bc6..ca0d83986b57f 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.h
> +++ b/drivers/scsi/qla2xxx/qla_bsg.h
[ ... ]
> +struct qla_block_rw {
> +	uint32_t region;
> +	uint32_t rw_length;
> +	uint32_t options;
> +	uint32_t region_offset;
> +	uint32_t chunk_length;
> +	uint8_t  reserved[44];
> +} __packed;

> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 1ec7ee578e0c3..719b6a1f9123a 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -3534,6 +3534,13 @@ struct isp_operations {
>  	int (*write_optrom)(struct scsi_qla_host *, void *, uint32_t,
>  		uint32_t);
>  
> +	void *(*read_optrom_region)(struct scsi_qla_host *vha,
> +		uint16_t reg_code, uint16_t opts, void *buf,
> +		uint32_t offset, uint32_t length);
> +	int (*write_optrom_region)(struct scsi_qla_host *vha,
> +		uint16_t reg_code, uint16_t opts, void *buf,
> +		uint32_t offset, uint32_t length);
> +

The function pointers use uint16_t for reg_code, but the caller above
provides a uint32_t.

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