Re: [PATCH] scsi: qedf: Fix memory leak in __qedf_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Error masking in `qedf_prepare_sb()` leads to potential uninitialized hardware queue access.
- [High] Missing NULL pointer checks for `alloc_workqueue()` return values.
- [High] Unbalanced `scsi_host_put()` on the recovery mode error path.
- [High] Stack Use-After-Free and resource leak involving `simd_handler_config`.
- [High] Missing little-endian conversions when populating physical DMA addresses.
--

commit 154b38f89b94af6b83ab002c8ef9ed82528ea0a5
Author: Dawei Feng <dawei.feng@seu.edu.cn>
scsi: qedf: Fix memory leak in __qedf_probe()

This commit addresses a memory leak in __qedf_probe() by routing cleanup
paths through qedf_free_fcoe_pf_param() after a partial allocation failure.
It clears freed PF queue pointers so probe-level unwinds can safely share
the same cleanup.

> diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
> index da429b3a42838..3a41c0210336e 100644
> --- a/drivers/scsi/qedf/qedf_main.c
> +++ b/drivers/scsi/qedf/qedf_main.c
[ ... ]
> @@ -3469,7 +3481,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
>  	if (rc) {
>  
>  		QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
> -		goto err2;
> +		goto err2_free_pf;
>  	}

[Severity: High]
This is a pre-existing issue, but is it possible that qedf_prepare_sb()
unconditionally returns 0 on error?

Looking at qedf_prepare_sb():

drivers/scsi/qedf/qedf_main.c:qedf_prepare_sb() {
    ...
    if (allocation_fails)
        goto err;
    ...
err:
    return 0;
}

If status block allocation or initialization fails, it seems qedf_prepare_sb()
jumps to err and unconditionally returns 0 (success). __qedf_probe() will
then proceed as if the queues were fully initialized, potentially dereferencing
a NULL pointer or processing garbage CQEs when a fastpath interrupt fires.
Should qedf_prepare_sb() propagate the error code instead?

[ ... ]
> @@ -3713,10 +3725,11 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
>  err5:
>  	qed_ops->stop(qedf->cdev);
>  err4:
> -	qedf_free_fcoe_pf_param(qedf);
>  	qedf_sync_free_irqs(qedf);
>  err3:
>  	qed_ops->common->slowpath_stop(qedf->cdev);
> +err2_free_pf:
> +	qedf_free_fcoe_pf_param(qedf);
>  err2:
>  	qed_ops->common->remove(qedf->cdev);
>  err1:

[Severity: High]
This is a pre-existing issue, but does this error path lead to an unbalanced
scsi_host_put() during recovery mode?

In __qedf_probe(), during a normal probe, a new Scsi_Host is allocated with
a reference count of 1. If the normal probe fails, jumping to err1 drops
this reference:

err1:
    scsi_host_put(lport->host);

However, if mode == QEDF_MODE_RECOVERY, the driver reuses the existing
Scsi_Host from qedf->lport without incrementing its reference count. If the
recovery probe fails and jumps here, won't scsi_host_put() prematurely drop
a reference that was not taken, leading to a use-after-free or double-free
of the Scsi_Host?

[Severity: High]
This is a pre-existing issue, but there appear to be missing NULL pointer
checks for alloc_workqueue() in __qedf_probe().

The driver allocates link_update_wq and dpc_wq:

drivers/scsi/qedf/qedf_main.c:__qedf_probe() {
    ...
    qedf->link_update_wq = alloc_workqueue("qedf_%u_link",
                           WQ_MEM_RECLAIM | WQ_PERCPU,
                           1, qedf->lport->host->host_no);
    ...
    qedf->dpc_wq = alloc_workqueue("%s", WQ_MEM_RECLAIM | WQ_PERCPU, 1,
                    host_buf);
    ...
}

Neither of these allocations are checked for failure. Under memory pressure,
if alloc_workqueue() returns NULL, wouldn't passing a NULL workqueue to
queue_work() or queue_delayed_work() trigger an immediate kernel panic?

[Severity: High]
This is a pre-existing issue, but is there a stack use-after-free risk when
configuring the SIMD interrupt handler?

In qedf_setup_int(), the driver passes the address of the local stack
variable qedf (&qedf) to simd_handler_config():

drivers/scsi/qedf/qedf_main.c:qedf_setup_int() {
    ...
    qed_ops->common->simd_handler_config(qedf->cdev, &qedf,
        QEDF_SIMD_HANDLER_NUM, qedf_simd_int_handler);
    ...
    if (qedf->int_info.msix_cnt == 0) {
        QEDF_ERR(&qedf->dbg_ctx,
             "Cannot load driver due to a lack of MSI-X vectors.\n");
        return -EINVAL;
    }
    ...
}

The registered handler then casts the cookie directly to the struct:

drivers/scsi/qedf/qedf_main.c:qedf_simd_int_handler() {
    struct qedf_ctx *qedf = (struct qedf_ctx *)cookie;
    QEDF_WARN(&(qedf->dbg_ctx), "qedf=%p.\n", qedf);
    ...
}

Since qedf_setup_int() passes a stack pointer as the cookie and then
immediately returns -EINVAL if msix_cnt == 0, __qedf_probe() will jump to
err3 and bypass qedf_sync_free_irqs(). This leaves the SIMD handler
permanently registered with a pointer to dead stack memory. If an interrupt
fires after the probe fails, will this dereference corrupted stack memory?

[Severity: High]
This is a pre-existing issue, but does qedf_alloc_global_queues() incorrectly
write physical addresses using native endianness?

When populating the physical block lists (PBL) for the completion queues and
global queues, the driver writes physical addresses using native assignments:

drivers/scsi/qedf/qedf_main.c:qedf_alloc_global_queues() {
    ...
    *pbl = U64_LO(page);
    pbl++;
    *pbl = U64_HI(page);
    ...
    *list = U64_LO(qedf->global_queues[i]->cq_pbl_dma);
    list++;
    *list = U64_HI(qedf->global_queues[i]->cq_pbl_dma);
    ...
}

Hardware and firmware typically expect these DMA pointers in little-endian
format. On big-endian architectures, won't these native writes corrupt the
DMA pointers and cause the hardware to DMA into arbitrary memory locations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616135513.3982797-1-dawei.feng@seu.edu.cn?part=1