Re: [PATCH v2 02/60] scsi: qla2xxx: Add flash read/write interface for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] FLT version check is bypassed due to incorrect logical operator and signature constant.
- [High] Flash layout parsing iterates over regions using a fixed struct size instead of the firmware-provided `region_size`.
- [High] Unprotected concurrent modification of `ha->flt_data` during ISP abort can cause out-of-bounds reads in sysfs/BSG handlers.
--

commit 406f516c2c12956321654662c33a7512be63f8be
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add flash read/write interface for 29xx

The 29xx series uses a different flash access mechanism than
earlier adapters. Add the mailbox wrappers and qla_sup helpers
needed for flash read and write operations, including the
necessary hooks in isp_ops so that the existing flash
infrastructure can drive the new hardware.

> diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
> index b6c36a8a2d60b..eb10904f14caa 100644
> --- a/drivers/scsi/qla2xxx/qla_sup.c
> +++ b/drivers/scsi/qla2xxx/qla_sup.c
> @@ -10,6 +10,651 @@
>  #include <linux/vmalloc.h>
>  #include <linux/uaccess.h>
>  
> +/**
> + * qla29xx_get_flt_layout - Retrieve the flash layout table (FLT) for QLA29xx.
[ ... ]
> +static void
> +qla29xx_get_flt_layout(scsi_qla_host_t *vha)
> +{
> +	struct qla_hw_data *ha = vha->hw;
> +	struct qla_flash_layout *flt_layout = ha->flt_data;
> +	struct qla_flt_region_header *flt_header = &flt_layout->flt_header;
> +	struct qla_flt_region_data *region = &flt_layout->region[0];
> +	uint32_t flt_options = 0;
> +	uint32_t flt_size;
> +	uint32_t cnt, chksum;
> +	uint16_t reg_cnt, i;
> +	__le32 *wptr;
> +	void *buf = NULL;
> +
> +	flt_size = sizeof(struct qla_flt_region_header);
> +	wptr = (__force __le32 *)ha->flt_data;
> +
> +	buf = qla29xx_read_optrom_data(vha, FLT_REG_MINI_FLT, flt_options,
> +				       ha->flt_data, 0, flt_size);

[Severity: High]
Is ha->flt_data safe from concurrent modification here?

This function can be reached asynchronously via the DPC thread during an
ISP abort (qla2x00_do_dpc -> qla2x00_abort_isp -> qla2x00_setup_chip ->
qla2xxx_get_flash_info), and it unconditionally overwrites ha->flt_data
without acquiring ha->optrom_mutex.

Concurrently, a sysfs or BSG read request might execute
qla29xx_get_flash_region(), holding only ha->optrom_mutex. If this
overwrite happens at the same time, the reading thread could see a torn
or garbage region_count value (up to 65535). Since the flt_data array is
sized for 255 regions, the subsequent loop could read far past the
allocated buffer bounds.

> +	if (!buf) {
> +		ql_log(ql_log_warn, vha, 0x007b,
> +		    "Failed to read FLT information.\n");
> +		goto exit_flt;
> +	}
> +
> +	ql_dbg(ql_dbg_init + ql_dbg_buffer, vha, 0x0111,
> +	       "Contents of Flash Layout (0x%x):\n", flt_size);
> +	ql_dump_buffer(ql_dbg_init + ql_dbg_buffer, vha, 0x0112,
> +		       ha->flt_data, flt_size);
> +
> +	/* check flt version checksum and other info */
> +	if ((le32_to_cpu(*wptr) == 0xffff) &&
> +	    flt_header->version != cpu_to_le32(FLT_HDR_VERSION)) {

[Severity: High]
Will this correctly catch corrupted or invalid FLT headers?

It looks like the logical && is used instead of ||, and the 32-bit
pointer is checked against 0xffff (which resolves to 0x0000ffff)
instead of 0xffffffff for an empty flash block.

Because both conditions must be true for the error to trigger, corrupted
or uninitialized flash data with an incorrect version but a first DWORD
other than 0x0000ffff might silently bypass this validation check. This
would allow the driver to parse garbage data as valid flash layout
regions.

> +		ql_log(ql_log_warn, vha, 0x007c,
[ ... ]
> +	ql_dbg(ql_dbg_init, vha, 0x007f,
> +		"FLT detected: version=0x%08x length=0x%08x signature=0x%08x region_count=0x%04x region_len=0x%04x segment_len=0x%08x checksum=0x%08x.\n",
> +		le32_to_cpu(flt_header->version),
> +		le32_to_cpu(flt_header->length),
> +		le32_to_cpu(flt_header->signature),
> +		le16_to_cpu(flt_header->region_count),
> +		le16_to_cpu(flt_header->region_size),
> +		le32_to_cpu(flt_header->segment_size),
> +		le32_to_cpu(flt_header->checksum));
> +
> +	for (i = 0; reg_cnt; i++, reg_cnt--) {
> +		region = &flt_layout->region[i];

[Severity: High]
Could this array indexing result in misaligned memory reads?

The total flash layout size is calculated dynamically using
le16_to_cpu(flt_header->region_size). However, standard C array indexing
(&flt_layout->region[i]) strides by the compile-time size of
struct qla_flt_region_data.

If the firmware provides a region_size that differs from
sizeof(struct qla_flt_region_data) due to padding or future extensions,
the pointer arithmetic will be misaligned, causing the driver to read
garbage memory for subsequent regions.

> +		ql_dbg(ql_dbg_init, vha, 0x0080,
[ ... ]
> +static int
> +qla29xx_get_flash_region(struct scsi_qla_host *vha, uint32_t code,
> +			 struct qla_flt_region_data *region)
> +{
> +	struct qla_hw_data *ha = vha->hw;
> +	struct qla_flash_layout *flt = ha->flt_data;
> +	struct qla_flt_region_header *flt_hdr = &flt->flt_header;
> +	struct qla_flt_region_data *flt_reg = &flt->region[0];
> +	uint16_t cnt;
> +	int rval = QLA_FUNCTION_FAILED;
> +
> +	if (!ha->flt_data || !ha->flags.valid_flt)
> +		return QLA_FUNCTION_FAILED;
> +
> +	cnt = le16_to_cpu(flt_hdr->region_count);
> +	for (; cnt; cnt--, flt_reg++) {

[Severity: High]
Does this loop also suffer from the same fixed-size stride issue as above?

Using flt_reg++ advances the pointer by sizeof(struct qla_flt_region_data)
rather than the dynamic region_size provided by the firmware. If those
sizes differ, this will misalign and read garbage memory.

> +		if (le16_to_cpu(flt_reg->region_code) == code) {
> +			memcpy((uint8_t *)region, flt_reg,
> +			    sizeof(struct qla_flt_region_data));
> +			rval = QLA_SUCCESS;
> +			break;
> +		}
> +	}
> +
> +	return rval;
> +}

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