Re: [PATCH v4 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] Incorrect usage of `kstrtoul` in `qla2x00_dfs_fce_write` passes a NULL pointer for the result argument, causing an unconditional kernel oops upon successful parsing.
- [High] A TOCTOU race condition in `qla2x00_dfs_fce_release` can lead to a NULL pointer dereference when accessing `ha->fce`.
- [High] Concurrent non-atomic modification of the `ha->flags` bitfield leads to data corruption, potentially dropping critical interrupts or mailbox states.
- [High] A concurrent firmware dump can trigger a Use-After-Free on `ha->fce` while the FCE trace is being disabled via DebugFS.
--

--- Patch [19]: [PATCH v4 19/56] scsi: qla2xxx: Enable serdes, resource count and FCE trace for 29xx ---
commit c0d2af5f457ed884072ede4fabe84388c9abed9e
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 isn't a bug introduced by this patch, but looking at the parsing logic
further down in qla2x00_dfs_fce_write():

qla2x00_dfs_fce_write() {
    ...
    enable = kstrtoul(buf, 0, 0);
    ...
}

Does this cause an unconditional kernel oops? kstrtoul() writes the parsed
result to the pointer provided in the third argument, which is 0 (NULL)
here. Additionally, the return value (0 on success, negative on error) is
assigned to enable, which appears to invert the intended logic.


[Severity: High]
This is a pre-existing issue, but does modifying the bitfields in
qla2x00_dfs_fce_write() risk dropping mailbox interrupts?

qla2x00_dfs_fce_write() {
    ...
    ha->flags.user_enabled_fce = 1;
    ...
}

Since user_enabled_fce shares the same 32-bit volatile struct word as
mbox_int and mbox_busy, and compilers use non-atomic read-modify-write
operations for bitfields, can a concurrent interrupt handler modifying those
mailbox flags overwrite this update or vice versa?


[Severity: High]
This is also a pre-existing issue, but is there a use-after-free risk when
disabling the FCE trace?

In qla2x00_dfs_fce_write(), the trace buffer is freed:

    qla2x00_free_fce_trace(ha);

This is called under ha->fce_mutex. However, a concurrent firmware dump in
qla25xx_fw_dump() calls qla25xx_copy_fce() which copies data from ha->fce
while only holding ha->hardware_lock:

qla25xx_copy_fce() {
    ...
    memcpy(iter_reg, ha->fce, ntohl(fcec->size));
    ...
}

If these paths do not share a lock, could a concurrent firmware dump read
from the freed FCE buffer?


[Severity: High]
This is another pre-existing issue, but could a race condition cause a NULL
pointer dereference in qla2x00_dfs_fce_release()?

The release function checks ha->fce outside of the mutex:

qla2x00_dfs_fce_release() {
    if (ha->flags.fce_enabled || !ha->fce)
        goto out;

    mutex_lock(&ha->fce_mutex);
    ha->flags.fce_enabled = 1;
    memset(ha->fce, 0, fce_calc_size(ha->fce_bufs));
    ...
}

If a concurrent write to qla2x00_dfs_fce_write() acquires the mutex, frees
ha->fce, sets it to NULL, and releases the mutex, will the release thread
then acquire the mutex and execute memset() on the NULL pointer?

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