Re: [PATCH v2 06/60] scsi: qla2xxx: Add FC operational firmware load for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unconditional byte-swapping of `risc_attr` using `swab32()` instead of `be32_to_cpu()` causes endianness corruption.
- [High] Heap out-of-bounds read in `qla29xx_load_fw_template()` due to missing bounds check on `template_size`.
- [High] Out-of-bounds read in `qla29xx_load_risc_blob()` due to missing bounds checks on `array3_addr` dereferences.
- [High] Logic Error: Missing `BIT_1` (last chunk) flag due to flawed `is_last_chunk` calculation.
- [High] Integer overflow of firmware section offsets and lengths bypasses bounds checks on 32-bit systems.
--

commit 010601e8a1ad75507948f2ba92e178c2b94266b2
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 sets up the corresponding firmware dump template.

> 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

[ ... ]

> +static int qla29xx_process_rd_image(struct scsi_qla_host *vha,

[ ... ]

> +				is_last_chunk =
> +					(chunk == (chunks_per_segment - 1) ||
> +					 size_remainder < CHUNK_SIZE);

[Severity: High]
Does this logic accurately capture the final chunk when the remaining size is
exactly a multiple of the chunk size?

If the final segment's remaining size is exactly CHUNK_SIZE, size_remainder <
CHUNK_SIZE evaluates to false. Because it is a partial segment, the chunk
index will also be less than chunks_per_segment - 1. Since both conditions
are false, is_last_chunk becomes false. Could this cause the final chunk to
be sent without the BIT_1 flag, potentially hanging the firmware load?

[ ... ]

> +static int
> +qla29xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)
> +{

[ ... ]

> +	header->header_length <<= 2;
> +	header->tim_length <<= 2;
> +	header->array1_length <<= 2;
> +	header->array2_length <<= 2;
> +	header->array3_length <<= 2;
> +	header->array4_length <<= 2;
> +
> +	if (!header->segment_size) {

[Severity: High]
Could these shifts result in integer overflow on 32-bit systems?

The firmware section lengths are multiplied by 4 without verifying if the
multiplication overflows.

[ ... ]

> +		/* Array 1 */
> +		offset += header->tim_length;
> +		if (offset + header->array1_length > fw_size) {
> +			ql_log(ql_log_fatal, vha, 0x0098,

If a crafted firmware image provides large section lengths (for instance,
0xFFFFFFFC), adding the offset could wrap around to a small value on 32-bit
architectures. Would this bypass the bounds check and cause the inner copy
loop in qla29xx_process_rd_image() to read past the end of the firmware
buffer?

[ ... ]

> +		/* 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 = swab32(array1_addr[9]);

[Severity: High]
Does using swab32() here cause endianness issues on big-endian architectures?

Since array1_addr is defined as an array of __be32, the data is already being
read in native memory order. Converting this to CPU endianness typically
requires be32_to_cpu(). Using an unconditional byte swap produces the correct
result on little-endian machines, but might corrupt the value on big-endian
systems, which could break the template count logic.

[ ... ]

> +	array3_addr = (__be32 *)(((char *)fwcode) + array3_offset);
> +	for (j = 0; j < templates; j++, fwdt++) {
> +		vfree(fwdt->template);
> +		fwdt->template = NULL;
> +		fwdt->length = 0;
> +
> +		array_size = be32_to_cpu(array3_addr[2]);

[Severity: High]
Is there a bounds check ensuring array3_addr has enough capacity before
dereferencing it?

While there is a check earlier to ensure array3_offset does not exceed the
total firmware size, that check allows the offset to sit exactly at the
end of the file. Dereferencing array3_addr[2] here might read past the end
of the buffer if the remaining size is less than 12 bytes.

[ ... ]

> +		fwdt->length = dlen;
> +		ql_dbg(ql_dbg_init, vha, 0x0168,
> +		       "-> fwdt%u loaded template ok\n", j);
> +
> +		array3_addr += array_size + 1;
> +	}

Additionally, if the loop continues for another template, array3_addr is
advanced and immediately dereferenced at the start of the next iteration
without validating if the new address remains within the firmware blob.
Could this also lead to an out-of-bounds read?

[ ... ]

> +static int
> +qla29xx_load_fw_template(scsi_qla_host_t *vha)
> +{

[ ... ]

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

[ ... ]

> +		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]
Does template_size need to be validated against the actual size of the
fw_dump_tmplt allocation?

The template_size is read directly from the flash buffer data. Without
checking if template_size is less than or equal to ha->fw_dump_tmplt_len,
a malformed flash image specifying an excessively large template_size might
cause memcpy() to read out of bounds from the fw_dump_tmplt heap allocation.

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