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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The `qla29xx_isp_ops` structure is missing the `.start_scsi` function pointer, leading to a NULL pointer dereference.
- [High] `qla29xx_load_fw_template` misinterprets the template size, truncating the buffer, and incorrectly copies the flash wrapper header.
- [Medium] Missing bounds check on `ha->fw_dump_tmplt_len` before parsing template size in `qla29xx_load_fw_template`.
- [Medium] Flawed bounds check when reading `risc_attr` from Array 1 in `qla29xx_load_risc_blob()`.
--

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

This commit adds support for loading 29xx FC operational firmware from the
filesystem. It also sets up the corresponding firmware dump template following
the 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
> index c6e928f11758c..5b0dc9b0a679c 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
[ ... ]
> @@ -9128,6 +9131,539 @@ qla81xx_load_risc(scsi_qla_host_t *vha, uint32_t *srisc_addr)
> +static int
> +qla29xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)
> +{
[ ... ]
> +		/* Array 2 */
> +		offset += header->array1_length;
> +		if (offset + header->array2_length > fw_size) {
> +			ql_log(ql_log_fatal, vha, 0x0098,
> +			       "FW image too small for Arr2 (%#zx > %#zx).\n",
> +			       offset + header->array2_length, fw_size);
> +			goto img_corrupt;
> +		}
> +		rval = qla29xx_process_rd_image(vha, header,
> +			(__be32 *)((char *)fwcode + offset),
> +			ARR2, header->array2_destination_addr,
> +			header->array2_length);
> +		if (rval) {
> +			ql_log(ql_log_fatal, vha, 0x0098,
> +			       "-> Failed load Arr2\n");
> +			goto img_corrupt;
> +		}
> +
> +		/* Array 3: FW_DUMP template */
> +		if (offset + 10 * sizeof(__be32) > fw_size) {
> +			ql_log(ql_log_fatal, vha, 0x0098,
> +			       "FW image too small for risc_attr.\n");
> +			goto img_corrupt;
> +		}
> +		array1_addr = (__be32 *)((char *)fwcode +
> +				header->header_length +
> +				header->tim_length);
> +		risc_attr = be32_to_cpu(array1_addr[9]);

[Severity: Medium]
Does this bounds check correctly protect the read from array1_addr? 

At this point, offset has already been advanced to the start of Array 2.
This checks if there are 40 bytes available after Array 2, but doesn't
verify if Array 1 itself is at least 40 bytes long. If Array 1 is
undersized, array1_addr[9] will erroneously read into Array 2 data.

[ ... ]
> +static int
> +qla29xx_load_fw_template(scsi_qla_host_t *vha)
> +{
> +	struct qla_hw_data *ha = vha->hw;
> +	struct fwdt *fwdt = ha->fwdt;
> +	uint templates = 1;
[ ... ]
> +	for (j = 0; j < templates; j++, fwdt++) {
> +		vfree(fwdt->template);
> +		fwdt->template = NULL;
> +		fwdt->length = 0;
> +
> +		template_size = le32_to_cpu(((__le32 *)fw_dump_tmplt)[2]);

[Severity: Medium]
Could ha->fw_dump_tmplt_len be less than 12 bytes here? If the flash layout
defines a length smaller than 12, this array index will read out of bounds
of the kzalloc buffer.

> +
> +		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);
> +		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);

[Severity: High]
Is template_size stored in dwords in the flash region? If so,
vmalloc(template_size) allocates 1/4th of the required memory.

Also, does the memcpy() need to skip the 7-dword flash wrapper header? As
written, it copies the header into fwdt->template, which will cause
qla27xx_fwdt_template_valid() to fail.

Furthermore, the loop hardcodes templates = 1 and doesn't advance
fw_dump_tmplt, which ignores any secondary MPI firmware dump template.

> +
> +		if (!qla27xx_fwdt_template_valid(dcode)) {
> +			ql_log(ql_log_warn, vha, 0x0165,
> +			       "-> fwdt%u failed template validate (rval %x)\n",
> +			       j, rval);
> +			goto failed;
> +		}
[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index c3eb941f71ad9..6560e58ad87c7 100644
> --- 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,
> +	.reset_chip		= qla24xx_reset_chip,
> +	.chip_diag		= qla24xx_chip_diag,
> +	.config_rings		= qla24xx_config_rings,
> +	.reset_adapter		= qla24xx_reset_adapter,
> +	.nvram_config		= qla81xx_nvram_config,
> +	.update_fw_options	= qla24xx_update_fw_options,
> +	.load_risc		= qla29xx_load_risc,
> +	.pci_info_str		= qla24xx_pci_info_str,
> +	.fw_version_str		= qla24xx_fw_version_str,
> +	.intr_handler		= qla24xx_intr_handler,
> +	.enable_intrs		= qla24xx_enable_intrs,
> +	.disable_intrs		= qla24xx_disable_intrs,
> +	.abort_command		= qla24xx_abort_command,
> +	.target_reset		= qla24xx_abort_target,
> +	.lun_reset		= qla24xx_lun_reset,
> +	.fabric_login		= qla24xx_login_fabric,
> +	.fabric_logout		= qla24xx_fabric_logout,
> +	.calc_req_entries	= NULL,
> +	.build_iocbs		= NULL,
> +	.prep_ms_iocb		= qla24xx_prep_ms_iocb,
> +	.prep_ms_fdmi_iocb	= qla24xx_prep_ms_fdmi_iocb,
> +	.read_nvram		= NULL,
> +	.write_nvram		= NULL,
> +	.fw_dump		= qla27xx_fwdump,
> +	.mpi_fw_dump		= qla27xx_mpi_fwdump,
> +	.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,
> +	.get_flash_version	= qla24xx_get_flash_version,
> +	.start_scsi_mq		= qla2xxx_dif_start_scsi_mq,
> +	.abort_isp		= qla2x00_abort_isp,
> +	.iospace_config		= qla83xx_iospace_config,
> +	.initialize_adapter	= qla2x00_initialize_adapter,
> +};

[Severity: Critical]
Is the missing .start_scsi pointer here intentional? 

If ha->mqenable is false or the designated qpair is NULL (e.g., due to a 
queue mapping failure or fallback to single-queue mode), 
qla2xxx_queuecommand() falls back to the legacy single-queue path by calling
ha->isp_ops->start_scsi(sp). 

Since this pointer is omitted, it defaults to NULL and will result in a NULL
pointer dereference.

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