Re: [PATCH RFC 2/2] can: mcp251xfd: mcp251xfd_irq(): add call to can_rx_offload_threaded_irq_flush()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] can: mcp251xfd: crash in __skb_cut_position() due to out-of-order timestamps
--

commit b3d8ad3e6e4f0483e24aef1e8fa597af3b315e6a
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: mcp251xfd: mcp251xfd_irq(): add call to can_rx_offload_threaded_irq_flush()

Under certain load situations, the threaded IRQ handler will not exit
the while loop, resulting in unlimited growth of the skb_irq_queue.
This commit adds a call into the newly added function
can_rx_offload_threaded_irq_flush() to flush parts of the queue.

> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index f441f2265299e..208595c0cc469 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -1615,6 +1615,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id)
>  			}
>  		}
>  
> +		can_rx_offload_threaded_irq_flush(&priv->offload);

[Severity: Critical]
Does this new loop expose a regression in the flushing logic when dealing with
out-of-order timestamps?

The mcp251xfd_irq threaded handler iterates over multiple RX FIFOs
sequentially. Packets from different FIFOs can overlap chronologically,
meaning a newly read packet from FIFO 1 may have an older timestamp than
a previously read packet from FIFO 0.

When can_rx_offload_queue_timestamp() is called, __skb_queue_add_sort()
inserts this older packet before the newer one in the skb_irq_queue:

drivers/net/can/dev/rx-offload.c:__skb_queue_add_sort() {
    ...
    __skb_queue_after(head, insert, new);
    ...
}

However, can_rx_offload_threaded_irq_flush() caches flush_skb and flush_len
as a snapshot:

drivers/net/can/dev/rx-offload.c:can_rx_offload_threaded_irq_flush() {
    ...
    if (!offload->flush_skb) {
        offload->flush_skb = skb_peek_tail(&offload->skb_irq_queue);
        offload->flush_len = irq_queue_len;
    }
    ...
}

If an older packet is subsequently inserted before flush_skb, the physical
number of elements from the head to flush_skb will exceed the cached
flush_len.

When __skb_cut_position() slices the queue, it uses the stale flush_len:

drivers/net/can/dev/rx-offload.c:__skb_cut_position() {
    ...
    list->qlen = qlen;

    WRITE_ONCE(head->next, new_first);
    WRITE_ONCE(new_first->prev, (struct sk_buff *)head);
    head->qlen -= qlen;
    ...
}

This leaves skb_irq_queue.qlen under-decremented. Over multiple iterations
of this new while loop, qlen grows artificially large.

Eventually, irq_queue_len exceeds the flush threshold even when the queue is
physically empty, causing skb_peek_tail() to return NULL for flush_skb.
__skb_cut_position() then dereferences entry->next where entry is NULL:

drivers/net/can/dev/rx-offload.c:__skb_cut_position() {
    ...
    struct sk_buff *new_first = entry->next;
    ...
}

Can a remote attacker trigger this kernel panic by flooding the CAN bus
with traffic to distribute packets across multiple FIFOs, creating the
out-of-order timestamp scenario?

>  		handled = IRQ_HANDLED;
>  	} while (1);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-upstream-can-rx-offload-batching-alternative-v1-0-4c08ccb516ec@pengutronix.de?part=2