[PATCH net 0/2] IPv6: reply ICMP error with fragment doesn't contain all headers

[PATCH net 0/2] IPv6: reply ICMP error with fragment doesn't contain all headers

[Hangbin Liu]

When our Engineer run latest IPv6 Core Conformance test, test v6LC.1.3.6:
First Fragment Doesn’t Contain All Headers[1] failed. The test purpose is to
verify that the node(Linux for example) should properly process IPv6 packets
that don’t include all the headers through the Upper-Layer header.

Based on 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.

The first patch add a definition for ICMPv6 Parameter Problem, code 3.
The second patch add a check for the 1st fragment packet to make sure
Upper-Layer header exist.

[1] Page 68, v6LC.1.3.6: First Fragment Doesn’t Contain All Headers part A, B,
C and D at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf
[2] My reproducer:
#!/usr/bin/env python3

import sys, os
from scapy.all import *

# Test v6LC.1.3.6: First Fragment Doesn’t Contain All Headers
def send_frag_dst_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 60, m = 1)
    dst_opt = IPv6ExtHdrDestOpt(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 1)

    pkt_1 = ip6/frag_1/dst_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

def send_frag_route_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 43, m = 1)
    route_opt = IPv6ExtHdrRouting(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 2)

    pkt_1 = ip6/frag_1/route_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

if __name__ == '__main__':
    src = sys.argv[1]
    dst = sys.argv[2]
    conf.iface = sys.argv[3]
    send_frag_dst_opt(src, dst)
    send_frag_route_opt(src, dst)

Hangbin Liu (2):
  ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition
  IPv6: reply ICMP error if the first fragment don't include all headers

 include/uapi/linux/icmpv6.h |  1 +
 net/ipv6/icmp.c             | 13 ++++++++++++-
 net/ipv6/ip6_input.c        | 20 +++++++++++++++++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.25.4

[PATCH net 1/2] ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition

[Hangbin Liu]

Based on RFC7112, Section 6:

   IANA has added the following "Type 4 - Parameter Problem" message to
   the "Internet Control Message Protocol version 6 (ICMPv6) Parameters"
   registry:

      CODE     NAME/DESCRIPTION
       3       IPv6 First Fragment has incomplete IPv6 Header Chain

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/uapi/linux/icmpv6.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
index c1661febc2dc..0564fd7ccde4 100644
--- a/include/uapi/linux/icmpv6.h
+++ b/include/uapi/linux/icmpv6.h
@@ -138,6 +138,7 @@ struct icmp6hdr {
 #define ICMPV6_HDR_FIELD		0
 #define ICMPV6_UNK_NEXTHDR		1
 #define ICMPV6_UNK_OPTION		2
+#define ICMPV6_HDR_INCOMP		3
 
 /*
  *	constants for (set|get)sockopt
-- 
2.25.4

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

[Hangbin Liu]

Based on 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.

As the packet may be any kind of L4 protocol, I only checked if there
has Upper-Layer header by pskb_may_pull(skb, offset + 1).

As the 1st truncated fragment may also be ICMP message, I also add
a check in ICMP code is_ineligible() to let fragment packet with nexthdr
ICMP but no ICMP header return false.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/icmp.c      | 13 ++++++++++++-
 net/ipv6/ip6_input.c | 20 +++++++++++++++++++-
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index a4e4912ad607..03060c8f463d 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -145,7 +145,9 @@ static bool is_ineligible(const struct sk_buff *skb)
 	int ptr = (u8 *)(ipv6_hdr(skb) + 1) - skb->data;
 	int len = skb->len - ptr;
 	__u8 nexthdr = ipv6_hdr(skb)->nexthdr;
+	unsigned int offs = 0;
 	__be16 frag_off;
+	bool is_frag;
 
 	if (len < 0)
 		return true;
@@ -153,12 +155,21 @@ static bool is_ineligible(const struct sk_buff *skb)
 	ptr = ipv6_skip_exthdr(skb, ptr, &nexthdr, &frag_off);
 	if (ptr < 0)
 		return false;
+
+	is_frag = (ipv6_find_hdr(skb, &offs, NEXTHDR_FRAGMENT, &frag_off, NULL) == NEXTHDR_FRAGMENT);
+
 	if (nexthdr == IPPROTO_ICMPV6) {
 		u8 _type, *tp;
 		tp = skb_header_pointer(skb,
 			ptr+offsetof(struct icmp6hdr, icmp6_type),
 			sizeof(_type), &_type);
-		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
+
+		/* Based on RFC 8200, Section 4.5 Fragment Header, return
+		 * false if this is a fragment packet with no icmp header info.
+		 */
+		if (!tp && is_frag)
+			return false;
+		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
 			return true;
 	}
 	return false;
diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index e96304d8a4a7..637d8d59e058 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -146,8 +146,11 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 				    struct net *net)
 {
 	const struct ipv6hdr *hdr;
-	u32 pkt_len;
 	struct inet6_dev *idev;
+	__be16 frag_off;
+	u32 pkt_len;
+	int offset;
+	u8 nexthdr;
 
 	if (skb->pkt_type == PACKET_OTHERHOST) {
 		kfree_skb(skb);
@@ -282,6 +285,21 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
+	/* 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 (frag_off == htons(IP6_MF) && !pskb_may_pull(skb, offset + 1)) {
+		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
+		icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
+		rcu_read_unlock();
+		return NULL;
+	}
+
 	rcu_read_unlock();
 
 	/* Must drop socket now because of tproxy. */
-- 
2.25.4

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

[Eric Dumazet]

On 10/7/20 5:55 AM, Hangbin Liu wrote:

>  		kfree_skb(skb);
> @@ -282,6 +285,21 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>  		}
>  	}
>  
> +	/* 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 (frag_off == htons(IP6_MF) && !pskb_may_pull(skb, offset + 1)) {
> +		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> +		icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
> +		rcu_read_unlock();
> +		return NULL;
> +	}
> +
>  	rcu_read_unlock();
>  
>  	/* Must drop socket now because of tproxy. */
> 

Ouch, this is quite a buggy patch.

I doubt we want to add yet another ipv6_skip_exthdr() call in IPv6 fast path.

Surely the presence of NEXTHDR_FRAGMENT is already tested elsewhere ?

Also, ipv6_skip_exthdr() does not pull anything in skb->head, it would be strange
to force a pull of hundreds of bytes just because you want to check if an extra byte is there,
if the packet could be forwarded as is, without additional memory allocations.

