netdev - Re: [PATCH net] bnxt_en: Fix and improve .ndo_features

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] [day] [month] [year] [list]

Message-ID: <CACKFLi=gzKF-3ybUG2-dxfOoQ8CSpEuHaFugK_sRHq-y3Lyp5w@mail.gmail.com>
Date:   Thu, 6 May 2021 08:14:00 -0700
From:   Michael Chan <michael.chan@...adcom.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Andy Gospodarek <gospo@...adcom.com>
Subject: Re: [PATCH net] bnxt_en: Fix and improve .ndo_features_check().

On Wed, May 5, 2021 at 6:01 PM Alexander Duyck
<alexander.duyck@...il.com> wrote:
>
> On Wed, May 5, 2021 at 3:43 PM Michael Chan <michael.chan@...adcom.com> wrote:
> > +static bool bnxt_exthdr_check(struct bnxt *bp, struct sk_buff *skb, int nw_off,
> > +                             u8 *nextproto)
> > +{
> > +       struct ipv6hdr *ip6h = (struct ipv6hdr *)(skb->data + nw_off);
> > +       int hdr_count = 0;
> > +       u8 nexthdr;
> > +       int start;
> > +
> > +       /* Check that there are at most 2 IPv6 extension headers, no
> > +        * fragment header, and each is <= 64 bytes.
> > +        */
> > +       start = nw_off + sizeof(*ip6h);
> > +       nexthdr = ip6h->nexthdr;
> > +       while (ipv6_ext_hdr(nexthdr)) {
> > +               struct ipv6_opt_hdr _hdr, *hp;
> > +               int hdrlen;
> > +
> > +               if (hdr_count >= 3 || nexthdr == NEXTHDR_NONE ||
> > +                   nexthdr == NEXTHDR_FRAGMENT)
> > +                       return false;
> > +               hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
> > +               if (!hp)
> > +                       return false;
> > +               if (nexthdr == NEXTHDR_AUTH)
> > +                       hdrlen = ipv6_authlen(hp);
> > +               else
> > +                       hdrlen = ipv6_optlen(hp);
> > +
> > +               if (hdrlen > 64)
> > +                       return false;
> > +               nexthdr = hp->nexthdr;
> > +               start += hdrlen;
> > +               hdr_count++;
> > +       }
> > +       if (nextproto)
> > +               *nextproto = nexthdr;
> > +       return true;
> > +}
> > +
>
> You should really be validating the nexthdr in all cases. I'm assuming
> your offloads are usually for TCP and UDP. You should probably be
> validating that you end with that if you are going to advertise the
> CSUM and GSO offloads.

Yes, I agree with you that we should check for TCP/UDP here.

> This still largely falls short of being able to determine if your
> hardware can handle offloading the packet or not. It would likely make
> much more sense to look at parsing all the way from the L2 up through
> the inner-most L4 header in the case of tunnels to verify that you can
> support offloading it.
>
> For example if I had a packet that had unsupported inner IPv6
> extension headers it doesn't look like it would be caught by this as
> you are only checking the outer headers and the UDP port.
>

I missed the inner ipv6 extension header check for the UDP and GRE
cases.  Thanks for the review.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)