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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Multi-queue qpairs on 29xx miss the inline IOCB submission optimization, leading to request ring corruption.

Pre-existing issues:
- [High] Enabling FCE trace via debugfs when `fw_dump` is already allocated causes a vmalloc out-of-bounds write during firmware dump.
- [High] Race condition between disabling FCE trace via debugfs and capturing a firmware dump leads to a Use-After-Free of `ha->fce`.
- [High] TOCTOU race condition in `qla2x00_dfs_fce_release()` causing a NULL pointer dereference.
- [High] Data race on bitfields in `ha->flags` between process context and hardware interrupt context.
--

--- Patch [19]: [PATCH v3 19/88] scsi: qla2xxx: Enable serdes, resource count and FCE trace for 29xx ---
commit edd8bd44658eaa99ef8b8fe98bcd34952eb875a0
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 enabling FCE trace via debugfs
when fw_dump is already allocated seems to cause a vmalloc out-of-bounds write
during a firmware dump.

If ql2xallocfwdump is true, qla2x00_alloc_fw_dump() is called at probe while
ha->fce is still NULL, omitting FCE_SIZE.  If we then enable FCE via debugfs:

drivers/scsi/qla2xxx/qla_dfs.c:qla2x00_dfs_fce_write() {
    ...
    if (!ha->flags.fce_dump_buf_alloced)
        qla2x00_alloc_fw_dump(vha);
    ...
}

qla2x00_alloc_fw_dump() returns early if ha->fw_dump is non-NULL, skipping
the resize.  When the dump is later captured:

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

Does this blindly copy FCE_SIZE into the undersized dump buffer, triggering an
out-of-bounds write?

[Severity: High]
This is a pre-existing issue, but there appears to be a race condition between
disabling FCE trace and capturing a firmware dump that leads to a
use-after-free.

Disabling FCE frees the DMA memory and NULLs ha->fce holding only
ha->fce_mutex:

drivers/scsi/qla2xxx/qla_dfs.c:qla2x00_dfs_fce_write() {
    ...
    qla2x00_free_fce_trace(ha);
    ...
}

However, a concurrent firmware dump executes under ha->hardware_lock but not
ha->fce_mutex.  If the debugfs thread frees the trace buffer between the
firmware dump's NULL check and the subsequent memcpy():

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

Could this result in a use-after-free read and data leak?

[Severity: High]
This is a pre-existing issue, but does qla2x00_dfs_fce_release() have a TOCTOU
race causing a NULL pointer dereference?

The guard condition is checked outside the critical section:

drivers/scsi/qla2xxx/qla_dfs.c:qla2x00_dfs_fce_release() {
    ...
    if (ha->flags.fce_enabled || !ha->fce)
        goto out;

    mutex_lock(&ha->fce_mutex);
    ...
}

If a concurrent thread writes 0 to the FCE node, it acquires the mutex,
frees ha->fce, and sets it to NULL.  When the first thread resumes, it
acquires the mutex and executes:

drivers/scsi/qla2xxx/qla_dfs.c:qla2x00_dfs_fce_release() {
    ...
    memset(ha->fce, 0, fce_calc_size(ha->fce_bufs));
    ...
}

Will this panic the kernel by dereferencing the newly NULL'd pointer?

[Severity: High]
This isn't a bug introduced by this patch, but does modifying user_enabled_fce
cause a data race with hardware interrupt context?

In qla2x00_dfs_fce_write():

drivers/scsi/qla2xxx/qla_dfs.c:qla2x00_dfs_fce_write() {
    ...
    ha->flags.user_enabled_fce = 1;
    ...
}

This executes in process context holding only fce_mutex.  However, it shares
the same 32-bit word as mbox_int:

drivers/scsi/qla2xxx/qla_def.h:
    volatile struct {
        uint32_t    mbox_int            :1;
        ...
        uint32_t    user_enabled_fce    :1;
        ...

The compiler updates these via non-atomic read-modify-write instructions.
If the ISR sets ha->flags.mbox_int = 1 under hardware_lock while the process
context is in the middle of its RMW cycle, won't the process context overwrite
the ISR's update, clearing mbox_int and causing dropped mailbox interrupts?

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index bd0684d97fef1..ae9bf68710793 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -3733,7 +3733,7 @@ int qla2x00_alloc_fce_trace(scsi_qla_host_t *vha)
>  		return -EINVAL;
>  
>  	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))
>  		return -EINVAL;
>  
>  	if (ha->fce) {

[Severity: High]
When dynamically creating multi-queue qpairs for a 29xx adapter, target paths
can fall back to calling qla2x00_start_iocbs() if the inline IOCB submission
optimization is missed:

drivers/scsi/qla2xxx/qla_init.c:qla2xxx_create_qpair() {
    ...
    if (IS_QLA27XX(ha) || IS_QLA83XX(ha) || IS_QLA28XX(ha))
        qpair->reqq_start_iocbs = qla_83xx_start_iocbs;
    ...
}

Because 29xx adapters use 128-byte IOCBs and track the ring slot using
req->ring_ext_ptr, qla2x00_start_iocbs() fails to advance ring_ext_ptr.

Does the lack of IS_QLA29XX(ha) here cause the next request to blindly overwrite
the same ring slot, corrupting the hardware request queue?

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