Testing skb->len should be more than enough at this stage.

Also ipv6_skip_exthdr() can return an error.

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

[Jakub Kicinski]

On Wed,  7 Oct 2020 11:55:02 +0800 Hangbin Liu wrote:
> Based on 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.
> 
> As the packet may be any kind of L4 protocol, I only checked if there
> has Upper-Layer header by pskb_may_pull(skb, offset + 1).
> 
> As the 1st truncated fragment may also be ICMP message, I also add
> a check in ICMP code is_ineligible() to let fragment packet with nexthdr
> ICMP but no ICMP header return false.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

net/ipv6/icmp.c:159:65: warning: incorrect type in argument 4 (different base types)
net/ipv6/icmp.c:159:65:    expected unsigned short *fragoff
net/ipv6/icmp.c:159:65:    got restricted __be16 *

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

[Hangbin Liu]

Hi Eric,

Thanks for the comments. I should add "RFC" in subject next time for the
uncertain fix patch.

On Wed, Oct 07, 2020 at 11:35:41AM +0200, Eric Dumazet wrote:
> 
> 
> On 10/7/20 5:55 AM, Hangbin Liu wrote:
> 
> >  		kfree_skb(skb);
> > @@ -282,6 +285,21 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >  		}
> >  	}
> >  
> > +	/* 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 (frag_off == htons(IP6_MF) && !pskb_may_pull(skb, offset + 1)) {
> > +		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> > +		icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
> > +		rcu_read_unlock();
> > +		return NULL;
> > +	}
> > +
> >  	rcu_read_unlock();
> >  
> >  	/* Must drop socket now because of tproxy. */
> > 
> 
> Ouch, this is quite a buggy patch.
> 
> I doubt we want to add yet another ipv6_skip_exthdr() call in IPv6 fast path.
> 
> Surely the presence of NEXTHDR_FRAGMENT is already tested elsewhere ?

Would you like to help point where NEXTHDR_FRAGMENT was tested before IPv6
defragment?

> 
> Also, ipv6_skip_exthdr() does not pull anything in skb->head, it would be strange
> to force a pull of hundreds of bytes just because you want to check if an extra byte is there,
> if the packet could be forwarded as is, without additional memory allocations.
> 
> Testing skb->len should be more than enough at this stage.

Ah, yes, I shouldn't call pskb_may_pull here.
> 
> Also ipv6_skip_exthdr() can return an error.

it returns -1 as error, If we tested it by (offset + 1 > skb->len), does
that count as an error handler?

Thanks
Hangbin

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

[Hangbin Liu]

On Wed, Oct 07, 2020 at 07:58:07AM -0700, Jakub Kicinski wrote:
> On Wed,  7 Oct 2020 11:55:02 +0800 Hangbin Liu wrote:
> > Based on 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.
> > 
> > As the packet may be any kind of L4 protocol, I only checked if there
> > has Upper-Layer header by pskb_may_pull(skb, offset + 1).
> > 
> > As the 1st truncated fragment may also be ICMP message, I also add
> > a check in ICMP code is_ineligible() to let fragment packet with nexthdr
> > ICMP but no ICMP header return false.
> > 
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> net/ipv6/icmp.c:159:65: warning: incorrect type in argument 4 (different base types)
> net/ipv6/icmp.c:159:65:    expected unsigned short *fragoff
> net/ipv6/icmp.c:159:65:    got restricted __be16 *

Ah, Thanks for pointing out this error, I will fix it.

Regards
Hangbin

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

[Eric Dumazet]

On 10/8/20 10:30 AM, Hangbin Liu wrote:
> Hi Eric,
> 
> Thanks for the comments. I should add "RFC" in subject next time for the
> uncertain fix patch.
> 
> On Wed, Oct 07, 2020 at 11:35:41AM +0200, Eric Dumazet wrote:
>>
>>
>> On 10/7/20 5:55 AM, Hangbin Liu wrote:
>>
>>>  		kfree_skb(skb);
>>> @@ -282,6 +285,21 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>>>  		}
>>>  	}
>>>  
>>> +	/* 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 (frag_off == htons(IP6_MF) && !pskb_may_pull(skb, offset + 1)) {
>>> +		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
>>> +		icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
>>> +		rcu_read_unlock();
>>> +		return NULL;
>>> +	}
>>> +
>>>  	rcu_read_unlock();
>>>  
>>>  	/* Must drop socket now because of tproxy. */
>>>
>>
>> Ouch, this is quite a buggy patch.
>>
>> I doubt we want to add yet another ipv6_skip_exthdr() call in IPv6 fast path.
>>
>> Surely the presence of NEXTHDR_FRAGMENT is already tested elsewhere ?
> 
> Would you like to help point where NEXTHDR_FRAGMENT was tested before IPv6
> defragment?
I think we have to ask the question : Should routers enforce the rule, or
only end points ?

End points must handle NEXTHDR_FRAGMENT, in ipv6_frag_rcv()


> 
>>
>> Also, ipv6_skip_exthdr() does not pull anything in skb->head, it would be strange
>> to force a pull of hundreds of bytes just because you want to check if an extra byte is there,
>> if the packet could be forwarded as is, without additional memory allocations.
>>
>> Testing skb->len should be more than enough at this stage.
> 
> Ah, yes, I shouldn't call pskb_may_pull here.
>>
>> Also ipv6_skip_exthdr() can return an error.
> 
> it returns -1 as error, If we tested it by (offset + 1 > skb->len), does
> that count as an error handler?

If there is an error, then you want to send the ICMP, right ?

The (offset + 1) expression will become 0, and surely the test will be false,
so you wont send the ICMP...

> 
> Thanks
> Hangbin
> 

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

[Hangbin Liu]

On Thu, Oct 08, 2020 at 11:47:00AM +0200, Eric Dumazet wrote:
> 
> 
> On 10/8/20 10:30 AM, Hangbin Liu wrote:
> >>> @@ -282,6 +285,21 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >>>  		}
> >>>  	}
> >>>  
> >>> +	/* 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 (frag_off == htons(IP6_MF) && !pskb_may_pull(skb, offset + 1)) {
> >>> +		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> >>> +		icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
> >>> +		rcu_read_unlock();
> >>> +		return NULL;
> >>> +	}
> >>> +
> >>>  	rcu_read_unlock();
> >>>  
> >>>  	/* Must drop socket now because of tproxy. */
> >>>
> >>
> >> Ouch, this is quite a buggy patch.
> >>
> >> I doubt we want to add yet another ipv6_skip_exthdr() call in IPv6 fast path.
> >>
> >> Surely the presence of NEXTHDR_FRAGMENT is already tested elsewhere ?
> > 
> > Would you like to help point where NEXTHDR_FRAGMENT was tested before IPv6
> > defragment?
> I think we have to ask the question : Should routers enforce the rule, or
> only end points ?

