Re: [PATCH net-next v6 10/14] net: lan966x: add PCIe FDMA support

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 3
> 
> - [Medium] DISCARDED — fixed later in the series.
> - [Low] DISCARDED — fixed later in the series.
> - [Low] DISCARDED — fixed later in the series.
> - [Low] DISCARDED — fixed later in the series.
> 
> Pre-existing issues:
> - [High] Use-after-free in the new PCIe receive path:
>   `lan966x_fdma_pci_rx_get_frame()`…
> - [Medium] DISCARDED — pre-existing shared-library ordering weakness.
> 
> --
> 
> net: lan966x: add PCIe FDMA support
> 
> Adds a second FDMA backend for lan966x used when the switch is reached over
> PCIe, using contiguous ATU-mapped DMA buffers and memcpy-based frame
> transfer instead of per-page mappings. XDP and PTP are not advertised or
> allowed on this path.
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> > new file mode 100644
> > index 000000000000..f1f3c789d3a6
> > --- /dev/null
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> > @@ -0,0 +1,404 @@
> 
> [ ... ]
> 
> > +/* Return true if blockl is a valid RX frame size. */
> > +static bool lan966x_fdma_pci_rx_size_fits(struct fdma *fdma, u32 blockl)
> > +{
> > +     return blockl >= IFH_LEN_BYTES + ETH_HLEN + ETH_FCS_LEN &&
> > +            blockl <= fdma->db_size - XDP_PACKET_HEADROOM;
> > +}
> 
> [ ... ]
> 
> > +     skb->protocol = eth_type_trans(skb, skb->dev);
> > +
> > +     if (lan966x->bridge_mask & BIT(src_port)) {
> > +             skb->offload_fwd_mark = 1;
> > +
> > +             skb_reset_network_header(skb);
> > +             if (!lan966x_hw_offload(lan966x, src_port, skb))
> > +                     skb->offload_fwd_mark = 0;
> > +     }
> > +
> > +     skb->dev->stats.rx_bytes += skb->len;
> > +     skb->dev->stats.rx_packets++;
> > +
> > +     return skb;
> > +}
> 
> [Severity: High]
> This isn't a bug introduced by this patch - lan966x_fdma_rx_get_frame() in
> lan966x_fdma.c has the same caller pattern - but can lan966x_fdma_pci_rx_get_frame()
> touch skb after lan966x_hw_offload() has already freed it?

Yes, but pre-existing. I think this deserves a proper fix through net and
updates for all callers.

> 
> lan966x_hw_offload() replaces the buffer and reports only a boolean:
> 
> drivers/net/ethernet/microchip/lan966x/lan966x_main.c:lan966x_hw_offload() {
>         ...
>         if (eth_type_vlan(skb->protocol)) {
>                 skb = skb_vlan_untag(skb);
>                 if (unlikely(!skb))
>                         return false;
>         }
>         ...
> }
> 
> and skb_vlan_untag() frees the caller's skb on failure:
> 
> net/core/skbuff.c:skb_vlan_untag() {
>         ...
>         if (unlikely(!pskb_may_pull(skb, VLAN_HLEN + sizeof(unsigned short))))
>                 goto err_free;
>         ...
> err_free:
>         kfree_skb(skb);
>         return NULL;
> }
> 
> So on the false return the caller's pointer is dangling, and
> lan966x_fdma_pci_rx_get_frame() then does:
> 
>         skb->offload_fwd_mark = 0;
>         skb->dev->stats.rx_bytes += skb->len;
>         skb->dev->stats.rx_packets++;
>         return skb;
> 
> with the returned skb going to napi_gro_receive() in
> lan966x_fdma_pci_napi_poll().
> 
> Is the new lower bound in lan966x_fdma_pci_rx_size_fits() wide enough to let
> this happen? IFH_LEN_BYTES + ETH_HLEN + ETH_FCS_LEN is 46, so a BLOCKL of
> 46..51 is accepted, and after skb_pull(IFH_LEN_BYTES),
> skb_trim(skb->len - ETH_FCS_LEN) and eth_type_trans() there are only 0..5
> bytes left. For a frame with h_proto 0x8100 on a bridged port that has IGMP
> or MLD redirection enabled, the pskb_may_pull(VLAN_HLEN + 2) inside
> skb_vlan_untag() then fails and the buffer is freed.
> 
> Would it make sense to have lan966x_hw_offload() take a struct sk_buff **,
> or to have the caller stop using skb once it returns false?
> 
> >  static int lan966x_fdma_pci_xmit(struct sk_buff *skb, __be32 *ifh,
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671%40microchip.com