Re: [PATCHv5 net 2/2] IPv6: reply ICMP error if the first fragment don't include all headers

From: "Georg Kohmann (geokohma)" <geokohma@cisco.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	Hangbin Liu <liuhangbin@gmail.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	"Eric Dumazet" <eric.dumazet@gmail.com>
Subject: Re: [PATCHv5 net 2/2] IPv6: reply ICMP error if the first fragment don't include all headers
Date: Fri, 30 Oct 2020 18:39:35 +0000	[thread overview]
Message-ID: <f8b18eb4-3b2f-7fc9-a020-b8e6451fd884@cisco.com> (raw)
In-Reply-To: <CA+FuTSfSUE8M+TuKkBQbEL7L5Bfd=wrZHEqQ67nWZy8oex1JCw@mail.gmail.com>

On 30.10.2020 16:31, Willem de Bruijn wrote:
> On Tue, Oct 27, 2020 at 5:57 AM Hangbin Liu <liuhangbin@gmail.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.