From IPv6 Core Conformance test[1], it applied to both router and host(It will
marked specifically if a test only for router).

> 
> End points must handle NEXTHDR_FRAGMENT, in ipv6_frag_rcv()

Yes, I was also try put the check there, but it looks that would be too late
if module nf_defrag_ipv6 loaded

> >> Also ipv6_skip_exthdr() can return an error.
> > 
> > it returns -1 as error, If we tested it by (offset + 1 > skb->len), does
> > that count as an error handler?
> 
> If there is an error, then you want to send the ICMP, right ?

No, this is only for fragment header with no enough Upper-Layer header, which need
send ICMP Parameter Problem, Code 3 specifically. For other errors, I guess
the other code will take care of it.

So for -1 return, I just skipped it.
> 
> The (offset + 1) expression will become 0, and surely the test will be false,
> so you wont send the ICMP...

[1] v6LC.1.3.6: First Fragment Doesn’t Contain All Headers part A, B,
C and D at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf

Thanks
Hangbin

[PATCHv2 net 0/2] IPv6: reply ICMP error with fragment doesn't contain all headers

[Hangbin Liu]

When our Engineer run latest IPv6 Core Conformance test, test v6LC.1.3.6:
First Fragment Doesn’t Contain All Headers[1] failed. The test purpose is to
verify that the node(Linux for example) should properly process IPv6 packets
that don’t include all the headers through the Upper-Layer header.

Based on 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.

The first patch add a definition for ICMPv6 Parameter Problem, code 3.
The second patch add a check for the 1st fragment packet to make sure
Upper-Layer header exist.

[1] Page 68, v6LC.1.3.6: First Fragment Doesn’t Contain All Headers part A, B,
C and D at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf
[2] My reproducer:
#!/usr/bin/env python3

import sys, os
from scapy.all import *

# Test v6LC.1.3.6: First Fragment Doesn’t Contain All Headers
def send_frag_dst_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 60, m = 1)
    dst_opt = IPv6ExtHdrDestOpt(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 1)

    pkt_1 = ip6/frag_1/dst_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

def send_frag_route_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 43, m = 1)
    route_opt = IPv6ExtHdrRouting(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 2)

    pkt_1 = ip6/frag_1/route_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

if __name__ == '__main__':
    src = sys.argv[1]
    dst = sys.argv[2]
    conf.iface = sys.argv[3]
    send_frag_dst_opt(src, dst)
    send_frag_route_opt(src, dst)

Hangbin Liu (2):
  ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition
  IPv6: reply ICMP error if the first fragment don't include all headers

 include/uapi/linux/icmpv6.h |  1 +
 net/ipv6/icmp.c             | 13 ++++++++++++-
 net/ipv6/reassembly.c       | 18 +++++++++++++++++-
 3 files changed, 30 insertions(+), 2 deletions(-)

-- 
2.25.4

[PATCHv2 net 1/2] ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition

[Hangbin Liu]

Based on RFC7112, Section 6:

   IANA has added the following "Type 4 - Parameter Problem" message to
   the "Internet Control Message Protocol version 6 (ICMPv6) Parameters"
   registry:

      CODE     NAME/DESCRIPTION
       3       IPv6 First Fragment has incomplete IPv6 Header Chain

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/uapi/linux/icmpv6.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
index c1661febc2dc..0564fd7ccde4 100644
--- a/include/uapi/linux/icmpv6.h
+++ b/include/uapi/linux/icmpv6.h
@@ -138,6 +138,7 @@ struct icmp6hdr {
 #define ICMPV6_HDR_FIELD		0
 #define ICMPV6_UNK_NEXTHDR		1
 #define ICMPV6_UNK_OPTION		2
+#define ICMPV6_HDR_INCOMP		3
 
 /*
  *	constants for (set|get)sockopt
-- 
2.25.4

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

[Hangbin Liu]

Based on 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.

As the packet may be any kind of L4 protocol, I only checked if there
has Upper-Layer header by (offset + 1) > skb->len. Checking each packet
header in IPv6 fast path will have performace impact, so I put the
checking in ipv6_frag_rcv().

When send ICMP error message, if the first truncated fragment is ICMP
message, icmp6_send() will break as is_ineligible() return true. So I
added a check in is_ineligible() to let fragment packet with nexthdr
ICMP but no ICMP header return false.

v2:
a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
   return value
b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/icmp.c       | 13 ++++++++++++-
 net/ipv6/reassembly.c | 18 +++++++++++++++++-
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index ec448b71bf9a..50d28764c8dd 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -145,7 +145,9 @@ static bool is_ineligible(const struct sk_buff *skb)
 	int ptr = (u8 *)(ipv6_hdr(skb) + 1) - skb->data;
 	int len = skb->len - ptr;
 	__u8 nexthdr = ipv6_hdr(skb)->nexthdr;
+	unsigned int offs = 0;
 	__be16 frag_off;
+	bool is_frag;
 
 	if (len < 0)
 		return true;
@@ -153,12 +155,21 @@ static bool is_ineligible(const struct sk_buff *skb)
 	ptr = ipv6_skip_exthdr(skb, ptr, &nexthdr, &frag_off);
 	if (ptr < 0)
 		return false;
+
+	is_frag = (ipv6_find_hdr(skb, &offs, NEXTHDR_FRAGMENT, NULL, NULL) == NEXTHDR_FRAGMENT);
+
 	if (nexthdr == IPPROTO_ICMPV6) {
 		u8 _type, *tp;
 		tp = skb_header_pointer(skb,
 			ptr+offsetof(struct icmp6hdr, icmp6_type),
 			sizeof(_type), &_type);
-		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
+
+		/* Based on RFC 8200, Section 4.5 Fragment Header, return
+		 * false if this is a fragment packet with no icmp header info.
+		 */
+		if (!tp && is_frag)
+			return false;
+		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
 			return true;
 	}
 	return false;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 1f5d4d196dcc..b359bffa2f58 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -322,7 +322,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 	struct frag_queue *fq;
 	const struct ipv6hdr *hdr = ipv6_hdr(skb);
 	struct net *net = dev_net(skb_dst(skb)->dev);
