Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN

[Eric Dumazet]

On Mon, Sep 9, 2024 at 7:07 PM Sean Anderson <sean.anderson@xxxxxxxxx> wrote:
>
> On 9/9/24 12:46, Eric Dumazet wrote:
> > On Mon, Sep 9, 2024 at 6:06 PM Sean Anderson <sean.anderson@xxxxxxxxx> wrote:
> >>
> >> When sending packets under 60 bytes, up to three bytes of the buffer following
> >> the data may be leaked. Avoid this by extending all packets to ETH_ZLEN,
> >> ensuring nothing is leaked in the padding. This bug can be reproduced by
> >> running
> >>
> >>         $ ping -s 11 destination
> >>
> >> Fixes: 9ad1a3749333 ("dpaa_eth: add support for DPAA Ethernet")
> >> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
> >> ---
> >>
> >>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> >> index cfe6b57b1da0..e4e8ee8b7356 100644
> >> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> >> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> >> @@ -2322,6 +2322,12 @@ dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev)
> >>         }
> >>  #endif
> >>
> >> +       /* Packet data is always read as 32-bit words, so zero out any part of
> >> +        * the skb which might be sent if we have to pad the packet
> >> +        */
> >> +       if (__skb_put_padto(skb, ETH_ZLEN, false))
> >> +               goto enomem;
> >> +
> >
> > This call might linearize the packet.
> >
> > @nonlinear variable might be wrong after this point.
> >
> >>         if (nonlinear) {
> >>                 /* Just create a S/G fd based on the skb */
> >>                 err = skb_to_sg_fd(priv, skb, &fd);
> >> --
> >> 2.35.1.1320.gc452695387.dirty
> >>
> >
> > Perhaps this instead ?
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index cfe6b57b1da0e45613ac1bbf32ddd6ace329f4fd..5763d2f1bf8dd31b80fda0681361514dad1dc307
> > 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2272,12 +2272,12 @@ static netdev_tx_t
> >  dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev)
> >  {
> >         const int queue_mapping = skb_get_queue_mapping(skb);
> > -       bool nonlinear = skb_is_nonlinear(skb);
> >         struct rtnl_link_stats64 *percpu_stats;
> >         struct dpaa_percpu_priv *percpu_priv;
> >         struct netdev_queue *txq;
> >         struct dpaa_priv *priv;
> >         struct qm_fd fd;
> > +       bool nonlinear;
> >         int offset = 0;
> >         int err = 0;
> >
> > @@ -2287,6 +2287,10 @@ dpaa_start_xmit(struct sk_buff *skb, struct
> > net_device *net_dev)
> >
> >         qm_fd_clear_fd(&fd);
> >
> > +       if (__skb_put_padto(skb, ETH_ZLEN, false))
> > +               goto enomem;
> > +
> > +       nonlinear = skb_is_nonlinear(skb);
> >         if (!nonlinear) {
> >                 /* We're going to store the skb backpointer at the beginning
> >                  * of the data buffer, so we need a privately owned skb
>
> Thanks for the suggestion; I was having a hard time figuring out where
> to call this.
>
> Do you have any hints for how to test this for correctness? I'm not sure
> how to generate a non-linear packet under 60 bytes.

I think pktgen can do this, with its frags parameter.