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

From: Hangbin Liu <liuhangbin@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Network Development <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: [PATCHv2 net 2/2] IPv6: reply ICMP error if the first fragment don't include all headers
Date: Thu, 22 Oct 2020 17:12:05 +0800	[thread overview]
Message-ID: <20201022091205.GN2531@dhcp-12-153.nay.redhat.com> (raw)
In-Reply-To: <CA+FuTSdCG4yVDb85M=fChfrkU9=F7j88TJujJy_y0pv-Ks_MwQ@mail.gmail.com>

Hi Willem,

Thanks for the comments, see replies below.

On Wed, Oct 21, 2020 at 10:02:55AM -0400, Willem de Bruijn wrote:
> > +       is_frag = (ipv6_find_hdr(skb, &offs, NEXTHDR_FRAGMENT, NULL, NULL) == NEXTHDR_FRAGMENT);
> > +
> 
> ipv6_skip_exthdr already walks all headers. Should we not already see
> frag_off != 0 if skipped over a fragment header? Analogous to the test
> in ipv6_frag_rcv below.

Ah, yes, I forgot we can use this check.

> > +       nexthdr = hdr->nexthdr;
> > +       offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
> > +       if (offset >= 0 && frag_off == htons(IP6_MF) && (offset + 1) > skb->len) {
> 
> Offset +1 does not fully test "all headers through an upper layer
> header". You note the caveat in your commit message. Perhaps for the
> small list of common protocols at least use a length derived from
> nexthdr?

Do you mean check the header like

if (nexthdr == IPPROTO_ICMPV6)
	offset = offset + seizeof(struct icmp6hdr);
else if (nexthdr == ...)
	offset = ...
else
	offset += 1;

if (frag_off == htons(IP6_MF) && offset > skb->len) {
	icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
	return -1;
}

Another questions is how to define the list, does TCP/UDP/SCTP/ICMPv6 enough?

Thanks
Hangbin