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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `.start_scsi` function pointer in `qla29xx_isp_ops` leads to NULL pointer dereference.
- [Medium] Out-of-bounds read and missing minimum size check in `qla29xx_load_fw_template()`.
- [Low] Wasteful use of `vmalloc()` for a small, short-lived 80-byte structure (`struct fcop_header`).
--

--- Patch [5]: [PATCH v4 05/56] scsi: qla2xxx: Add FC operational firmware load for 29xx ---
commit 9c9c2ed926e65df8d7c92e4c4efc603458f304c5
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 implements the request_firmware and segment-load pattern
used by earlier adapters and sets up the firmware dump template.

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index f5d230fc3642d..f6a8bcbb2a037 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)
> +{
> +	int rval;
> +	struct fcop_header *header;
> +	uint32_t *dcode = NULL;
> +	struct fw_blob *blob;
> +	__be32 *fwcode, *array1_addr, *array3_addr;
> +	uint templates;
> +	uint32_t array_size, risc_attr = 0;
> +	ulong i;
> +	uint j;
> +	ulong dlen, array3_offset;
> +	size_t max_dwords;
> +	struct qla_hw_data *ha = vha->hw;
> +	struct fwdt *fwdt = ha->fwdt;
> +
> +	header = vmalloc(sizeof(*header));

[Severity: Low]
Is it necessary to use vmalloc() for this 80-byte fcop_header structure?
Allocating such a small structure with vmalloc consumes a full page and TLB
entries. Would it be better to allocate this on the stack or use kzalloc() 
instead?

> +	if (!header)
> +		return QLA_FUNCTION_FAILED;
[ ... ]
> +static int
> +qla29xx_load_fw_template(scsi_qla_host_t *vha)
> +{
[ ... ]
> +	buf = qla29xx_read_optrom_data(vha, FLT_REG_FW_DUMP_TMPLT, 0,
> +				       fw_dump_tmplt, 0,
> +				       ha->fw_dump_tmplt_len);
> +	if (!buf) {
> +		ql_log(ql_log_info, vha, 0x0013,
> +		       "Unable to read fw dump temp info.\n");
> +		goto free_fw_dump;
> +	}
> +
> +	ql_dump_buffer(ql_dbg_init, vha, 0x006b, fw_dump_tmplt,
> +		       min_t(uint32_t, ha->fw_dump_tmplt_len, 1024));
> +
> +	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 this access out-of-bounds memory?

This reads the third 32-bit word of the fw_dump_tmplt buffer. If the
flash or firmware advertises a ha->fw_dump_tmplt_len that is smaller
than 12 bytes, this will read past the end of the allocation. Should
there be a minimum size check against fw_dump_tmplt_len before this
access?

> +
> +		ql_dbg(ql_dbg_init, vha, 0x0161,
> +		       "-> fwdt%u template array at %p (0x%x bytes)\n",
> +		       j, fw_dump_tmplt, template_size);
[ ... ]
> 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,
[ ... ]
> +	.get_flash_version	= qla24xx_get_flash_version,
> +	.start_scsi_mq		= qla2xxx_dif_start_scsi_mq,
> +	.abort_isp		= qla2x00_abort_isp,

[Severity: High]
Does this isp_operations struct need a .start_scsi callback?

In qla2xxx_queuecommand(), if multiqueue is disabled via module parameters
(ql2xmqsupport=0) or falls back because MSI-X allocation fails, the code
bypasses the MQ path and invokes:

    rval = ha->isp_ops->start_scsi(sp);

Since the .start_scsi pointer is omitted for 29xx, won't this result in a
NULL pointer dereference?

> +	.iospace_config		= qla83xx_iospace_config,
> +	.initialize_adapter	= qla2x00_initialize_adapter,
> +};

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