Re: [PATCH v5 19/56] scsi: qla2xxx: Enable serdes, resource count and FCE trace for 29xx

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `kstrtoul` function in `qla2x00_dfs_fce_write` is called with a NULL pointer for its output parameter, leading to a guaranteed kernel crash when a valid input is parsed.
- [High] Dynamically enabling FCE via debugfs causes a heap buffer overflow during firmware dump collection because the pre-allocated dump buffer is not resized.
- [High] Concurrent execution of `qla2x00_dfs_fce_write` and firmware dump generation causes a use-after-free on `ha->fce`.
- [High] QLA29XX is missing from the adapter shutdown firmware cleanup logic, leading to improper shutdown state.
--

--- Patch [19]: [PATCH v5 19/56] scsi: qla2xxx: Enable serdes, resource count and FCE trace for 29xx ---
commit 128c0c55efd466adf4231392e126249fba36c3dc
Author: Nilesh Javali <njavali@marvell.com>
Subject: scsi: qla2xxx: Enable serdes, resource count and FCE trace for 29xx

The 29xx adapters share the diagnostic and management interfaces
already supported on ISP27xx/28xx, but several family capability
gates still omitted IS_QLA29XX(), leaving these paths unreachable
on 29xx.

> diff --git a/drivers/scsi/qla2xxx/qla_dfs.c b/drivers/scsi/qla2xxx/qla_dfs.c
> index 43970caca7b39..177d47e92e494 100644
> --- a/drivers/scsi/qla2xxx/qla_dfs.c
> +++ b/drivers/scsi/qla2xxx/qla_dfs.c
> @@ -497,7 +497,7 @@ qla2x00_dfs_fce_write(struct file *file, const char __user *buffer,
>  	unsigned long enable;
>  
>  	if (!IS_QLA25XX(ha) && !IS_QLA81XX(ha) && !IS_QLA83XX(ha) &&
> -	    !IS_QLA27XX(ha) && !IS_QLA28XX(ha)) {
> +	    !IS_QLA27XX(ha) && !IS_QLA28XX(ha) && !IS_QLA29XX(ha)) {
>  		ql_dbg(ql_dbg_user, vha, 0xd034,
>  		       "this adapter does not support FCE.");
>  		return -EINVAL;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but since 29xx adapters will now
execute this code, could we fix a NULL pointer dereference later in this
function?

In qla2x00_dfs_fce_write(), kstrtoul() is called with a NULL pointer for the
result:

	enable = kstrtoul(buf, 0, 0);

A successful parse will unconditionally write the value to the address
provided, resulting in a kernel panic when a valid number string is
written to the debugfs node.

[Severity: High]
This is a pre-existing issue, but enabling FCE dynamically via debugfs might
corrupt memory when a firmware dump is collected.

When FCE is enabled, qla2x00_dfs_fce_write() attempts to adjust the firmware
dump buffer:

	if (!ha->flags.fce_dump_buf_alloced)
		qla2x00_alloc_fw_dump(vha);

However, qla2x00_alloc_fw_dump() returns early if the buffer is already
allocated, failing to resize it to accommodate the FCE data:

	if (ha->fw_dump) {
		ql_dbg(ql_dbg_init, vha, 0x00bd,
		    "Firmware dump already allocated.\n");
		return;
	}

If a firmware dump is later triggered, functions like qla27xx_insertbuf()
will append the FCE trace to the undersized buffer:

	buf += *len;
	memcpy(buf, mem, size);

Could this cause a heap buffer overflow?

[Severity: High]
This is a pre-existing issue, but does the FCE trace buffer free path
have a race condition with firmware dump generation?

When disabling FCE, qla2x00_dfs_fce_write() frees the buffer under fce_mutex:

	qla2x00_free_fce_trace(ha);

Concurrently, if a firmware crash triggers a dump, qla27xx_fwdt_entry_t264()
reads and dereferences the pointer without holding fce_mutex:

	if (vha->hw->fce) {
		...
		qla27xx_insertbuf(vha->hw->fce, FCE_SIZE, buf, len);

Can this lead to a use-after-free if the dump routine reads the pointer before
it is freed but executes the memcpy after the free?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but should the IS_QLA29XX check
also be added to qla2x00_shutdown() in qla_os.c?

During system shutdown, the capability gate bypasses
qla2x00_abort_isp_cleanup() for QLA29XX:

	if (IS_QLA25XX(ha) ||  IS_QLA2031(ha) || IS_QLA27XX(ha) ||
	    IS_QLA28XX(ha)) {
		if (ha->flags.fw_started)
			qla2x00_abort_isp_cleanup(vha);
	} else {
	...

This leaves the firmware and DMA engines active, which might cause memory
corruption or hangs during kexec or a warm reboot.

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