netdev - Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

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: <CAKgT0UcGy_fD6HYxfzO_xoWOcB+Cjf38Qon8ZdWxyTNtzP7-=Q@mail.gmail.com>
Date:   Mon, 17 Oct 2016 12:49:51 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:     "Duyck, Alexander H" <alexander.h.duyck@...el.com>,
        Netdev <netdev@...r.kernel.org>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>
Subject: Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute
 l4_proto only if non-paged data has network/transport headers

On Mon, Oct 17, 2016 at 12:18 PM, Sowmini Varadhan
<sowmini.varadhan@...cle.com> wrote:
> On (10/17/16 11:15), Alexander Duyck wrote:
>> I would say you probably only need the first check here for skb->data
>> and could probably skip the second part.  You will be testing for
>> skb_tail_pointer in all the other tests you added so this check is
>> redundant anyway.
>>
>> Also you might want to go through and wrap these with unlikely() since
>> most of these are exception cases.
>
> Ok.. v3 will have this.
>
>> >         /* Currently only IPv4/IPv6 with TCP is supported */
>> >         switch (hdr.ipv4->version) {
>> >         case IPVERSION:
>> >                 /* access ihl as u8 to avoid unaligned access on ia64 */
>> >                 hlen = (hdr.network[0] & 0x0F) << 2;
>> > +               if (skb_tail_pointer(skb) < hdr.network + hlen +
>> > +                                           sizeof(struct tcphdr))
>> > +                       return;
>> >                 l4_proto = hdr.ipv4->protocol;
>> >                 break;
>> >         case 6:
>> >                 hlen = hdr.network - skb->data;
>> > +               if (skb_tail_pointer(skb) < hdr.network + hlen +
>> > +                                           sizeof(struct tcphdr))
>> > +                       return;
>> >                 l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL, NULL);
>> >                 hlen -= hdr.network - skb->data;
>> >                 break;
>>
>> I believe one more check is needed after this block to verify the TCP
>> header fields are present.
>>
>> So you probably need to add a check for "skb_tail_pointer(skb) <
>> (hdr.network + hlen + 20)".
>
> But isnt that the same thing as the checks before l4_proto computation above?

Sort of.  The problem is IPv6 can include extension headers and that
can totally mess with us.  So we need to do one more check to verify
that we have enough space for IPv6 w/ TCP which would be hdr.raw + 20
+ hlenl.

Thanks.

- Alex