-	int iif;
+	__be16 frag_off;
+	int iif, offset;
+	u8 nexthdr;
 
 	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
 		goto fail_hdr;
@@ -351,6 +353,20 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 		return 1;
 	}
 
+	/* 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 && frag_off == htons(IP6_MF) && (offset + 1) > 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) {
-- 
2.25.4

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

[Willem de Bruijn]

On Wed, Oct 21, 2020 at 12:20 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Based on 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.
>
> As the packet may be any kind of L4 protocol, I only checked if there
> has Upper-Layer header by (offset + 1) > skb->len. Checking each packet
> header in IPv6 fast path will have performace impact, so I put the

nit: performa[n]ce

> checking in ipv6_frag_rcv().
>
> When send ICMP error message, if the first truncated fragment is ICMP
> message, icmp6_send() will break as is_ineligible() return true. So I
> added a check in is_ineligible() to let fragment packet with nexthdr
> ICMP but no ICMP header return false.
>
> v2:
> a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
>    return value
> b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/icmp.c       | 13 ++++++++++++-
>  net/ipv6/reassembly.c | 18 +++++++++++++++++-
>  2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index ec448b71bf9a..50d28764c8dd 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -145,7 +145,9 @@ static bool is_ineligible(const struct sk_buff *skb)
>         int ptr = (u8 *)(ipv6_hdr(skb) + 1) - skb->data;
>         int len = skb->len - ptr;
>         __u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> +       unsigned int offs = 0;
>         __be16 frag_off;
> +       bool is_frag;
>
>         if (len < 0)
>                 return true;
> @@ -153,12 +155,21 @@ static bool is_ineligible(const struct sk_buff *skb)
>         ptr = ipv6_skip_exthdr(skb, ptr, &nexthdr, &frag_off);
>         if (ptr < 0)
>                 return false;
> +
> +       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.

>         if (nexthdr == IPPROTO_ICMPV6) {
>                 u8 _type, *tp;
>                 tp = skb_header_pointer(skb,
>                         ptr+offsetof(struct icmp6hdr, icmp6_type),
>                         sizeof(_type), &_type);
> -               if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
> +
> +               /* Based on RFC 8200, Section 4.5 Fragment Header, return
> +                * false if this is a fragment packet with no icmp header info.
> +                */
> +               if (!tp && is_frag)
> +                       return false;
> +               else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
>                         return true;
>         }
>         return false;
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 1f5d4d196dcc..b359bffa2f58 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -322,7 +322,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>         struct frag_queue *fq;
>         const struct ipv6hdr *hdr = ipv6_hdr(skb);
>         struct net *net = dev_net(skb_dst(skb)->dev);
> -       int iif;
> +       __be16 frag_off;
> +       int iif, offset;
> +       u8 nexthdr;
>
>         if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
>                 goto fail_hdr;
> @@ -351,6 +353,20 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>                 return 1;
>         }
>
> +       /* 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 && 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?


> +               __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) {
> --
> 2.25.4
>

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

[Hangbin Liu]

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

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

[Willem de Bruijn]

On Thu, Oct 22, 2020 at 5:12 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> 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?

Exactly. But only if it's possible without adding a ton of #include's.
It is best effort.

If feasible, TCP + UDP alone would suffice to cover most traffic.

[PATCHv3 net 0/2] IPv6: reply ICMP error if fragment doesn't contain all headers

[Hangbin Liu]

When our Engineer run latest IPv6 Core Conformance test, test v6LC.1.3.6:
First Fragment Doesn’t Contain All Headers[1] failed. The test purpose is to
verify that the node(Linux for example) should properly process IPv6 packets
that don’t include all the headers through the Upper-Layer header.

Based on 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.

The first patch add a definition for ICMPv6 Parameter Problem, code 3.
The second patch add a check for the 1st fragment packet to make sure
Upper-Layer header exist.

[1] Page 68, v6LC.1.3.6: First Fragment Doesn’t Contain All Headers part A, B,
C and D at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf
[2] My reproducer:
#!/usr/bin/env python3

import sys, os
from scapy.all import *

# Test v6LC.1.3.6: First Fragment Doesn’t Contain All Headers
def send_frag_dst_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 60, m = 1)
    dst_opt = IPv6ExtHdrDestOpt(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 1)

    pkt_1 = ip6/frag_1/dst_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

def send_frag_route_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 43, m = 1)
    route_opt = IPv6ExtHdrRouting(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 2)

    pkt_1 = ip6/frag_1/route_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

if __name__ == '__main__':
    src = sys.argv[1]
    dst = sys.argv[2]
    conf.iface = sys.argv[3]
    send_frag_dst_opt(src, dst)
    send_frag_route_opt(src, dst)

Hangbin Liu (2):
  ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition
  IPv6: reply ICMP error if the first fragment don't include all headers

 include/uapi/linux/icmpv6.h |  1 +
 net/ipv6/icmp.c             | 10 +++++++++-
 net/ipv6/reassembly.c       | 33 ++++++++++++++++++++++++++++++++-
 3 files changed, 42 insertions(+), 2 deletions(-)

-- 
2.25.4

[PATCHv3 net 1/2] ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition

[Hangbin Liu]

Based on RFC7112, Section 6:

   IANA has added the following "Type 4 - Parameter Problem" message to
   the "Internet Control Message Protocol version 6 (ICMPv6) Parameters"
   registry:

      CODE     NAME/DESCRIPTION
       3       IPv6 First Fragment has incomplete IPv6 Header Chain

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/uapi/linux/icmpv6.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
index c1661febc2dc..0564fd7ccde4 100644
--- a/include/uapi/linux/icmpv6.h
+++ b/include/uapi/linux/icmpv6.h
@@ -138,6 +138,7 @@ struct icmp6hdr {
 #define ICMPV6_HDR_FIELD		0
 #define ICMPV6_UNK_NEXTHDR		1
 #define ICMPV6_UNK_OPTION		2
+#define ICMPV6_HDR_INCOMP		3
 
 /*
  *	constants for (set|get)sockopt
-- 
2.25.4

[PATCHv3 net 2/2] IPv6: reply ICMP error if the first fragment doesn't include all headers

[Hangbin Liu]

Based on 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.

As the packet may be any kind of L4 protocol, I only checked some common
protocols' header length and handle others by (offset + 1) > skb->len.
Checking each packet header in IPv6 fast path will have performance impact,
so I put the checking in ipv6_frag_rcv().

When send ICMP error message, if the 1st truncated fragment is ICMP message,
icmp6_send() will break as is_ineligible() return true. So I added a check
in is_ineligible() to let fragment packet with nexthdr ICMP but no ICMP header
return false.

v3:
a) use frag_off to check if this is a fragment packet
b) check some common protocols' header length

v2:
a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
   return value
b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/icmp.c       | 10 +++++++++-
 net/ipv6/reassembly.c | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index ec448b71bf9a..0bda77d7e6b8 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -145,6 +145,7 @@ static bool is_ineligible(const struct sk_buff *skb)
 	int ptr = (u8 *)(ipv6_hdr(skb) + 1) - skb->data;
 	int len = skb->len - ptr;
 	__u8 nexthdr = ipv6_hdr(skb)->nexthdr;
+	unsigned int offs = 0;
 	__be16 frag_off;
 
 	if (len < 0)
@@ -153,12 +154,19 @@ static bool is_ineligible(const struct sk_buff *skb)
 	ptr = ipv6_skip_exthdr(skb, ptr, &nexthdr, &frag_off);
 	if (ptr < 0)
 		return false;
+
 	if (nexthdr == IPPROTO_ICMPV6) {
 		u8 _type, *tp;
 		tp = skb_header_pointer(skb,
 			ptr+offsetof(struct icmp6hdr, icmp6_type),
 			sizeof(_type), &_type);
-		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
+
+		/* Based on RFC 8200, Section 4.5 Fragment Header, return
+		 * false if this is a fragment packet with no icmp header info.
+		 */
+		if (!tp && frag_off != 0)
+			return false;
+		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
 			return true;
 	}
 	return false;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 1f5d4d196dcc..bf042bcb5a47 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -42,6 +42,8 @@
 #include <linux/skbuff.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
 
 #include <net/sock.h>
 #include <net/snmp.h>
