Re: [PATCH v2 21/60] 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] Calling kstrtoul(buf, 0, 0) passes a NULL pointer for the result, which unconditionally causes a kernel panic upon successful string parsing.
- [High] qla2x00_alloc_fw_dump() lacks an IS_QLA29XX check when deciding whether to size the fw_dump buffer using FWDT, leading to legacy fixed-size allocations that are too small.
- [High] Enabling FCE trace dynamically via debugfs does not resize the pre-allocated firmware dump buffer, leading to an out-of-bounds write of 64KB (FCE_SIZE) during a firmware dump.
- [High] A race condition between disabling the FCE trace via debugfs and a concurrent firmware dump leads to a Use-After-Free of the DMA-allocated FCE trace buffer.
--

commit f8a1650862943af8f27bc3e84bb79567498967d8
Author: Nilesh Javali <njavali@marvell.com>

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)) {

[Severity: High]
This isn't a bug introduced by this patch, but exposing this interface to 29xx
adapters makes it relevant here. Looking further down in qla2x00_dfs_fce_write(),
there is a call to kstrtoul():

    enable = kstrtoul(buf, 0, 0);
    rc = count;

Does passing a NULL pointer as the third argument to kstrtoul() cause a kernel
crash when parsing succeeds? Additionally, since kstrtoul() returns a status
code (0 on success, negative on failure), does assigning its return value
directly to the boolean enable flag cause inverse logic where writing a valid
number skips FCE enablement?

[Severity: High]
This is a pre-existing issue, but since 29xx adapters are now enabling FCE and
fw dump features, should IS_QLA29XX be added to the sizing check in
qla2x00_alloc_fw_dump()?

Currently, the check only includes IS_QLA27XX and IS_QLA28XX:

    if (IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
        struct fwdt *fwdt = ha->fwdt;

If a 29xx adapter falls through this check, it defaults to legacy fixed-size
allocations that are too small. When qla27xx_fwdump() later executes the FWDT
templates for QLA29XX, could this write past the end of the undersized buffer?

[Severity: High]
This is a pre-existing issue, but exposing FCE trace dynamically via debugfs
highlights it. When qla2x00_dfs_fce_write() enables FCE, it attempts to resize
the firmware dump buffer to account for the new 64KB FCE trace:

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

However, if ha->fw_dump is already allocated, qla2x00_alloc_fw_dump() returns
immediately without expanding the buffer.

Later, if a firmware dump occurs, qla27xx_fwdt_entry_t264() sees ha->fce is not
NULL and unconditionally copies FCE_SIZE (64KB) bytes into the dump buffer.
Can this result in an out-of-bounds write since the buffer was never actually
enlarged?

[Severity: High]
This is a pre-existing issue, but could disabling the FCE trace via this
debugfs node race with a concurrent firmware dump?

When FCE is disabled in qla2x00_dfs_fce_write(), qla2x00_free_fce_trace()
frees the DMA memory and sets ha->fce to NULL under ha->fce_mutex, but
without holding ha->hardware_lock.

Conversely, the firmware dump routines (like qla27xx_fwdt_entry_t264)
execute under ha->hardware_lock and read from ha->fce without acquiring
ha->fce_mutex.

Could this race allow the FCE buffer to be freed after the dump routine
verifies the pointer but before the memcpy completes, leading to a
use-after-free?

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