Re: [PATCH v6 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The debugfs write handler `qla2x00_dfs_fce_write()` passes a NULL pointer to `kstrtoul()`, guaranteeing a kernel crash upon successful parsing, and erroneously evaluates its error code as the parsed value.
- [High] Dynamically enabling FCE trace via debugfs causes a 64KB out-of-bounds write during a firmware dump because the fw_dump buffer is not properly resized.
- [High] Concurrent execution of a firmware dump and disabling the FCE trace via debugfs leads to a Use-After-Free of the FCE DMA buffer.
--

--- Patch [19]: [PATCH v6 19/56] scsi: qla2xxx: Enable serdes, resource count and FCE trace for 29xx ---
commit ce99a18d7438f0c23e7c18bd3dad0bfb7220456a
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)) {
>  		ql_dbg(ql_dbg_user, vha, 0xd034,
>  		       "this adapter does not support FCE.");
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but since this patch exposes the FCE debugfs
node to 29xx adapters, does writing to this node cause a guaranteed kernel
panic?

Looking at qla2x00_dfs_fce_write() just below this check:

    enable = kstrtoul(buf, 0, 0);

Does passing 0 (NULL) as the third argument to kstrtoul() cause a NULL pointer
dereference when the function attempts to write the successfully parsed value?

Additionally, since it assigns the return value (an error code) to enable
rather than the parsed number, does writing an invalid string return a
non-zero negative error code that evaluates to true, incorrectly enabling
the trace without crashing?

[Severity: High]
This is another pre-existing issue exposed by enabling FCE trace via debugfs,
but does dynamically enabling this feature cause an out-of-bounds write
during a firmware dump?

When FCE is enabled dynamically, qla2x00_dfs_fce_write() calls
qla2x00_alloc_fw_dump() to resize the dump buffer. However,
qla2x00_alloc_fw_dump() returns early if the buffer is already allocated:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_alloc_fw_dump() {
    if (ha->fw_dump) {
        ql_dbg(ql_dbg_init, vha, 0x00bd,
            "Firmware dump already allocated.\n");
        return;
    }
    ...
}

During a firmware dump, qla25xx_copy_fce() appends the 64KB FCE buffer to
the fw_dump buffer:

drivers/scsi/qla2xxx/qla_dbg.c:qla25xx_copy_fce() {
    ...
    memcpy(iter_reg, ha->fce, ntohl(fcec->size));
    ...
}

Does this blindly write 64KB past the end of the unresized firmware dump
buffer?

[Severity: High]
This is also a pre-existing issue, but does disabling the FCE trace via this
debugfs node race with firmware dumps, leading to a Use-After-Free?

When disabling FCE, qla2x00_dfs_fce_write() calls qla2x00_free_fce_trace()
which frees ha->fce using dma_free_coherent() while holding fce_mutex.

Concurrently, if a firmware dump triggers (e.g., via a system error),
the dump path accesses this same memory:

drivers/scsi/qla2xxx/qla_dbg.c:qla25xx_copy_fce() {
    ...
    memcpy(iter_reg, ha->fce, ntohl(fcec->size));
    ...
}

Since the dump path executes under hardware_lock but without holding
fce_mutex, and the free path does not acquire hardware_lock, can the dump
path observe a non-NULL pointer and access the memory after it has been
freed?

[ ... ]

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