@@ -322,7 +324,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 	struct frag_queue *fq;
 	const struct ipv6hdr *hdr = ipv6_hdr(skb);
 	struct net *net = dev_net(skb_dst(skb)->dev);
-	int iif;
+	__be16 frag_off;
+	int iif, offset;
+	u8 nexthdr;
 
 	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
 		goto fail_hdr;
@@ -351,6 +355,33 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 		return 1;
 	}
 
+	/* 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)
+		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;
+
+	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) {
-- 
2.25.4

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

[Jakub Kicinski]

On Fri, 23 Oct 2020 14:43:47 +0800 Hangbin Liu wrote:
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index ec448b71bf9a..0bda77d7e6b8 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -145,6 +145,7 @@ static bool is_ineligible(const struct sk_buff *skb)
>  	int ptr = (u8 *)(ipv6_hdr(skb) + 1) - skb->data;
>  	int len = skb->len - ptr;
>  	__u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> +	unsigned int offs = 0;
>  	__be16 frag_off;
>  
>  	if (len < 0)

net/ipv6/icmp.c: In function ‘is_ineligible’:
net/ipv6/icmp.c:148:15: warning: unused variable ‘offs’ [-Wunused-variable]
  148 |  unsigned int offs = 0;
      |               ^~~~

[PATCHv4 net 0/2] IPv6: reply ICMP error if fragment doesn't contain all headers

[Hangbin Liu]

When our Engineer run latest IPv6 Core Conformance test, test v6LC.1.3.6:
First Fragment Doesn’t Contain All Headers[1] failed. The test purpose is to
verify that the node(Linux for example) should properly process IPv6 packets
that don’t include all the headers through the Upper-Layer header.

Based on 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.

The first patch add a definition for ICMPv6 Parameter Problem, code 3.
The second patch add a check for the 1st fragment packet to make sure
Upper-Layer header exist.

[1] Page 68, v6LC.1.3.6: First Fragment Doesn’t Contain All Headers part A, B,
C and D at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf
[2] My reproducer:
#!/usr/bin/env python3

import sys, os
from scapy.all import *

# Test v6LC.1.3.6: First Fragment Doesn’t Contain All Headers
def send_frag_dst_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 60, m = 1)
    dst_opt = IPv6ExtHdrDestOpt(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 1)

    pkt_1 = ip6/frag_1/dst_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

def send_frag_route_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 43, m = 1)
    route_opt = IPv6ExtHdrRouting(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 2)

    pkt_1 = ip6/frag_1/route_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

if __name__ == '__main__':
    src = sys.argv[1]
    dst = sys.argv[2]
    conf.iface = sys.argv[3]
    send_frag_dst_opt(src, dst)
    send_frag_route_opt(src, dst)

Hangbin Liu (2):
  ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition
  IPv6: reply ICMP error if the first fragment don't include all headers

 include/uapi/linux/icmpv6.h |  1 +
 net/ipv6/icmp.c             |  8 +++++++-
 net/ipv6/reassembly.c       | 33 ++++++++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

-- 
2.25.4

[PATCHv4 net 1/2] ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition

[Hangbin Liu]

Based on RFC7112, Section 6:

   IANA has added the following "Type 4 - Parameter Problem" message to
   the "Internet Control Message Protocol version 6 (ICMPv6) Parameters"
   registry:

      CODE     NAME/DESCRIPTION
       3       IPv6 First Fragment has incomplete IPv6 Header Chain

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/uapi/linux/icmpv6.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
index c1661febc2dc..0564fd7ccde4 100644
--- a/include/uapi/linux/icmpv6.h
+++ b/include/uapi/linux/icmpv6.h
@@ -138,6 +138,7 @@ struct icmp6hdr {
 #define ICMPV6_HDR_FIELD		0
 #define ICMPV6_UNK_NEXTHDR		1
 #define ICMPV6_UNK_OPTION		2
+#define ICMPV6_HDR_INCOMP		3
 
 /*
  *	constants for (set|get)sockopt
-- 
2.25.4

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

[Hangbin Liu]

Based on 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.

As the packet may be any kind of L4 protocol, I only checked some common
protocols' header length and handle others by (offset + 1) > skb->len.
Checking each packet header in IPv6 fast path will have performance impact,
so I put the checking in ipv6_frag_rcv().

When send ICMP error message, if the 1st truncated fragment is ICMP message,
icmp6_send() will break as is_ineligible() return true. So I added a check
in is_ineligible() to let fragment packet with nexthdr ICMP but no ICMP header
return false.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v4:
remove unused variable

v3:
a) use frag_off to check if this is a fragment packet
b) check some common protocols' header length

v2:
a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
   return value
b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()

---
 net/ipv6/icmp.c       |  8 +++++++-
 net/ipv6/reassembly.c | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index ec448b71bf9a..8956144ea65e 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -158,7 +158,13 @@ static bool is_ineligible(const struct sk_buff *skb)
 		tp = skb_header_pointer(skb,
 			ptr+offsetof(struct icmp6hdr, icmp6_type),
 			sizeof(_type), &_type);
-		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
+
+		/* Based on RFC 8200, Section 4.5 Fragment Header, return
+		 * false if this is a fragment packet with no icmp header info.
+		 */
+		if (!tp && frag_off != 0)
+			return false;
+		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
 			return true;
 	}
 	return false;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 1f5d4d196dcc..bf042bcb5a47 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -42,6 +42,8 @@
 #include <linux/skbuff.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
 
 #include <net/sock.h>
 #include <net/snmp.h>
