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]
Date:   Wed, 12 Jul 2023 21:58:25 -0500
From:   Yan Zhai <yan@...udflare.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     "open list:NETWORKING [TCP]" <netdev@...r.kernel.org>,
        kernel-team@...udflare.com, Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        David Ahern <dsahern@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Xin Long <lucien.xin@...il.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Andrew Melnychenko <andrew@...nix.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:SCTP PROTOCOL" <linux-sctp@...r.kernel.org>
Subject: Re: [PATCH net] gso: fix GSO_DODGY bit handling for related protocols

On Wed, Jul 12, 2023 at 9:11 PM Jason Wang <jasowang@...hat.com> wrote:
>
> On Thu, Jul 13, 2023 at 9:55 AM Yan Zhai <yan@...udflare.com> wrote:
> >
> > SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source.
> > The canonical way is to recompute the gso_segs to avoid device driver
> > issues. Afterwards, the DODGY bit can be removed to avoid re-check at the
> > egress of later devices, e.g. packets can egress to a vlan device backed
> > by a real NIC.
> >
> > Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4
> > packets.") checks DODGY bit for UDP, but for packets that can be fed
> > directly to the device after gso_segs reset, it actually falls through
> > to fragmentation [1].
> >
> > Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417
> > ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY
> > bit after recomputing gso_segs.
>
> If we try to fix two issues, we'd better use separate patches.
>
> >
> > This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit
> > at other places.
> >
> > Fixes: 90017accff61 ("sctp: Add GSO support")
> > Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy")
> > Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.")
> > Signed-off-by: Yan Zhai <yan@...udflare.com>
> >
> > ---
> > [1]:
> > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/
> >
> > ---
> >  net/ipv4/tcp_offload.c |  1 +
> >  net/ipv4/udp_offload.c | 19 +++++++++++++++----
> >  net/ipv6/udp_offload.c | 19 +++++++++++++++----
> >  net/sctp/offload.c     |  2 ++
> >  4 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index 8311c38267b5..f9b93708c22e 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
> >                 /* Packet is from an untrusted source, reset gso_segs. */
> >
> >                 skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
> > +               skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
> >
> >                 segs = NULL;
> >                 goto out;
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 75aa4de5b731..bd29cf19bb6b 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
> >         if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> >                 goto out;
> >
> > -       if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 &&
> > -           !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST))
> > -               return __udp_gso_segment(skb, features, false);
> > -
> >         mss = skb_shinfo(skb)->gso_size;
> > +
> > +       if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
> > +               if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
> > +                       /* Packet is from an untrusted source, reset actual gso_segs */
> > +                       skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh),
> > +                                                                mss);
> > +                       skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
> > +
> > +                       segs = NULL;
> > +                       goto out;
> > +               } else {
> > +                       return __udp_gso_segment(skb, features, false);
>
> I think it's better and cleaner to move those changes in
> __udp_gso_segment() as Willem suggests.
>
> > +               }
> > +       }
> > +
> >         if (unlikely(skb->len <= mss))
> >                 goto out;
> >
> > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> > index ad3b8726873e..6857d9f7bd06 100644
> > --- a/net/ipv6/udp_offload.c
> > +++ b/net/ipv6/udp_offload.c
> > @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
> >                 if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> >                         goto out;
> >
> > -               if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 &&
> > -                   !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST))
> > -                       return __udp_gso_segment(skb, features, true);
> > -
> >                 mss = skb_shinfo(skb)->gso_size;
> > +
> > +               if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
> > +                       if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
> > +                               /* Packet is from an untrusted source, reset actual gso_segs */
> > +                               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh),
> > +                                                                        mss);
> > +                               skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
>
> Any reason you want to remove the DODGY here? Is this an optimization?
> We will lose the chance to recognize/validate it elsewhere.
>
It is intended as a small optimization. And this is in fact the piece
I am not fully confident about: after validating the gso_segs at a
trusted location (i.e. assuming the kernel is the trusted computing
base), do we need to validate it somewhere else? For example, in our
scenario, we have a tun/tap device in a net namespace, so the packet
going out will enter from the tap, get forwarded through an veth, and
then a vlan backed by a real ethernet interface. If the bit is carried
over, then at each egress of these devices, we need to enter the GSO
code, which feels pretty redundant as long as the packet does not
leave kernel space. WDYT?

thanks


> Thanks
>
> > +
> > +                               segs = NULL;
> > +                               goto out;
> > +                       } else {
> > +                               return __udp_gso_segment(skb, features, true);
> > +                       }
> > +               }
> > +
> >                 if (unlikely(skb->len <= mss))
> >                         goto out;
> >
> > diff --git a/net/sctp/offload.c b/net/sctp/offload.c
> > index 502095173d88..3d2b44db0d42 100644
> > --- a/net/sctp/offload.c
> > +++ b/net/sctp/offload.c
> > @@ -65,6 +65,8 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,
> >                 skb_walk_frags(skb, frag_iter)
> >                         pinfo->gso_segs++;
> >
> > +               pinfo->gso_type &= ~SKB_GSO_DODGY;
> > +
> >                 segs = NULL;
> >                 goto out;
> >         }
> > --
> > 2.30.2
> >
>


-- 

Yan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