On Wed, Oct 21, 2020 at 12:20 AM Hangbin Liu <liuhang...@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 <liuhang...@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
>
