| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 22 Jan 2019 18:13:56 -0800
From: Tom Herbert <tom@...bertland.com>
To: Peter Oskolkov <posk@...gle.com>
Cc: David Miller <davem@...emloft.net>,
Linux Kernel Network Developers <netdev@...r.kernel.org>,
Peter Oskolkov <posk.devel@...il.com>,
Eric Dumazet <edumazet@...gle.com>,
Florian Westphal <fw@...len.de>
Subject: Re: [PATCH net-next 2/4] net: IP6 defrag: use rbtrees for IPv6 defrag
On Tue, Jan 22, 2019 at 5:13 PM Peter Oskolkov <posk@...gle.com> wrote:
>
> On Tue, Jan 22, 2019 at 5:04 PM Tom Herbert <tom@...bertland.com> wrote:
> >
> > On Tue, Jan 22, 2019 at 4:53 PM Peter Oskolkov <posk@...gle.com> wrote:
> > >
> > > On Tue, Jan 22, 2019 at 4:37 PM Tom Herbert <tom@...bertland.com> wrote:
> > > >
> > > > On Tue, Jan 22, 2019 at 10:03 AM Peter Oskolkov <posk@...gle.com> wrote:
> > > > >
> > > > > Currently, IPv6 defragmentation code drops non-last fragments that
> > > > > are smaller than 1280 bytes: see
> > > > > commit 0ed4229b08c1 ("ipv6: defrag: drop non-last frags smaller than min mtu")
> > > > >
> > > > Peter,
> > > >
> > > > I'm a bit confused on why so much code is required to undo this patch
> > > > which was a whole three lines of code to begin with.
> > >
> > > I agree that if the goal is to just lower the 1280 IPv6 fragment
> > > limit, but keep it reasonably high, then my patchset is not needed.
> > > However, if the decision (ultimately by David Miller, I guess) is to
> > > accept fragments of any size, as the standard seems to imply, then
> > > something similar to what I suggested here is needed, I think. This
> > > was done in IPv4...
> >
> > Peter,
> >
> > Sorry, I'm still not following. Before "ipv6: defrag: drop non-last
> > frags smaller than min mtu" wouldn't we have accepted fragments of any
> > size? Are you saying that things were previously broken otherwise?
>
> Yes, before the patch you reference above, fragments of any size had
> been accepted. And this had been identified as a DDOS threat and
> mitigated in IPv6 by setting the 1280 fragment limit and in IPv4 by
> using rbtrees. The IPv4/IPv6 difference was due to the belief, at the
> time, that IPv6 standard actually required non-last fragments less
> than 1280 to be dropped, while IPv4 standard required accepting
> fragments of any size.
>
> So things were broken in the sense of having this DDOS vulnerability,
> and the vulnerability was fixed in IPv4 and IPv6 using different
> approaches...
I see, thanks for the explanation! Even if there is a limit on
fragment size, it should be configurable so your patches would still
be relevant. As for eliminating the limit completely, I think that
question is what is the remaining DOSability with a tiny fragment
attack. Have you done any analysis that might shed light on that?
Tom
>
> Thanks,
> Peter
>
> >
> > Tom
> >
> > >
> > > Thanks,
> > > Peter
> > >
> > > >
> > > > > This behavior is not specified in IPv6 RFCs and appears to break
> > > > > compatibility with some IPv6 implemenations, as reported here:
> > > > > https://www.spinics.net/lists/netdev/msg543846.html
> > > > >
> > > > I am not yet convinced that the patch should be reverted. While it is
> > > > not conformant, it is a potential mitigation against DOS attack. For
> > > > instance, a single fragmented packet could be composed of over 8000
> > > > fragments with the minimum fragment size of eight bytes. Receiving
> > > > packets like that attacks both memory and CPU, and I don't see any
> > > > purpose to sending tiny fragments other than DOS attack. For practical
> > > > implemenation, I believe a limit is justified. The 1280 MTU limit in
> > > > the patch was too austere, I think it should be smaller than that.
> > > > Also, any such limit should apply to fragments payload length and not
> > > > length of the packet since a clever attacker could stuff the packet
> > > > with various other extension headers and still end up sending tiny
> > > > fragments. I have a patch along these lines with limit configurable by
> > > > sysctl. I will post shortly.
> > > >
> > > > Tom
> > > >
> > > > > This patch re-uses common IP defragmentation queueing and reassembly
> > > > > code in IPv6, removing the 1280 byte restriction.
> > > > >
> > > > > Signed-off-by: Peter Oskolkov <posk@...gle.com>
> > > > > Reported-by: Tom Herbert <tom@...bertland.com>
> > > > > Cc: Eric Dumazet <edumazet@...gle.com>
> > > > > Cc: Florian Westphal <fw@...len.de>
> > > > > ---
> > > > > include/net/ipv6_frag.h | 11 +-
> > > > > net/ipv6/reassembly.c | 233 +++++++++++-----------------------------
> > > > > 2 files changed, 71 insertions(+), 173 deletions(-)
> > > > >
> > > > > diff --git a/include/net/ipv6_frag.h b/include/net/ipv6_frag.h
> > > > > index 6ced1e6899b6..28aa9b30aece 100644
> > > > > --- a/include/net/ipv6_frag.h
> > > > > +++ b/include/net/ipv6_frag.h
> > > > > @@ -82,8 +82,15 @@ ip6frag_expire_frag_queue(struct net *net, struct frag_queue *fq)
> > > > > __IP6_INC_STATS(net, __in6_dev_get(dev), IPSTATS_MIB_REASMTIMEOUT);
> > > > >
> > > > > /* Don't send error if the first segment did not arrive. */
> > > > > - head = fq->q.fragments;
> > > > > - if (!(fq->q.flags & INET_FRAG_FIRST_IN) || !head)
> > > > > + if (!(fq->q.flags & INET_FRAG_FIRST_IN))
> > > > > + goto out;
> > > > > +
> > > > > + /* sk_buff::dev and sk_buff::rbnode are unionized. So we
> > > > > + * pull the head out of the tree in order to be able to
> > > > > + * deal with head->dev.
> > > > > + */
> > > > > + head = inet_frag_pull_head(&fq->q);
> > > > > + if (!head)
> > > > > goto out;
> > > > >
> > > > > head->dev = dev;
> > > > > diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> > > > > index 36a3d8dc61f5..24264d0a4b85 100644
> > > > > --- a/net/ipv6/reassembly.c
> > > > > +++ b/net/ipv6/reassembly.c
> > > > > @@ -69,8 +69,8 @@ static u8 ip6_frag_ecn(const struct ipv6hdr *ipv6h)
> > > > >
> > > > > static struct inet_frags ip6_frags;
> > > > >
> > > > > -static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
> > > > > - struct net_device *dev);
> > > > > +static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *skb,
> > > > > + struct sk_buff *prev_tail, struct net_device *dev);
> > > > >
> > > > > static void ip6_frag_expire(struct timer_list *t)
> > > > > {
> > > > > @@ -111,21 +111,26 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
> > > > > struct frag_hdr *fhdr, int nhoff,
> > > > > u32 *prob_offset)
> > > > > {
> > > > > - struct sk_buff *prev, *next;
> > > > > - struct net_device *dev;
> > > > > - int offset, end, fragsize;
> > > > > struct net *net = dev_net(skb_dst(skb)->dev);
> > > > > + int offset, end, fragsize;
> > > > > + struct sk_buff *prev_tail;
> > > > > + struct net_device *dev;
> > > > > + int err = -ENOENT;
> > > > > u8 ecn;
> > > > >
> > > > > if (fq->q.flags & INET_FRAG_COMPLETE)
> > > > > goto err;
> > > > >
> > > > > + err = -EINVAL;
> > > > > offset = ntohs(fhdr->frag_off) & ~0x7;
> > > > > end = offset + (ntohs(ipv6_hdr(skb)->payload_len) -
> > > > > ((u8 *)(fhdr + 1) - (u8 *)(ipv6_hdr(skb) + 1)));
> > > > >
> > > > > if ((unsigned int)end > IPV6_MAXPLEN) {
> > > > > *prob_offset = (u8 *)&fhdr->frag_off - skb_network_header(skb);
> > > > > + /* note that if prob_offset is set, the skb is freed elsewhere,
> > > > > + * we do not free it here.
> > > > > + */
> > > > > return -1;
> > > > > }
> > > > >
> > > > > @@ -170,62 +175,27 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
> > > > > if (end == offset)
> > > > > goto discard_fq;
> > > > >
> > > > > + err = -ENOMEM;
> > > > > /* Point into the IP datagram 'data' part. */
> > > > > if (!pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data))
> > > > > goto discard_fq;
> > > > >
> > > > > - if (pskb_trim_rcsum(skb, end - offset))
> > > > > + err = pskb_trim_rcsum(skb, end - offset);
> > > > > + if (err)
> > > > > goto discard_fq;
> > > > >
> > > > > - /* Find out which fragments are in front and at the back of us
> > > > > - * in the chain of fragments so far. We must know where to put
> > > > > - * this fragment, right?
> > > > > - */
> > > > > - prev = fq->q.fragments_tail;
> > > > > - if (!prev || prev->ip_defrag_offset < offset) {
> > > > > - next = NULL;
> > > > > - goto found;
> > > > > - }
> > > > > - prev = NULL;
> > > > > - for (next = fq->q.fragments; next != NULL; next = next->next) {
> > > > > - if (next->ip_defrag_offset >= offset)
> > > > > - break; /* bingo! */
> > > > > - prev = next;
> > > > > - }
> > > > > -
> > > > > -found:
> > > > > - /* RFC5722, Section 4, amended by Errata ID : 3089
> > > > > - * When reassembling an IPv6 datagram, if
> > > > > - * one or more its constituent fragments is determined to be an
> > > > > - * overlapping fragment, the entire datagram (and any constituent
> > > > > - * fragments) MUST be silently discarded.
> > > > > - */
> > > > > -
> > > > > - /* Check for overlap with preceding fragment. */
> > > > > - if (prev &&
> > > > > - (prev->ip_defrag_offset + prev->len) > offset)
> > > > > - goto discard_fq;
> > > > > -
> > > > > - /* Look for overlap with succeeding segment. */
> > > > > - if (next && next->ip_defrag_offset < end)
> > > > > - goto discard_fq;
> > > > > -
> > > > > - /* Note : skb->ip_defrag_offset and skb->sk share the same location */
> > > > > + /* Note : skb->rbnode and skb->dev share the same location. */
> > > > > dev = skb->dev;
> > > > > - if (dev)
> > > > > - fq->iif = dev->ifindex;
> > > > > /* Makes sure compiler wont do silly aliasing games */
> > > > > barrier();
> > > > > - skb->ip_defrag_offset = offset;
> > > > >
> > > > > - /* Insert this fragment in the chain of fragments. */
> > > > > - skb->next = next;
> > > > > - if (!next)
> > > > > - fq->q.fragments_tail = skb;
> > > > > - if (prev)
> > > > > - prev->next = skb;
> > > > > - else
> > > > > - fq->q.fragments = skb;
> > > > > + prev_tail = fq->q.fragments_tail;
> > > > > + err = inet_frag_queue_insert(&fq->q, skb, offset, end);
> > > > > + if (err)
> > > > > + goto insert_error;
> > > > > +
> > > > > + if (dev)
> > > > > + fq->iif = dev->ifindex;
> > > > >
> > > > > fq->q.stamp = skb->tstamp;
> > > > > fq->q.meat += skb->len;
> > > > > @@ -246,44 +216,48 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
> > > > >
> > > > > if (fq->q.flags == (INET_FRAG_FIRST_IN | INET_FRAG_LAST_IN) &&
> > > > > fq->q.meat == fq->q.len) {
> > > > > - int res;
> > > > > unsigned long orefdst = skb->_skb_refdst;
> > > > >
> > > > > skb->_skb_refdst = 0UL;
> > > > > - res = ip6_frag_reasm(fq, prev, dev);
> > > > > + err = ip6_frag_reasm(fq, skb, prev_tail, dev);
> > > > > skb->_skb_refdst = orefdst;
> > > > > - return res;
> > > > > + return err;
> > > > > }
> > > > >
> > > > > skb_dst_drop(skb);
> > > > > - return -1;
> > > > > + return -EINPROGRESS;
> > > > >
> > > > > +insert_error:
> > > > > + if (err == IPFRAG_DUP) {
> > > > > + kfree_skb(skb);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > + err = -EINVAL;
> > > > > + __IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
> > > > > + IPSTATS_MIB_REASM_OVERLAPS);
> > > > > discard_fq:
> > > > > inet_frag_kill(&fq->q);
> > > > > -err:
> > > > > __IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
> > > > > IPSTATS_MIB_REASMFAILS);
> > > > > +err:
> > > > > kfree_skb(skb);
> > > > > - return -1;
> > > > > + return err;
> > > > > }
> > > > >
> > > > > /*
> > > > > * Check if this packet is complete.
> > > > > - * Returns NULL on failure by any reason, and pointer
> > > > > - * to current nexthdr field in reassembled frame.
> > > > > *
> > > > > * It is called with locked fq, and caller must check that
> > > > > * queue is eligible for reassembly i.e. it is not COMPLETE,
> > > > > * the last and the first frames arrived and all the bits are here.
> > > > > */
> > > > > -static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
> > > > > - struct net_device *dev)
> > > > > +static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *skb,
> > > > > + struct sk_buff *prev_tail, struct net_device *dev)
> > > > > {
> > > > > struct net *net = container_of(fq->q.net, struct net, ipv6.frags);
> > > > > - struct sk_buff *fp, *head = fq->q.fragments;
> > > > > - int payload_len, delta;
> > > > > unsigned int nhoff;
> > > > > - int sum_truesize;
> > > > > + void *reasm_data;
> > > > > + int payload_len;
> > > > > u8 ecn;
> > > > >
> > > > > inet_frag_kill(&fq->q);
> > > > > @@ -292,121 +266,40 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
> > > > > if (unlikely(ecn == 0xff))
> > > > > goto out_fail;
> > > > >
> > > > > - /* Make the one we just received the head. */
> > > > > - if (prev) {
> > > > > - head = prev->next;
> > > > > - fp = skb_clone(head, GFP_ATOMIC);
> > > > > -
> > > > > - if (!fp)
> > > > > - goto out_oom;
> > > > > -
> > > > > - fp->next = head->next;
> > > > > - if (!fp->next)
> > > > > - fq->q.fragments_tail = fp;
> > > > > - prev->next = fp;
> > > > > -
> > > > > - skb_morph(head, fq->q.fragments);
> > > > > - head->next = fq->q.fragments->next;
> > > > > -
> > > > > - consume_skb(fq->q.fragments);
> > > > > - fq->q.fragments = head;
> > > > > - }
> > > > > -
> > > > > - WARN_ON(head == NULL);
> > > > > - WARN_ON(head->ip_defrag_offset != 0);
> > > > > + reasm_data = inet_frag_reasm_prepare(&fq->q, skb, prev_tail);
> > > > > + if (!reasm_data)
> > > > > + goto out_oom;
> > > > >
> > > > > - /* Unfragmented part is taken from the first segment. */
> > > > > - payload_len = ((head->data - skb_network_header(head)) -
> > > > > + payload_len = ((skb->data - skb_network_header(skb)) -
> > > > > sizeof(struct ipv6hdr) + fq->q.len -
> > > > > sizeof(struct frag_hdr));
> > > > > if (payload_len > IPV6_MAXPLEN)
> > > > > goto out_oversize;
> > > > >
> > > > > - delta = - head->truesize;
> > > > > -
> > > > > - /* Head of list must not be cloned. */
> > > > > - if (skb_unclone(head, GFP_ATOMIC))
> > > > > - goto out_oom;
> > > > > -
> > > > > - delta += head->truesize;
> > > > > - if (delta)
> > > > > - add_frag_mem_limit(fq->q.net, delta);
> > > > > -
> > > > > - /* If the first fragment is fragmented itself, we split
> > > > > - * it to two chunks: the first with data and paged part
> > > > > - * and the second, holding only fragments. */
> > > > > - if (skb_has_frag_list(head)) {
> > > > > - struct sk_buff *clone;
> > > > > - int i, plen = 0;
> > > > > -
> > > > > - clone = alloc_skb(0, GFP_ATOMIC);
> > > > > - if (!clone)
> > > > > - goto out_oom;
> > > > > - clone->next = head->next;
> > > > > - head->next = clone;
> > > > > - skb_shinfo(clone)->frag_list = skb_shinfo(head)->frag_list;
> > > > > - skb_frag_list_init(head);
> > > > > - for (i = 0; i < skb_shinfo(head)->nr_frags; i++)
> > > > > - plen += skb_frag_size(&skb_shinfo(head)->frags[i]);
> > > > > - clone->len = clone->data_len = head->data_len - plen;
> > > > > - head->data_len -= clone->len;
> > > > > - head->len -= clone->len;
> > > > > - clone->csum = 0;
> > > > > - clone->ip_summed = head->ip_summed;
> > > > > - add_frag_mem_limit(fq->q.net, clone->truesize);
> > > > > - }
> > > > > -
> > > > > /* We have to remove fragment header from datagram and to relocate
> > > > > * header in order to calculate ICV correctly. */
> > > > > nhoff = fq->nhoffset;
> > > > > - skb_network_header(head)[nhoff] = skb_transport_header(head)[0];
> > > > > - memmove(head->head + sizeof(struct frag_hdr), head->head,
> > > > > - (head->data - head->head) - sizeof(struct frag_hdr));
> > > > > - if (skb_mac_header_was_set(head))
> > > > > - head->mac_header += sizeof(struct frag_hdr);
> > > > > - head->network_header += sizeof(struct frag_hdr);
> > > > > -
> > > > > - skb_reset_transport_header(head);
> > > > > - skb_push(head, head->data - skb_network_header(head));
> > > > > -
> > > > > - sum_truesize = head->truesize;
> > > > > - for (fp = head->next; fp;) {
> > > > > - bool headstolen;
> > > > > - int delta;
> > > > > - struct sk_buff *next = fp->next;
> > > > > -
> > > > > - sum_truesize += fp->truesize;
> > > > > - if (head->ip_summed != fp->ip_summed)
> > > > > - head->ip_summed = CHECKSUM_NONE;
> > > > > - else if (head->ip_summed == CHECKSUM_COMPLETE)
> > > > > - head->csum = csum_add(head->csum, fp->csum);
> > > > > -
> > > > > - if (skb_try_coalesce(head, fp, &headstolen, &delta)) {
> > > > > - kfree_skb_partial(fp, headstolen);
> > > > > - } else {
> > > > > - fp->sk = NULL;
> > > > > - if (!skb_shinfo(head)->frag_list)
> > > > > - skb_shinfo(head)->frag_list = fp;
> > > > > - head->data_len += fp->len;
> > > > > - head->len += fp->len;
> > > > > - head->truesize += fp->truesize;
> > > > > - }
> > > > > - fp = next;
> > > > > - }
> > > > > - sub_frag_mem_limit(fq->q.net, sum_truesize);
> > > > > + skb_network_header(skb)[nhoff] = skb_transport_header(skb)[0];
> > > > > + memmove(skb->head + sizeof(struct frag_hdr), skb->head,
> > > > > + (skb->data - skb->head) - sizeof(struct frag_hdr));
> > > > > + if (skb_mac_header_was_set(skb))
> > > > > + skb->mac_header += sizeof(struct frag_hdr);
> > > > > + skb->network_header += sizeof(struct frag_hdr);
> > > > > +
> > > > > + skb_reset_transport_header(skb);
> > > > > +
> > > > > + inet_frag_reasm_finish(&fq->q, skb, reasm_data);
> > > > >
> > > > > - skb_mark_not_on_list(head);
> > > > > - head->dev = dev;
> > > > > - head->tstamp = fq->q.stamp;
> > > > > - ipv6_hdr(head)->payload_len = htons(payload_len);
> > > > > - ipv6_change_dsfield(ipv6_hdr(head), 0xff, ecn);
> > > > > - IP6CB(head)->nhoff = nhoff;
> > > > > - IP6CB(head)->flags |= IP6SKB_FRAGMENTED;
> > > > > - IP6CB(head)->frag_max_size = fq->q.max_size;
> > > > > + skb->dev = dev;
> > > > > + ipv6_hdr(skb)->payload_len = htons(payload_len);
> > > > > + ipv6_change_dsfield(ipv6_hdr(skb), 0xff, ecn);
> > > > > + IP6CB(skb)->nhoff = nhoff;
> > > > > + IP6CB(skb)->flags |= IP6SKB_FRAGMENTED;
> > > > > + IP6CB(skb)->frag_max_size = fq->q.max_size;
> > > > >
> > > > > /* Yes, and fold redundant checksum back. 8) */
> > > > > - skb_postpush_rcsum(head, skb_network_header(head),
> > > > > - skb_network_header_len(head));
> > > > > + skb_postpush_rcsum(skb, skb_network_header(skb),
> > > > > + skb_network_header_len(skb));
> > > > >
> > > > > rcu_read_lock();
> > > > > __IP6_INC_STATS(net, __in6_dev_get(dev), IPSTATS_MIB_REASMOKS);
> > > > > @@ -414,6 +307,7 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
> > > > > fq->q.fragments = NULL;
> > > > > fq->q.rb_fragments = RB_ROOT;
> > > > > fq->q.fragments_tail = NULL;
> > > > > + fq->q.last_run_head = NULL;
> > > > > return 1;
> > > > >
> > > > > out_oversize:
> > > > > @@ -464,10 +358,6 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
> > > > > return 1;
> > > > > }
> > > > >
> > > > > - if (skb->len - skb_network_offset(skb) < IPV6_MIN_MTU &&
> > > > > - fhdr->frag_off & htons(IP6_MF))
> > > > > - goto fail_hdr;
> > > > > -
> > > > > iif = skb->dev ? skb->dev->ifindex : 0;
> > > > > fq = fq_find(net, fhdr->identification, hdr, iif);
> > > > > if (fq) {
> > > > > @@ -485,6 +375,7 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
> > > > > if (prob_offset) {
> > > > > __IP6_INC_STATS(net, __in6_dev_get_safely(skb->dev),
> > > > > IPSTATS_MIB_INHDRERRORS);
> > > > > + /* icmpv6_param_prob() calls kfree_skb(skb) */
> > > > > icmpv6_param_prob(skb, ICMPV6_HDR_FIELD, prob_offset);
> > > > > }
> > > > > return ret;
> > > > > --
> > > > > 2.20.1.321.g9e740568ce-goog
> > > > >
Powered by blists - more mailing lists