@@ -322,7 +324,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 	struct frag_queue *fq;
 	const struct ipv6hdr *hdr = ipv6_hdr(skb);
 	struct net *net = dev_net(skb_dst(skb)->dev);
-	int iif;
+	__be16 frag_off;
+	int iif, offset;
+	u8 nexthdr;
 
 	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
 		goto fail_hdr;
@@ -351,6 +355,33 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 		return 1;
 	}
 
+	/* 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)
+		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;
+
+	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) {
-- 
2.25.4

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

[Georg Kohmann (geokohma)]

On 26.10.2020 08:29, Hangbin Liu wrote:
> Based on 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.
>
> As the packet may be any kind of L4 protocol, I only checked some common
> protocols' header length and handle others by (offset + 1) > skb->len.
> Checking each packet header in IPv6 fast path will have performance impact,
> so I put the checking in ipv6_frag_rcv().
>
> When send ICMP error message, if the 1st truncated fragment is ICMP message,
> icmp6_send() will break as is_ineligible() return true. So I added a check
> in is_ineligible() to let fragment packet with nexthdr ICMP but no ICMP header
> return false.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> v4:
> remove unused variable
>
> v3:
> a) use frag_off to check if this is a fragment packet
> b) check some common protocols' header length
>
> v2:
> a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
>    return value
> b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()
>
> ---
>  net/ipv6/icmp.c       |  8 +++++++-
>  net/ipv6/reassembly.c | 33 ++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index ec448b71bf9a..8956144ea65e 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -158,7 +158,13 @@ static bool is_ineligible(const struct sk_buff *skb)
>  		tp = skb_header_pointer(skb,
>  			ptr+offsetof(struct icmp6hdr, icmp6_type),
>  			sizeof(_type), &_type);
> -		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
> +
> +		/* Based on RFC 8200, Section 4.5 Fragment Header, return
> +		 * false if this is a fragment packet with no icmp header info.
> +		 */
> +		if (!tp && frag_off != 0)
> +			return false;
> +		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
>  			return true;
>  	}
>  	return false;
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 1f5d4d196dcc..bf042bcb5a47 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -42,6 +42,8 @@
>  #include <linux/skbuff.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
> +#include <linux/tcp.h>
> +#include <linux/udp.h>
>  
>  #include <net/sock.h>
>  #include <net/snmp.h>
> @@ -322,7 +324,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>  	struct frag_queue *fq;
>  	const struct ipv6hdr *hdr = ipv6_hdr(skb);
>  	struct net *net = dev_net(skb_dst(skb)->dev);
> -	int iif;
> +	__be16 frag_off;
> +	int iif, offset;
> +	u8 nexthdr;
>  
>  	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
>  		goto fail_hdr;
> @@ -351,6 +355,33 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>  		return 1;
>  	}
>  
> +	/* 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)
> +		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?

> +
> +	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?

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

[Hangbin Liu]

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?

> > +
> > +	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

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

[Georg Kohmann (geokohma)]

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

[PATCHv5 net 0/2] IPv6: reply ICMP error if fragment doesn't contain all headers

[Hangbin Liu]

When our Engineer run latest IPv6 Core Conformance test, test v6LC.1.3.6:
First Fragment Doesn’t Contain All Headers[1] failed. The test purpose is to
verify that the node(Linux for example) should properly process IPv6 packets
that don’t include all the headers through the Upper-Layer header.

Based on 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.

The first patch add a definition for ICMPv6 Parameter Problem, code 3.
The second patch add a check for the 1st fragment packet to make sure
Upper-Layer header exist.

[1] Page 68, v6LC.1.3.6: First Fragment Doesn’t Contain All Headers part A, B,
C and D at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf
[2] My reproducer:
#!/usr/bin/env python3

import sys, os
from scapy.all import *

# Test v6LC.1.3.6: First Fragment Doesn’t Contain All Headers
def send_frag_dst_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 60, m = 1)
    dst_opt = IPv6ExtHdrDestOpt(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 1)

    pkt_1 = ip6/frag_1/dst_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

def send_frag_route_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 43, m = 1)
    route_opt = IPv6ExtHdrRouting(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 2)

    pkt_1 = ip6/frag_1/route_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

if __name__ == '__main__':
    src = sys.argv[1]
    dst = sys.argv[2]
    conf.iface = sys.argv[3]
    send_frag_dst_opt(src, dst)
    send_frag_route_opt(src, dst)

Hangbin Liu (2):
  ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition
  IPv6: reply ICMP error if the first fragment don't include all headers

 include/uapi/linux/icmpv6.h |  1 +
 net/ipv6/icmp.c             |  8 +++++++-
 net/ipv6/reassembly.c       | 33 ++++++++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

-- 
2.25.4

[PATCHv5 net 1/2] ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition

[Hangbin Liu]

Based on RFC7112, Section 6:

   IANA has added the following "Type 4 - Parameter Problem" message to
   the "Internet Control Message Protocol version 6 (ICMPv6) Parameters"
   registry:

      CODE     NAME/DESCRIPTION
       3       IPv6 First Fragment has incomplete IPv6 Header Chain

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/uapi/linux/icmpv6.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
index c1661febc2dc..0564fd7ccde4 100644
--- a/include/uapi/linux/icmpv6.h
+++ b/include/uapi/linux/icmpv6.h
@@ -138,6 +138,7 @@ struct icmp6hdr {
 #define ICMPV6_HDR_FIELD		0
 #define ICMPV6_UNK_NEXTHDR		1
 #define ICMPV6_UNK_OPTION		2
+#define ICMPV6_HDR_INCOMP		3
 
 /*
  *	constants for (set|get)sockopt
-- 
2.25.4

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

[Hangbin Liu]

Based on 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.

As the packet may be any kind of L4 protocol, I only checked some common
protocols' header length and handle others by (offset + 1) > skb->len.
Checking each packet header in IPv6 fast path will have performance impact,
so I put the checking in ipv6_frag_rcv().

When send ICMP error message, if the 1st truncated fragment is ICMP message,
icmp6_send() will break as is_ineligible() return true. So I added a check
in is_ineligible() to let fragment packet with nexthdr ICMP but no ICMP header
return false.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v5:
Only check nexthdr if ipv6_skip_exthdr() does not return -1. For
IPPROTO_NONE/NEXTHDR_NONE, later code will handle and ignore it.

v4:
remove unused variable

v3:
a) use frag_off to check if this is a fragment packet
b) check some common protocols' header length

v2:
a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
   return value
b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()

---
 net/ipv6/icmp.c       |  8 +++++++-
 net/ipv6/reassembly.c | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index ec448b71bf9a..8956144ea65e 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -158,7 +158,13 @@ static bool is_ineligible(const struct sk_buff *skb)
 		tp = skb_header_pointer(skb,
 			ptr+offsetof(struct icmp6hdr, icmp6_type),
 			sizeof(_type), &_type);
-		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
+
+		/* Based on RFC 8200, Section 4.5 Fragment Header, return
+		 * false if this is a fragment packet with no icmp header info.
+		 */
+		if (!tp && frag_off != 0)
+			return false;
+		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
 			return true;
 	}
 	return false;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 1f5d4d196dcc..effe1d086e5d 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -42,6 +42,8 @@
 #include <linux/skbuff.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
 
 #include <net/sock.h>
 #include <net/snmp.h>
