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

From: "Georg Kohmann (geokohma)" <geokohma@cisco.com>
To: 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>,
	Willem de Bruijn <willemb@google.com>,
	"David S . Miller" <davem@davemloft.net>,
	Eric Dumazet <eric.dumazet@gmail.com>
Subject: Re: [PATCHv4 net 2/2] IPv6: reply ICMP error if the first fragment don't include all headers
Date: Mon, 26 Oct 2020 14:49:25 +0000	[thread overview]
Message-ID: <ec2d260d-2d83-772f-9430-fee0d9d21c59@cisco.com> (raw)
In-Reply-To: <20201026125519.GO2531@dhcp-12-153.nay.redhat.com>

On 26.10.2020 13:55, Hangbin Liu wrote:
> On Mon, Oct 26, 2020 at 08:09:21AM +0000, Georg Kohmann (geokohma) wrote:
>>> +	nexthdr = hdr->nexthdr;
>>> +	offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
>>> +	if (offset < 0)
>>> +		goto fail_hdr;
>>> +
>>> +	/* 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;
>> Maybe also check the special case IPPROTO_NONE?
> IPPROTO_NONE defines the same with NEXTHDR_NONE. So ipv6_skip_exthdr() will
> return -1, and we will goto fail_hdr and send ICMP parameter error message.
>
> The question is if it's OK to reply a ICMP error for fragment + IPPROTO_NONE
> packet? For pure IPPROTO_NONE message, we should drop silently, but what about
> fragment message?
According to RFC8200 section 4.7: "If the Payload Length field of the IPv6

header indicates the presence of octets past the end of a header whose

Next Header field contains 59, those octets must be ignored and passed

on unchanged if the packet is forwarded." I have not found any RFC

describing different behaviour for fragmented packets.

>
>>> +
>>> +	if (frag_off == htons(IP6_MF) && offset > skb->len) {
>>> +		__IP6_INC_STATS(net, __in6_dev_get_safely(skb->dev), IPSTATS_MIB_INHDRERRORS);
>>> +		icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
>>> +		return -1;
>>> +	}
>>> +
>>>  	iif = skb->dev ? skb->dev->ifindex : 0;
>>>  	fq = fq_find(net, fhdr->identification, hdr, iif);
>>>  	if (fq) {
>> Are you planning to also add this fix for the fragmentation handling in the netfilter?
>>
> I have no plan to fix this on netfilter as netfilter is a module.
> It may have different behavior during defragment.
>
> Thanks
> Hangbin

I might have a look at the netfilter myself then.

Thanks

Georg