netdev - Re: [PATCHv5 net 2/2] IPv6: reply ICMP error if the first fragment don't include all 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]

Date:   Fri, 30 Oct 2020 18:39:35 +0000
From:   "Georg Kohmann (geokohma)" <geokohma@...co.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Hangbin Liu <liuhangbin@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        "Jakub Kicinski" <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        "Eric Dumazet" <eric.dumazet@...il.com>
Subject: Re: [PATCHv5 net 2/2] IPv6: reply ICMP error if the first fragment
 don't include all headers

On 30.10.2020 16:31, Willem de Bruijn wrote:
> On Tue, Oct 27, 2020 at 5:57 AM Hangbin Liu <liuhangbin@...il.com> wrote:
>> On Tue, Oct 27, 2020 at 07:57:06AM +0000, Georg Kohmann (geokohma) wrote:
>>>> +   /* RFC 8200, Section 4.5 Fragment Header:
>>>> +    * If the first fragment does not include all headers through an
>>>> +    * Upper-Layer header, then that fragment should be discarded and
>>>> +    * an ICMP Parameter Problem, Code 3, message should be sent to
>>>> +    * the source of the fragment, with the Pointer field set to zero.
>>>> +    */
>>>> +   nexthdr = hdr->nexthdr;
>>>> +   offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
>>>> +   if (offset >= 0) {
>>>> +           /* Check some common protocols' header */
>>>> +           if (nexthdr == IPPROTO_TCP)
>>>> +                   offset += sizeof(struct tcphdr);
>>>> +           else if (nexthdr == IPPROTO_UDP)
>>>> +                   offset += sizeof(struct udphdr);
>>>> +           else if (nexthdr == IPPROTO_ICMPV6)
>>>> +                   offset += sizeof(struct icmp6hdr);
>>>> +           else
>>>> +                   offset += 1;
>>>> +
>>>> +           if (frag_off == htons(ip6_mf) && offset > skb->len) {
>>> This do not catch atomic fragments (fragmented packet with only one fragment). frag_off also contains two reserved bits (both 0) that might change in the future.
>> Thanks, I also didn't aware this scenario.
> Sorry, what are atomic fragments?
>
> Do you mean packets with a fragment header that encapsulates the
> entire packet? If so, isn't that handled in the branch right above?
> ("/* It is not a fragmented frame */"). That said, the test based on
> IP6_OFFSET LGTM.
Yes, an atomic fragment is a packet containing a fragment header
without actually beeing fragmented (see RFC6946 and RFC8021).

And you are right, it is handled in the branch right above, sorry for
not seeing that. But still, the test based on IP6_OFFSET is more
accurate as IP6_MF is set for all but the very last fragment.
However, it probably doesn't matter in this context.