@@ -322,7 +324,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 	struct frag_queue *fq;
 	const struct ipv6hdr *hdr = ipv6_hdr(skb);
 	struct net *net = dev_net(skb_dst(skb)->dev);
-	int iif;
+	__be16 frag_off;
+	int iif, offset;
+	u8 nexthdr;
 
 	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
 		goto fail_hdr;
@@ -351,6 +355,33 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 		return 1;
 	}
 
+	/* 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) {
+			__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) {
-- 
2.25.4

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

[Georg Kohmann (geokohma)]

On 27.10.2020 03:28, Hangbin Liu wrote:
> Based on 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.
>
> As the packet may be any kind of L4 protocol, I only checked some common
> protocols' header length and handle others by (offset + 1) > skb->len.
> Checking each packet header in IPv6 fast path will have performance impact,
> so I put the checking in ipv6_frag_rcv().
>
> When send ICMP error message, if the 1st truncated fragment is ICMP message,
> icmp6_send() will break as is_ineligible() return true. So I added a check
> in is_ineligible() to let fragment packet with nexthdr ICMP but no ICMP header
> return false.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> v5:
> Only check nexthdr if ipv6_skip_exthdr() does not return -1. For
> IPPROTO_NONE/NEXTHDR_NONE, later code will handle and ignore it.
>
> v4:
> remove unused variable
>
> v3:
> a) use frag_off to check if this is a fragment packet
> b) check some common protocols' header length
>
> v2:
> a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
>    return value
> b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()
>
> ---
>  net/ipv6/icmp.c       |  8 +++++++-
>  net/ipv6/reassembly.c | 33 ++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index ec448b71bf9a..8956144ea65e 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -158,7 +158,13 @@ static bool is_ineligible(const struct sk_buff *skb)
>  		tp = skb_header_pointer(skb,
>  			ptr+offsetof(struct icmp6hdr, icmp6_type),
>  			sizeof(_type), &_type);
> -		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
> +
> +		/* Based on RFC 8200, Section 4.5 Fragment Header, return
> +		 * false if this is a fragment packet with no icmp header info.
> +		 */
> +		if (!tp && frag_off != 0)
> +			return false;
> +		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
>  			return true;
>  	}
>  	return false;
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 1f5d4d196dcc..effe1d086e5d 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -42,6 +42,8 @@
>  #include <linux/skbuff.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
> +#include <linux/tcp.h>
> +#include <linux/udp.h>
>  
>  #include <net/sock.h>
>  #include <net/snmp.h>
> @@ -322,7 +324,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>  	struct frag_queue *fq;
>  	const struct ipv6hdr *hdr = ipv6_hdr(skb);
>  	struct net *net = dev_net(skb_dst(skb)->dev);
> -	int iif;
> +	__be16 frag_off;
> +	int iif, offset;
> +	u8 nexthdr;
>  
>  	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
>  		goto fail_hdr;
> @@ -351,6 +355,33 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>  		return 1;
>  	}
>  
> +	/* 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. I suggest you only check that the offset is 0:

frag_off & htons(IP6_OFFSET)

Sorry for not commenting on this earlier.

> +			__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) {

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

[Hangbin Liu]

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.

> I suggest you only check that the offset is 0:
> frag_off & htons(IP6_OFFSET)

This will match all other fragment packets. RFC request we reply ICMP for the
first fragment packet, Do you mean

if (!frag_off & htons(IP6_OFFSET) && offset > skb->len)

Thanks
Hangbin

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

[Georg Kohmann (geokohma)]

On 27.10.2020 10:57, Hangbin Liu 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.
>
>> I suggest you only check that the offset is 0:
>> frag_off & htons(IP6_OFFSET)
> This will match all other fragment packets. RFC request we reply ICMP for the
> first fragment packet, Do you mean
>
> if (!frag_off & htons(IP6_OFFSET) && offset > skb->len)

Almost, add some parentheses:

if (!(frag_off & htons(IP6_OFFSET)) && offset > skb->len)

>
> Thanks
> Hangbin

[PATCHv6 net 0/2] IPv6: reply ICMP error if fragment doesn't contain all headers

[Hangbin Liu]

When our Engineer run latest IPv6 Core Conformance test, test v6LC.1.3.6:
First Fragment Doesn’t Contain All Headers[1] failed. The test purpose is to
verify that the node(Linux for example) should properly process IPv6 packets
that don’t include all the headers through the Upper-Layer header.

Based on 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.

The first patch add a definition for ICMPv6 Parameter Problem, code 3.
The second patch add a check for the 1st fragment packet to make sure
Upper-Layer header exist.

[1] Page 68, v6LC.1.3.6: First Fragment Doesn’t Contain All Headers part A, B,
C and D at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf
[2] My reproducer:
#!/usr/bin/env python3

import sys, os
from scapy.all import *

# Test v6LC.1.3.6: First Fragment Doesn’t Contain All Headers
def send_frag_dst_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 60, m = 1)
    dst_opt = IPv6ExtHdrDestOpt(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 1)

    pkt_1 = ip6/frag_1/dst_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

def send_frag_route_opt(src_ip6, dst_ip6):
    ip6 = IPv6(src = src_ip6, dst = dst_ip6, nh = 44)

    frag_1 = IPv6ExtHdrFragment(nh = 43, m = 1)
    route_opt = IPv6ExtHdrRouting(nh = 58)

    frag_2 = IPv6ExtHdrFragment(nh = 58, offset = 4, m = 1)
    icmp_echo = ICMPv6EchoRequest(seq = 2)

    pkt_1 = ip6/frag_1/route_opt
    pkt_2 = ip6/frag_2/icmp_echo

    send(pkt_1)
    send(pkt_2)

if __name__ == '__main__':
    src = sys.argv[1]
    dst = sys.argv[2]
    conf.iface = sys.argv[3]
    send_frag_dst_opt(src, dst)
    send_frag_route_opt(src, dst)

Hangbin Liu (2):
  ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition
  IPv6: reply ICMP error if the first fragment don't include all headers

 include/uapi/linux/icmpv6.h |  1 +
 net/ipv6/icmp.c             |  8 +++++++-
 net/ipv6/reassembly.c       | 33 ++++++++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

-- 
2.25.4

[PATCHv6 net 1/2] ICMPv6: Add ICMPv6 Parameter Problem, code 3 definition

[Hangbin Liu]

Based on RFC7112, Section 6:

   IANA has added the following "Type 4 - Parameter Problem" message to
   the "Internet Control Message Protocol version 6 (ICMPv6) Parameters"
   registry:

      CODE     NAME/DESCRIPTION
       3       IPv6 First Fragment has incomplete IPv6 Header Chain

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/uapi/linux/icmpv6.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
index c1661febc2dc..0564fd7ccde4 100644
--- a/include/uapi/linux/icmpv6.h
+++ b/include/uapi/linux/icmpv6.h
@@ -138,6 +138,7 @@ struct icmp6hdr {
 #define ICMPV6_HDR_FIELD		0
 #define ICMPV6_UNK_NEXTHDR		1
 #define ICMPV6_UNK_OPTION		2
+#define ICMPV6_HDR_INCOMP		3
 
 /*
  *	constants for (set|get)sockopt
-- 
2.25.4

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

[Hangbin Liu]

Based on 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.

Checking each packet header in IPv6 fast path will have performance impact,
so I put the checking in ipv6_frag_rcv().

As the packet may be any kind of L4 protocol, I only checked some common
protocols' header length and handle others by (offset + 1) > skb->len.
Also use !(frag_off & htons(IP6_OFFSET)) to catch atomic fragments
(fragmented packet with only one fragment).

When send ICMP error message, if the 1st truncated fragment is ICMP message,
icmp6_send() will break as is_ineligible() return true. So I added a check
in is_ineligible() to let fragment packet with nexthdr ICMP but no ICMP header
return false.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---

v6:
Use !(frag_off & htons(IP6_OFFSET)) to catch atomic fragments

v5:
Only check nexthdr if ipv6_skip_exthdr() does not return -1. For
IPPROTO_NONE/NEXTHDR_NONE, later code will handle and ignore it.

v4:
remove unused variable

v3:
a) use frag_off to check if this is a fragment packet
b) check some common protocols' header length

v2:
a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
   return value
b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()

---
 net/ipv6/icmp.c       |  8 +++++++-
 net/ipv6/reassembly.c | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index ec448b71bf9a..8956144ea65e 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -158,7 +158,13 @@ static bool is_ineligible(const struct sk_buff *skb)
 		tp = skb_header_pointer(skb,
 			ptr+offsetof(struct icmp6hdr, icmp6_type),
 			sizeof(_type), &_type);
-		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
+
+		/* Based on RFC 8200, Section 4.5 Fragment Header, return
+		 * false if this is a fragment packet with no icmp header info.
+		 */
+		if (!tp && frag_off != 0)
+			return false;
+		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
 			return true;
 	}
 	return false;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 1f5d4d196dcc..c8cf1bbad74a 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -42,6 +42,8 @@
 #include <linux/skbuff.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
 
 #include <net/sock.h>
 #include <net/snmp.h>
