Re: [PATCH net-next v2 11/13] net: lan966x: add PCIe FDMA XDP support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) race condition in XDP program execution causes NULL pointer dereference.
- [Critical] Double addition of `XDP_PACKET_HEADROOM` misaligns DMA/virtual pointers, causing buffer overruns and memory corruption.
- [High] Hardware FCS is incorrectly exposed to XDP payload and causes data truncation on XDP_PASS.
- [High] Integer underflow on runt frames leads to out-of-bounds memory accesses.
- [High] Discrepancy in FCS length handling between TX paths causes information leak or payload corruption.
- [Medium] `lan966x_fdma_pci_xmit_xdpf()` fails to pad short frames to minimum Ethernet length.
--

commit 99263814b4c5264a0eb9e0faa0f7f6b6f10d121f
Author: Daniel Machon <daniel.machon@microchip.com>
Subject: net: lan966x: add PCIe FDMA XDP support

Add XDP support for the PCIe FDMA path. The implementation operates on
contiguous ATU-mapped buffers with memcpy-based XDP_TX, unlike the
platform path which uses page_pool.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> index 61a62dbcc313..8d867b7edd39 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
[ ... ]
> @@ -15,7 +17,8 @@ static int lan966x_fdma_pci_dataptr_cb(struct fdma *fdma, int dcb, int db,
>  
>  	addr = fdma_dataptr_dma_addr_contiguous(fdma, dcb, db);
>  
> -	*dataptr = fdma_pci_atu_translate_addr(fdma->atu_region, addr);
> +	*dataptr = fdma_pci_atu_translate_addr(fdma->atu_region, addr) +
> +		   XDP_PACKET_HEADROOM;

Is it possible that XDP_PACKET_HEADROOM is being added twice here? Looking at
fdma_dataptr_dma_addr_contiguous(), the headroom is already implicitly added:

fdma_api.h:fdma_dataptr_dma_addr_contiguous() {
    return fdma->dma + ... + XDP_PACKET_HEADROOM;
}

Could this double addition push the pointers past the allocated db_size
boundary for MTU-sized packets and corrupt adjacent memory?

[ ... ]
> +static int lan966x_fdma_pci_xmit_xdpf(struct lan966x_port *port,
> +				      void *ptr, u32 len)
> +{
[ ... ]
> +	virt_addr = fdma_dataptr_virt_addr_contiguous(fdma, next_to_use, 0) +
> +		    XDP_PACKET_HEADROOM;

Would this also suffer from the same double XDP_PACKET_HEADROOM addition,
since fdma_dataptr_virt_addr_contiguous() likely includes it?

[ ... ]
> +	fdma_dcb_add(fdma,
> +		     next_to_use,
> +		     0,
> +		     FDMA_DCB_STATUS_INTR |
> +		     FDMA_DCB_STATUS_SOF |
> +		     FDMA_DCB_STATUS_EOF |
> +		     FDMA_DCB_STATUS_BLOCKO(0) |
> +		     FDMA_DCB_STATUS_BLOCKL(IFH_LEN_BYTES + len));

If len from the XDP program is less than ETH_ZLEN, will the hardware drop the
runt frame or transmit it with garbage padding? The standard
lan966x_fdma_pci_xmit() path explicitly pads short frames to ETH_ZLEN.

Also, does ETH_FCS_LEN need to be included in the block length here?
lan966x_fdma_pci_xmit() includes it. If the hardware expects the MAC FCS space
to be included in the block length, could omitting it here cause the MAC to
replace the last 4 bytes of the XDP payload with the FCS?

[ ... ]
> +static int lan966x_fdma_pci_rx_check_frame(struct lan966x_rx *rx, u64 *src_port,
> +					   void **data, u32 *data_len)
>  {
[ ... ]
>  	virt_addr = fdma_dataptr_virt_addr_contiguous(fdma,
>  						      fdma->dcb_index,
>  						      fdma->db_index);
> +	ifh = virt_addr + XDP_PACKET_HEADROOM;

This looks like a third instance of double-adding XDP_PACKET_HEADROOM.

[ ... ]
> +	/* Present the Ethernet frame (no IFH); may be overridden by XDP. */
> +	*data = ifh + IFH_LEN_BYTES;
> +	*data_len = FDMA_DCB_STATUS_BLOCKL(db->status) - IFH_LEN_BYTES;

What happens if the hardware returns a runt or corrupted frame where
FDMA_DCB_STATUS_BLOCKL is less than IFH_LEN_BYTES (28)? Will this unsigned
subtraction underflow to a large value and cause out-of-bounds memory accesses
during XDP processing?

Additionally, if the block length includes the Ethernet FCS, does this pass
the FCS into the XDP program as part of the packet payload?

> +	if (!lan966x_xdp_port_present(port))
> +		return FDMA_PASS;
> +
> +	return lan966x_xdp_pci_run(port, virt_addr, *data_len, data, data_len);
>  }

Can a concurrent XDP program detach cause a NULL pointer dereference here?

lan966x_xdp_port_present() checks if port->xdp_prog is set:

lan966x_main.h:lan966x_xdp_port_present() {
    return !!port->xdp_prog;
}

If it returns true, lan966x_xdp_pci_run() reads the pointer a second time:

lan966x_fdma_pci.c:lan966x_xdp_pci_run() {
    struct bpf_prog *xdp_prog = port->xdp_prog;
    ...
    act = bpf_prog_run_xdp(xdp_prog, &xdp);
}

If lan966x_xdp_setup() detaches the program via xchg(&port->xdp_prog, NULL)
between these two reads, could xdp_prog be NULL when passed to
bpf_prog_run_xdp()? Should the pointer be read exactly once using READ_ONCE()
or rcu_dereference() to ensure it remains valid?

[ ... ]
>  static struct sk_buff *lan966x_fdma_pci_rx_get_frame(struct lan966x_rx *rx,
> -						     u64 src_port)
> +						     u64 src_port, void *data,
> +						     u32 data_len)
>  {
[ ... ]
>  	if (likely(!(skb->dev->features & NETIF_F_RXFCS)))
>  		skb_trim(skb, skb->len - ETH_FCS_LEN);

If the XDP program modifies the packet length (for instance, stripping an
encapsulation header) and returns XDP_PASS, will subtracting a hardcoded
ETH_FCS_LEN from the newly modified length blindly truncate legitimate payload
data?

[ ... ]
> @@ -180,9 +278,10 @@ static int lan966x_fdma_pci_xmit(struct sk_buff *skb, __be32 *ifh,
>  
>  	skb_tx_timestamp(skb);
>  
> -	virt_addr = fdma_dataptr_virt_addr_contiguous(fdma, next_to_use, 0);
> +	virt_addr = fdma_dataptr_virt_addr_contiguous(fdma, next_to_use, 0) +
> +		    XDP_PACKET_HEADROOM;

This appears to be a fourth instance where XDP_PACKET_HEADROOM is added on top
of a pointer that already includes it.

Also, further down in this function, FDMA_DCB_STATUS_BLOCKL includes
ETH_FCS_LEN, unlike lan966x_fdma_pci_xmit_xdpf(). If the hardware actually
appends the FCS automatically and doesn't expect ETH_FCS_LEN to be part of the
block length space, will adding it here transmit uninitialized buffer memory
over the network?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428-lan966x-pci-fdma-v2-0-d3ec66e06202@microchip.com?part=11