Re: [PATCH v6 05/56] scsi: qla2xxx: Add FC operational firmware load for 29xx

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `.start_scsi` function pointer is missing from the `qla29xx_isp_ops` structure.
- [High] The `.read_optrom` and `.write_optrom` callbacks in `qla29xx_isp_ops` incorrectly use legacy handlers that trigger invalid flash accesses.
- [High] `qla29xx_load_fw_template()` misinterprets the firmware dump template size as bytes instead of dwords, leading to severe memory under-allocation.
- [High] `qla29xx_load_fw_template()` fails to skip the flash region header when extracting the firmware dump template, breaking validation.
- [High] Firmware dump template validation failures in 29xx load paths incorrectly abort the entire adapter initialization.

Pre-existing issues:
- [High] Use-After-Free in `ha->mpi_fw_dump` pointer when firmware dump buffer is reallocated.
--

--- Patch [5]: [PATCH v6 05/56] scsi: qla2xxx: Add FC operational firmware load for 29xx ---
commit ed0a0bf5b60c5fe925e18499926faba91b6ac03a
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add FC operational firmware load for 29xx

This commit adds support to load the 29xx FC operational firmware from the
filesystem and to set up the corresponding firmware dump template. It
follows the same request_firmware and segment load pattern used by earlier
adapters.

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -3937,7 +3937,8 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
>  				    "Allocated (%d KB) for firmware dump.\n",
>  				    dump_size / 1024);
>  
> -				if (IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
> +				if (IS_QLA27XX(ha) || IS_QLA28XX(ha) ||
> +				    IS_QLA29XX(ha)) {
>  					ha->mpi_fw_dump = (char *)fw_dump +
>  						ha->fwdt[1].dump_size;
>  					mutex_unlock(&ha->optrom_mutex);

[Severity: High]
This is a pre-existing issue, but does adding 29xx to this path expose a
use-after-free when the firmware dump buffer is reallocated?

Earlier in qla2x00_alloc_fw_dump(), when ha->fw_dumped is true, the old
buffer is freed via vfree(ha->fw_dump) and reallocated. However, the
ha->mpi_fw_dump pointer is not updated to point into the new buffer. It
seems it will remain pointing to the freed memory, and a subsequent MPI
firmware dump would corrupt memory.

[ ... ]
> +static int
> +qla29xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)
> +{
[ ... ]
> +failed:
> +	vfree(fwdt->template);
> +	fwdt->template = NULL;
> +	fwdt->length = 0;
> +img_corrupt:
> +	vfree(header);
> +	return QLA_FUNCTION_FAILED;
> +}

[Severity: High]
Should template validation failures in the 29xx load paths abort the entire
adapter initialization?

In previous hardware generations, firmware dump template failures are
treated as non-fatal and return QLA_SUCCESS, allowing the driver to operate
without firmware dump capabilities. Returning QLA_FUNCTION_FAILED here
propagates up and makes the adapter completely unusable if the optional
template validation fails.

[ ... ]
> +static int
> +qla29xx_load_fw_template(scsi_qla_host_t *vha)
> +{
[ ... ]
> +		template_size = le32_to_cpu(((__le32 *)fw_dump_tmplt)[2]);
> +
> +		ql_dbg(ql_dbg_init, vha, 0x0161,
> +		       "-> fwdt%u template array at %p (0x%x bytes)\n",
> +		       j, fw_dump_tmplt, template_size);
> +
> +		if (!template_size || !~template_size ||
> +		    template_size > ha->fw_dump_tmplt_len) {
> +			ql_dbg(ql_dbg_init, vha, 0x0162,
> +			       "-> fwdt%u failed to read array\n", j);
> +			goto failed;
> +		}
> +
> +		fwdt->template = vmalloc(template_size);

[Severity: High]
Does this misinterpret the firmware dump template size as bytes instead of
dwords?

The template_size is read from the flash header which typically stores the
size in dwords. Passing this directly to vmalloc() and memcpy() would
allocate and copy only 25 percent of the needed memory, and guarantee that
the subsequent size verification (len > template_size) fails.

> +		if (!fwdt->template) {
> +			ql_log(ql_log_warn, vha, 0x0164,
> +			       "-> fwdt%u failed allocate template.\n", j);
> +			goto failed;
> +		}
> +
> +		dcode = fwdt->template;
> +		memcpy((char *)dcode, (char *)fw_dump_tmplt, template_size);
> +
> +		if (!qla27xx_fwdt_template_valid(dcode)) {

[Severity: High]
Does this copy fail to skip the 7-dword flash region header?

Other firmware load functions skip the header before extracting the
template. Copying directly from offset 0 means the dcode buffer points to
the flash header instead of the template header, which would cause
qla27xx_fwdt_template_valid() to unconditionally fail.

[ ... ]
> +free_fw_dump:
> +	kfree(fw_dump_tmplt);
> +	return QLA_FUNCTION_FAILED;
> +}

[Severity: High]
As mentioned above, should this optional firmware dump template failure
abort the entire adapter initialization? Returning QLA_FUNCTION_FAILED here
will fail the probe.

[ ... ]
> 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
> @@ -2640,6 +2640,45 @@ static struct isp_operations qla27xx_isp_ops = {
>  	.initialize_adapter	= qla2x00_initialize_adapter,
>  };
>  
> +static struct isp_operations qla29xx_isp_ops = {
> +	.pci_config		= qla25xx_pci_config,

[Severity: High]
The start_scsi function pointer appears to be missing from the
qla29xx_isp_ops structure.

In qla2xxx_queuecommand(), if ha->mqenable evaluates to 0 (which happens if
MSI-X allocation fails or is disabled by user), the driver falls back to
single-queue mode and explicitly calls ha->isp_ops->start_scsi(). Does this
missing callback cause a fatal NULL pointer dereference on the first
submitted SCSI I/O?

[ ... ]
> +	.beacon_on		= qla24xx_beacon_on,
> +	.beacon_off		= qla24xx_beacon_off,
> +	.beacon_blink		= qla83xx_beacon_blink,
> +	.read_optrom		= qla25xx_read_optrom_data,
> +	.write_optrom		= qla24xx_write_optrom_data,

[Severity: High]
Are these legacy flash I/O functions compatible with the 29xx adapter?

These functions compute flash addresses using flash_data_addr(), which adds
ha->flash_data_off to the offset. For 29xx, ha->flash_data_off is
initialized to ~0 (0xFFFFFFFF). This appears to cause invalid flash reads
and writes.

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