| Message ID | 20201007035502.3928521-3-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | IPv6: reply ICMP error with fragment doesn't contain all headers | expand |
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.
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 *
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
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
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 >
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
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. */
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(-)