| Message ID | 20201021042005.736568-3-liuhangbin@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | IPv6: reply ICMP error with fragment doesn't contain all headers | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 6 this patch: 6 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 61 lines checked |
| jkicinski/build_allmodconfig_warn | fail | Errors and warnings before: 6 this patch: 6 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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 >
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
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.
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) {
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(-)