| Message ID | 20201026072926.3663480-3-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | IPv6: reply ICMP error if fragment doesn't contain all headers | expand |
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?
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
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
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) {
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(-)