netdev - Re: [PATCH net-next v2 4/8] udp: never accept GSO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Mar 2021 15:29:53 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Alexander Lobakin <alobakin@...me>
Subject: Re: [PATCH net-next v2 4/8] udp: never accept GSO_FRAGLIST packets

On Mon, 2021-03-29 at 08:31 -0400, Willem de Bruijn wrote:
> On Mon, Mar 29, 2021 at 4:14 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > On Fri, 2021-03-26 at 14:15 -0400, Willem de Bruijn wrote:
> > > On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni <pabeni@...hat.com> wrote:
> > > > Currently the UDP protocol delivers GSO_FRAGLIST packets to
> > > > the sockets without the expected segmentation.
> > > > 
> > > > This change addresses the issue introducing and maintaining
> > > > a couple of new fields to explicitly accept SKB_GSO_UDP_L4
> > > > or GSO_FRAGLIST packets. Additionally updates  udp_unexpected_gso()
> > > > accordingly.
> > > > 
> > > > UDP sockets enabling UDP_GRO stil keep accept_udp_fraglist
> > > > zeroed.
> > > > 
> > > > v1 -> v2:
> > > >  - use 2 bits instead of a whole GSO bitmask (Willem)
> > > > 
> > > > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> > > > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > > 
> > > This looks good to me in principle, thanks for the revision.
> > > 
> > > I hadn't fully appreciated that gro_enabled implies accept_udp_l4, but
> > > not necessarily vice versa.
> > > 
> > > It is equivalent to (accept_udp_l4 && !up->gro_receive), right?
> > 
> > In this series, yes.
> > 
> > > Could the extra bit be avoided with
> > > 
> > > "
> > > +      /* Prefer fraglist GRO unless target is a socket with UDP_GRO,
> > > +       * which requires all but last segments to be of same gso_size,
> > > passed in cmsg */
> > >         if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> > > -                NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
> > > +               NAPI_GRO_CB(skb)->is_flist = sk ?
> > > (!udp_sk(sk)->gro_enabled || udp_sk(sk)->accept_udp_fraglist) : 1;
> > 
> > This is not ovious at all to me.
> > 
> > > +     /* Apply transport layer GRO if forwarding is enabled or the
> > > flow lands at a local socket */
> > >        if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||
> > >             (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||
> > > NAPI_GRO_CB(skb)->is_flist) {
> > >                 pp = call_gro_receive(udp_gro_receive_segment, head, skb);
> > >                 return pp;
> > >         }
> > > 
> > > +      /* Continue with tunnel GRO */
> > > "
> > > 
> > > .. not that the extra bit matters a lot. And these two conditions with
> > > gro_enabled are not very obvious.
> > > 
> > > Just a thought.
> > 
> > Overall looks more complex to me. I would keep the extra bit, unless
> > you have strong opinion.
> 
> Sounds good.
> 
> > Side note: I was wondering about a follow-up to simplify the condition:
> > 
> >         if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||
> >              (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) || NAPI_GRO_CB(skb)->is_flist) {
> > 
> > Since UDP sockets could process (segmenting as needed) unexpected GSO
> > packets, we could always do 'NETIF_F_GRO_UDP_FWD', when enabled on the
> > device level. The above becomes:
> > 
> >         if (skb->dev->features & NETIF_F_GRO_UDP_FWD) ||
> >             (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||
> >             NAPI_GRO_CB(skb)->is_flist) {
> > 
> > which is hopefully more clear (and simpler). As said, non for this
> > series anyhow.
> 
> UDP sockets can segment, but it is expensive. In this case I think the
> simplification is not worth the possible regression.

No strong opinion here, I will not do the thing mentioned above.

Thanks!

Paolo