Re: [PATCH net 1/4] net: ti: icssg-prueth: Fix AF_XDP fill ring alloc and wakeup condition

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Integer underflow in `pkt_len -= 4` leads to massive XDP out-of-bounds read/write in Zero Copy mode and skb_over_panic in standard mode.
- [High] In `icssg_napi_rx_poll()`, when zero-copy is enabled, `num_rx` is overwritten instead of accumulated across multiple flows, and the full `budget` is passed to each flow.
--

commit c6fd345481cd54bff877bdb42096a81eabfe43d0
Author: Meghana Malladi <m-malladi@ti.com>

net: ti: icssg-prueth: Fix AF_XDP fill ring alloc and wakeup condition

This patch fixes AF_XDP fill ring allocation by introducing an allocation
budget based on available descriptors. It prevents pool exhaustion errors
and spurious XSK wakeups by avoiding allocations when the descriptor pool
is empty.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index a28a608f9bf4b..55a696912811f 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -927,6 +927,7 @@ static int emac_rx_packet_zc(struct prueth_emac *emac, u32 flow_id,
>  	struct cppi5_host_desc_t *desc_rx;
>  	struct prueth_swdata *swdata;
>  	dma_addr_t desc_dma, buf_dma;
> +	int avail_desc, alloc_budget;

[Severity: High]
This isn't a bug introduced by this patch, but could icssg_napi_rx_poll()
mishandle the budget and num_rx values when calling emac_rx_packet_zc()
for multiple flows?

In icssg_napi_rx_poll():

    if (rx_chn->xsk_pool) {
        num_rx = emac_rx_packet_zc(emac, flow, budget);
    } else {

Because num_rx is overwritten instead of accumulated across multiple flows,
and the full budget is unconditionally passed to each flow, might the driver
process up to N * budget packets per NAPI cycle? If the last flow evaluated
processes 0 packets while previous flows processed many, could NAPI
incorrectly assume no work was done and drop out of polling mode?

[Severity: Critical]
This is a pre-existing issue, but does unconditionally subtracting 4 from
pkt_len cause an integer underflow if a short descriptor is received?

In emac_rx_packet_zc():

    pkt_len = cppi5_hdesc_get_pktlen(desc_rx);
    /* firmware adds 4 CRC bytes, strip them */
    pkt_len -= 4;
    ...
    xsk_buff_set_size(xdp, pkt_len);

If the hardware provides a malformed descriptor with a length less than 4,
the unsigned subtraction could underflow, resulting in a large 32-bit value.
When passed to xsk_buff_set_size(), this sets a malformed data_end, which
could allow an XDP program to perform out-of-bounds reads or writes.

The same integer underflow exists in the standard RX path in
emac_rx_packet():

    pkt_len -= 4;
    ...
    skb_put(skb, pkt_len);

Passing the underflowed length to skb_put() might directly trigger a kernel
panic via skb_over_panic(). Should there be a bounds check before the
subtraction?

>  	struct xdp_buff *xdp;
>  	int xdp_status = 0;
>  	int count = 0;
> @@ -993,16 +994,13 @@ static int emac_rx_packet_zc(struct prueth_emac *emac, u32 flow_id,
>  	if (xdp_status & ICSSG_XDP_REDIR)
>  		xdp_do_flush();
>  
> -	/* Allocate xsk buffers from the pool for the "count" number of
> -	 * packets processed in order to be able to receive more packets.
> -	 */
> -	ret = prueth_rx_alloc_zc(emac, count);
> +	avail_desc = k3_cppi_desc_pool_avail(rx_chn->desc_pool);
> +	alloc_budget = min_t(int, budget, avail_desc);
> +
> +	ret = prueth_rx_alloc_zc(emac, alloc_budget);
>  
>  	if (xsk_uses_need_wakeup(rx_chn->xsk_pool)) {
> -		/* If the user space doesn't provide enough buffers then it must
> -		 * explicitly wake up the kernel when new buffers are available
> -		 */
> -		if (ret < count)
> +		if (ret < alloc_budget)
>  			xsk_set_rx_need_wakeup(rx_chn->xsk_pool);
>  		else
>  			xsk_clear_rx_need_wakeup(rx_chn->xsk_pool);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611185744.2498070-1-m-malladi@ti.com?part=1