@@ -322,7 +324,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 	struct frag_queue *fq;
 	const struct ipv6hdr *hdr = ipv6_hdr(skb);
 	struct net *net = dev_net(skb_dst(skb)->dev);
-	int iif;
+	__be16 frag_off;
+	int iif, offset;
+	u8 nexthdr;
 
 	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
 		goto fail_hdr;
@@ -351,6 +355,33 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 		return 1;
 	}
 
+	/* 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_OFFSET)) && 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) {
-- 
2.25.4

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

[Willem de Bruijn]

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.

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

[Georg Kohmann (geokohma)]

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.

Re: [PATCHv6 net 0/2] IPv6: reply ICMP error if fragment doesn't contain all headers

[Jakub Kicinski]

On Tue, 27 Oct 2020 20:33:11 +0800 Hangbin Liu wrote:
> When our Engineer run latest IPv6 Core Conformance test, test v6LC.1.3.6:
> First Fragment Doesn’t Contain All Headers[1] failed. The test purpose is to
> verify that the node(Linux for example) should properly process IPv6 packets
> that don’t include all the headers through the Upper-Layer header.
> 
> Based on 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.
> 
> The first patch add a definition for ICMPv6 Parameter Problem, code 3.
> The second patch add a check for the 1st fragment packet to make sure
> Upper-Layer header exist.

Applied, thank you!