Re: [PATCH v2 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails

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

New issues:
- [Medium] The patch adds cleanup code to a mathematically unreachable error path, completely failing to address the actual root cause of the reqtag exhaustion.

Pre-existing issues:
- [Critical] Unconditional freeing of a reused sequence buffer (`seq`) leads to a catastrophic Use-After-Free, Double-Free, and hardware DMA corruption when sending a task set full/busy response.
- [High] Guaranteed NULL pointer dereference when sending a task set full/busy response.
- [High] `efct_hw_reqtag_free()` detects a double-free but incorrectly proceeds to corrupt the freelist.
- [High] `efct_hw_init()` fails to handle `efct_hw_io_alloc()` failure, leading to a NULL pointer dereference.
--

--- Patch [3]: [PATCH v2 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails ---
commit 415b71fb97ba10c30ffb204dfd40acdb37bb24d1
Author: Ali Ahmet Memis <[email protected]>

scsi: elx: efct: free the request tag when the send frame WQE fails

efct_hw_send_frame() takes a request tag from the pool and then builds the
WQE. When sli_send_frame_wqe() fails it returns without giving the tag
back.

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index 6cc48fa3e656a..db68516e80750 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c
> @@ -3009,6 +3009,7 @@ efct_hw_send_frame(struct efct_hw *hw, struct fc_frame_header *hdr,
>  				ctx->wqcb->instance_index);
>  	if (rc) {
>  		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
> +		efct_hw_reqtag_free(hw, ctx->wqcb);
>  		return -EIO;
>  	}

[Severity: Medium]
Does this error path actually ever execute?

Looking at sli_send_frame_wqe(), it appears to unconditionally return 0:

libefc_sli/sli4.c:sli_send_frame_wqe() {
    ...
    return 0;
}

Since sli_send_frame_wqe() always returns 0, this error handling block seems
unreachable. Is there a different root cause for the reqtag exhaustion?


While reviewing this change, I also noticed a few other areas in the driver
that might need attention.

[Severity: Critical]
This isn't a bug introduced by this patch, but does efct_dispatch_frame()
unconditionally free a reused sequence buffer, causing a use-after-free?

When efct_scsi_io_alloc() fails, the driver sends a task set full/busy
response via efct_sframe_common_send(). It allocates the async transmission
context directly inside the incoming sequence's DMA buffer:

efct_unsol.c:efct_sframe_common_send() {
    ctx = (void *)(heap_virt_base + heap_offset);
    ...
    ctx->seq = seq;
}

However, efct_dispatch_frame() ignores the return value of
efct_dispatch_fcp_cmd() and unconditionally frees the sequence:

efct_unsol.c:efct_dispatch_frame() {
    kref_put(&node->ref, node->release);
    efct_hw_sequence_free(&efct->hw, seq);
    return 0;
}

When transmission completes, efct_sframe_common_send_cb() double-frees the
sequence:

efct_unsol.c:efct_sframe_common_send_cb() {
    /* Free sequence */
    efct_hw_sequence_free(hw, ctx->seq);
}

Could this cause memory corruption and hardware DMA issues if the buffer is
returned to the hardware receive ring before the async send completes?


[Severity: High]
This is a pre-existing issue, but does sending a task set full/busy response
result in a NULL pointer dereference?

In efct_sframe_common_send(), the context is zeroed out, leaving wqebuf as
NULL:

efct_unsol.c:efct_sframe_common_send() {
    memset(ctx, 0, sizeof(*ctx));
    ...
}

This uninitialized context is passed to efct_hw_send_frame(), which passes
the NULL wqe->wqebuf to sli_send_frame_wqe():

efct_hw.c:efct_hw_send_frame() {
    rc = sli_send_frame_wqe(&hw->sli, wqe->wqebuf, ...);
}

sli_send_frame_wqe() immediately calls memset() on this NULL buffer:

libefc_sli/sli4.c:sli_send_frame_wqe() {
    memset(buf, 0, sli->wqe_size);
}

Will this guarantee a kernel panic when triggered?


[Severity: High]
This isn't a bug introduced by this patch, but does efct_hw_reqtag_free()
corrupt the freelist if it detects a double-free?

If a tag is already freed, the function logs an error but doesn't return
early:

efct_hw.c:efct_hw_reqtag_free() {
    if (!wqcb->callback)
        efc_log_err(hw->os, "WQCB is already freed\n");

    spin_lock_irqsave(&reqtag_pool->lock, flags);
    wqcb->callback = NULL;
    ...
    INIT_LIST_HEAD(&wqcb->list_entry);
    list_add(&wqcb->list_entry, &hw->wq_reqtag_pool->freelist);
    spin_unlock_irqrestore(&reqtag_pool->lock, flags);
}

If the tag is already in the freelist, doesn't continuing with
INIT_LIST_HEAD() and list_add() completely corrupt the list pointers?


[Severity: High]
This is a pre-existing issue, but does an allocation failure in
efct_hw_init() lead to a NULL pointer dereference later?

If efct_hw_io_alloc() fails, it logs an error but returns success:

efct_hw.c:efct_hw_init() {
    hw->hw_wq[0]->send_frame_io = efct_hw_io_alloc(hw);
    if (!hw->hw_wq[0]->send_frame_io)
        efc_log_err(hw->os, "alloc for send_frame_io failed\n");

    /* Initialize send frame sequence id */
    atomic_set(&hw->send_frame_seq_id, 0);

    return 0;
}

Later, efct_hw_send_frame() unconditionally dereferences it:

efct_hw.c:efct_hw_send_frame() {
    xri = wq->send_frame_io->indicator;
}

Should efct_hw_init() return an error code instead of 0 when this
allocation fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3