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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `isp_ops` hooks for 29xx flash read/write cause sysfs MMIO out-of-bounds crashes.
- [High] Concurrent stateful flash read operations cause firmware state corruption due to missing `optrom_mutex`.
--

--- Patch [2]: [PATCH v5 02/56] scsi: qla2xxx: Add flash read/write interface for 29xx ---
commit 3849a5ddec04d8b5a4e4730fd38a29a1014a62fa
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.
This patch adds the mailbox wrappers and helpers needed for flash read and
write operations so that the existing flash infrastructure can drive the new
hardware.

Link: https://lore.kernel.org/oe-kbuild-all/202607141520.k9T31Dpp-lkp@intel.com/

> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
[ ... ]
> @@ -3237,6 +3237,7 @@ qla2x00_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  		ha->flash_data_off = ~0;
>  		ha->nvram_conf_off = ~0;
>  		ha->nvram_data_off = ~0;
> +		ha->flt_segment_length = QLA_SEGMENT_LENGTH;
>  	}

[Severity: High]
Are the isp_ops hooks for the 29xx flash read/write missing here?

The newly introduced qla29xx_read_optrom_data() and 
qla29xx_write_optrom_data() functions appear to be left unassigned to the 
29xx operations table. If the legacy handlers (like qla25xx_read_optrom_data) 
are still used by default, would reading or writing the optrom sysfs node 
attempt MMIO operations at iobase + ~0 and cause out-of-bounds PCIe memory 
crashes, since flash_conf_off and flash_data_off are initialized to ~0?

[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
> --- a/drivers/scsi/qla2xxx/qla_sup.c
> +++ b/drivers/scsi/qla2xxx/qla_sup.c
[ ... ]
> +static void
> +qla29xx_get_boot_version(scsi_qla_host_t *vha, void *mbuf)
> +{
> +	struct qla_hw_data *ha = vha->hw;
> +	uint8_t *bcode = mbuf;
> +	uint32_t pcids;
> +	uint8_t code_type;
> +
> +	if (!qla29xx_read_optrom_data(vha, FLT_REG_BOOT_CODE, 0, mbuf, 0,
> +				      QLA29XX_BOOT_PROBE_LEN)) {

[Severity: High]
Is it safe to call qla29xx_read_optrom_data() here without holding 
ha->optrom_mutex?

During an adapter reset, the background DPC thread can call 
qla2x00_initialize_adapter() which invokes ha->isp_ops->get_flash_version(), 
leading to this code without acquiring the mutex. 

If a user is concurrently reading or writing the sysfs optrom file (which 
does hold the mutex but wouldn't block this background path), could the 
interleaved stateful mailbox commands (MBC_RD_WR_FLASH) violate the firmware's 
flash state machine sequence and cause corruption?

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