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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Use-After-Free and Double-Free of receive sequence buffer during task set full/busy response.
- [Critical] Deterministic NULL pointer dereference passing uninitialized `wqebuf` to `sli_send_frame_wqe()`.
- [High] NULL pointer dereference in `efct_hw_send_frame()` if `send_frame_io` allocation fails.
- [High] Missing `dma_rmb()` memory barrier after checking the valid bit of the EQE and CQE.
- [High] Incorrect byte swapping of the FC frame header on Big-Endian architectures.
--

--- Patch [3]: [PATCH 3/5] scsi: elx: efct: free the request tag when the send frame WQE fails ---
commit 66a0b41398fa81e3a9b1b1edf2e7a32743d6bf86
Author: Ali Ahmet Memis <ali@iusegentoo.com>

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,

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference 
if send_frame_io allocation fails?

During driver initialization, if efct_hw_io_alloc() fails, it logs an error 
but completes successfully:

drivers/scsi/elx/efct/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() directly dereferences it without checking for NULL 
just before creating the WQE:

drivers/scsi/elx/efct/efct_hw.c:efct_hw_send_frame() {
    ...
    /* Set XRI and RX_ID in the header based on which WQ, and which
     * send_frame_io we are using
     */
    xri = wq->send_frame_io->indicator;
    ...
}

Should initialization fail if this allocation fails?

[Severity: Critical]
This is a pre-existing issue, but does this lead to a NULL pointer dereference 
when attempting to send a task set full or busy response?

In efct_sframe_common_send(), the context is zero-initialized:

drivers/scsi/elx/efct/efct_unsol.c:efct_sframe_common_send() {
    ...
    memset(ctx, 0, sizeof(*ctx));
    /* Save sequence */
    ctx->seq = seq;
    ...
}

This leaves the embedded ctx->wqe.wqebuf pointer as NULL. When 
efct_hw_send_frame() is called, it passes this uninitialized wqebuf to 
sli_send_frame_wqe():

drivers/scsi/elx/efct/efct_hw.c:efct_hw_send_frame() {
    ...
    rc = sli_send_frame_wqe(&hw->sli, wqe->wqebuf,
                            sof, eof, (u32 *)hdr, payload, payload->len,
    ...
}

Then sli_send_frame_wqe() immediately attempts to memset the buffer:

drivers/scsi/elx/libefc_sli/sli4.c:sli_send_frame_wqe() {
    ...
    struct sli4_send_frame_wqe *sf = buf;

    memset(buf, 0, sli->wqe_size);
    ...
}

Will this cause a guaranteed kernel panic on command flooding when IO contexts 
are exhausted?

[Severity: Critical]
This is a pre-existing issue, but can this sequence lead to a use-after-free 
and double-free of the receive sequence buffer?

When efct_scsi_io_alloc() fails, it rejects the command via 
efct_sframe_send_task_set_full_or_busy(). This uses the receive sequence's 
payload buffer to build the transmission payload and context:

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

After queuing the transmit with efct_hw_send_frame(), control returns to 
efct_dispatch_frame() which unconditionally frees the sequence, returning it to 
the hardware receive ring:

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

This appears to return the buffer to the hardware receive queue while the 
hardware is simultaneously trying to DMA transmit from it. 

Furthermore, when the asynchronous transmit completes, it frees the exact same 
sequence a second time:

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

Is it safe to free the sequence in efct_dispatch_frame() when it has been 
repurposed for transmission?

[Severity: High]
This is a pre-existing issue, but does this incorrectly byte-swap the FC frame 
header on Big-Endian architectures?

In sli_send_frame_wqe(), the frame header bytes are cast to u32 and swapped:

drivers/scsi/elx/libefc_sli/sli4.c:sli_send_frame_wqe() {
    ...
    /* Copy FC header */
    sf->fc_header_0_1[0] = cpu_to_le32(hdr[0]);
    sf->fc_header_0_1[1] = cpu_to_le32(hdr[1]);
    sf->fc_header_2_5[0] = cpu_to_le32(hdr[2]);
    sf->fc_header_2_5[1] = cpu_to_le32(hdr[3]);
    sf->fc_header_2_5[2] = cpu_to_le32(hdr[4]);
    sf->fc_header_2_5[3] = cpu_to_le32(hdr[5]);
    ...
}

On Big-Endian architectures, reading a byte array via a 32-bit load reverses 
the 4 bytes, and cpu_to_le32() applies a second swap. When stored to memory, 
the Big-Endian store behavior reverses it again. Will this result in the 
hardware receiving a byte-reversed FC frame header?

[Severity: High]
This is a pre-existing issue, but is a read memory barrier required after 
checking the valid bit of the DMA-coherent queue entry?

In sli_eq_read(), the valid bit is checked and then the entry is copied:

drivers/scsi/elx/libefc_sli/sli4.c:sli_eq_read() {
    ...
    wflags = le16_to_cpu(((struct sli4_eqe *)qe)->dw0w0_flags);

    if ((wflags & SLI4_EQE_VALID) != q->phase) {
        spin_unlock_irqrestore(&q->lock, flags);
        return -EIO;
    }
    ...
    memcpy(entry, qe, q->size);
    ...
}

A similar pattern exists in sli_cq_read():

drivers/scsi/elx/libefc_sli/sli4.c:sli_cq_read() {
    ...
    dwflags = le32_to_cpu(((struct sli4_mcqe *)qe)->dw3_flags);
    valid_bit_set = (dwflags & SLI4_MCQE_VALID) != 0;

    if (valid_bit_set != q->phase) {
        spin_unlock_irqrestore(&q->lock, flags);
        return -EIO;
    }
    ...
    memcpy(entry, qe, q->size);
    ...
}

On weakly ordered architectures, could the CPU speculatively read the entry's 
body before the valid bit check completes over the bus, leading to the 
processing of stale or corrupted DMA descriptor data? Does this require a 
dma_rmb() before the memcpy()?

>  				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;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806192345.328621-1-ali@iusegentoo.com?part=3