Re: [PATCH net-next v3 09/13] net: lan966x: add PCIe FDMA support

[Daniel Machon]

> On 5/4/26 4:23 PM, Daniel Machon wrote:
> > +static int lan966x_fdma_pci_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
> > +{
> > +     struct lan966x *lan966x = rx->lan966x;
> > +     struct fdma *fdma = &rx->fdma;
> > +     struct lan966x_port *port;
> > +     struct fdma_db *db;
> > +     void *virt_addr;
> > +     u32 blockl;
> > +
> > +     /* virt_addr points to the IFH. */
> > +     virt_addr = fdma_dataptr_virt_addr_contiguous(fdma,
> > +                                                   fdma->dcb_index,
> > +                                                   fdma->db_index);
> > +
> > +     lan966x_ifh_get_src_port(virt_addr, src_port);
> > +
> > +     if (WARN_ON(*src_port >= lan966x->num_phys_ports))
> > +             return FDMA_ERROR;
> > +
> > +     port = lan966x->ports[*src_port];
> > +     if (!port)
> > +             return FDMA_ERROR;
> > +
> > +     db = fdma_db_next_get(fdma);
> > +
> > +     /* BLOCKL is a 16-bit HW-populated field; reject obviously-bad
> > +      * values before they feed memcpy/XDP sizes.
> > +      */
> > +     blockl = FDMA_DCB_STATUS_BLOCKL(db->status);
> > +     if (blockl < IFH_LEN_BYTES + ETH_FCS_LEN || blockl > fdma->db_size)
> > +             return FDMA_ERROR;
> 
> Pre-existing issues reported by sashiko (most of them actually) can be
> safely ignored/postponed to follow-ups, but the above OOB (and in patch
> 11/13) access looks real and IMHO should be addressed.
> 
> /P
>

This one looks right. The check ought to be: blockl > fdma->db_size -
XDP_PACKET_HEADROOM.

For patch #11, which issue are you referring to? If its the sashiko-gemini
critical issue:

  > +xdp_init_buff(&xdp, fdma->db_size, &port->xdp_rxq);
  >    +
  > +/* Headroom includes the IFH; BPF may grow into it via adjust_he ad.
  > + * The IFH is rebuilt on XDP_TX and unread on XDP_PASS.
  > + */      
  > +xdp_prepare_buff(&xdp,
  > + data - XDP_PACKET_HEADROOM,
  > + XDP_PACKET_HEADROOM + IFH_LEN_BYTES,
  > + data_len,
  > + false);   

Then no, this is a false-positive. The data pointer is already offset by
XDP_PACKET_HEADROOM, so the hard_start lands correctly at offset 0.

/Daniel