netdev - Re: [RFC PATCH v2 06/10] udp: cope with UDP GRO packet misdirection

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAF=yD-KW4LqkT3+JQ2DCvrs8ZpPPHLcKzyvwVgYZm61PaED4ng@mail.gmail.com>
Date:   Mon, 22 Oct 2018 12:00:31 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        steffen.klassert@...unet.com
Subject: Re: [RFC PATCH v2 06/10] udp: cope with UDP GRO packet misdirection

On Mon, Oct 22, 2018 at 6:29 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Sun, 2018-10-21 at 16:08 -0400, Willem de Bruijn wrote:
> > On Fri, Oct 19, 2018 at 10:31 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > >
> > > In some scenarios, the GRO engine can assemble an UDP GRO packet
> > > that ultimately lands on a non GRO-enabled socket.
> > > This patch tries to address the issue explicitly checking for the UDP
> > > socket features before enqueuing the packet, and eventually segmenting
> > > the unexpected GRO packet, as needed.
> > >
> > > We must also cope with re-insertion requests: after segmentation the
> > > UDP code calls the helper introduced by the previous patches, as needed.
> > >
> > > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > > ---
> > > +static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
> > > +                                             struct sk_buff *skb)
> > > +{
> > > +       struct sk_buff *segs;
> > > +
> > > +       /* the GSO CB lays after the UDP one, no need to save and restore any
> > > +        * CB fragment, just initialize it
> > > +        */
> > > +       segs = __skb_gso_segment(skb, NETIF_F_SG, false);
> > > +       if (unlikely(IS_ERR(segs)))
> > > +               kfree_skb(skb);
> > > +       else if (segs)
> > > +               consume_skb(skb);
> > > +       return segs;
> > > +}
> > > +
> > > +
> > > +void ip_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int proto);
> > > +
> > > +static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> > > +{
> > > +       struct sk_buff *next, *segs;
> > > +       int ret;
> > > +
> > > +       if (likely(!udp_unexpected_gso(sk, skb)))
> > > +               return udp_queue_rcv_one_skb(sk, skb);
> > > +
> > > +       BUILD_BUG_ON(sizeof(struct udp_skb_cb) > SKB_SGO_CB_OFFSET);
> > > +       __skb_push(skb, -skb_mac_offset(skb));
> > > +       segs = udp_rcv_segment(sk, skb);
> > > +       for (skb = segs; skb; skb = next) {
> >
> > need to check IS_ERR(segs) again?
>
> whooops ... yes, I think so, thanks for catching it.
>
> Since the error code is always discarded, perhpas udp_rcv_segment() can
> simply return 0 when IS_ERR(segs) is true, so we can save a conditional
> here. This is currently a slower/exceptional path, but if we will
> enable UDP GRO for forwaded packets, it will be hit often.

That sounds fine.

In udp_rcv_segment, we should probably account the dropped segments
to UDP_MIB_INERRORS and sk_drops.