netdev - [PATCH v4.19.y] ip: fail fast on IP defrag errors

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <20190122174344.112456-1-zsm@chromium.org>
Date:   Tue, 22 Jan 2019 09:43:44 -0800
From:   Zubin Mithra <zsm@...omium.org>
To:     netdev@...r.kernel.org
Cc:     gregkh@...uxfoundation.org, posk@...gle.com, edumazet@...gle.com,
        willemb@...gle.com, davem@...emloft.net, zsm@...omium.org
Subject: [PATCH v4.19.y] ip: fail fast on IP defrag errors

From: Peter Oskolkov <posk@...gle.com>

commit 0ff89efb524631ac9901b81446b453c29711c376 upstream

The current behavior of IP defragmentation is inconsistent:
- some overlapping/wrong length fragments are dropped without
  affecting the queue;
- most overlapping fragments cause the whole frag queue to be dropped.

This patch brings consistency: if a bad fragment is detected,
the whole frag queue is dropped. Two major benefits:
- fail fast: corrupted frag queues are cleared immediately, instead of
  by timeout;
- testing of overlapping fragments is now much easier: any kind of
  random fragment length mutation now leads to the frag queue being
  discarded (IP packet dropped); before this patch, some overlaps were
  "corrected", with tests not seeing expected packet drops.

Note that in one case (see "if (end&7)" conditional) the current
behavior is preserved as there are concerns that this could be
legitimate padding.

Signed-off-by: Peter Oskolkov <posk@...gle.com>
Reviewed-by: Eric Dumazet <edumazet@...gle.com>
Reviewed-by: Willem de Bruijn <willemb@...gle.com>
Signed-off-by: David S. Miller <davem@...emloft.net>
Signed-off-by: Zubin Mithra <zsm@...omium.org>
---
Backport Note:
- Syzkaller reported a UAF, as 0ff89efb5246 ("ip: fail fast on IP defrag
errors") was not applied prior to applying d5f9565c8d5a ("net: ipv4: do
not handle duplicate fragments as overlapping").
Conflicts occur when 0ff89efb5246 is now applied onto 4.14.y/4.19.y,
which this patch addresses.
- An alternative to this patch would be to do the following :-
    - revert "net: ipv4: do not handle duplicate fragments as overlapping"
      (d5f9565c8d5ad on 4.19.y, 95b4b711444a on 4.14.y)
    - apply "ip: fail fast on IP defrag errors" (0ff89efb5246)
    - apply "net: ipv4: do not handle duplicate fragments as overlapping"
      (ade446403bfb)

 net/ipv4/ip_fragment.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index f8bbd693c19c2..03576ff7557e0 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -382,7 +382,7 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 		 */
 		if (end < qp->q.len ||
 		    ((qp->q.flags & INET_FRAG_LAST_IN) && end != qp->q.len))
-			goto err;
+			goto discard_qp;
 		qp->q.flags |= INET_FRAG_LAST_IN;
 		qp->q.len = end;
 	} else {
@@ -394,20 +394,20 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 		if (end > qp->q.len) {
 			/* Some bits beyond end -> corruption. */
 			if (qp->q.flags & INET_FRAG_LAST_IN)
-				goto err;
+				goto discard_qp;
 			qp->q.len = end;
 		}
 	}
 	if (end == offset)
-		goto err;
+		goto discard_qp;
 
 	err = -ENOMEM;
 	if (!pskb_pull(skb, skb_network_offset(skb) + ihl))
-		goto err;
+		goto discard_qp;
 
 	err = pskb_trim_rcsum(skb, end - offset);
 	if (err)
-		goto err;
+		goto discard_qp;
 
 	/* Note : skb->rbnode and skb->dev share the same location. */
 	dev = skb->dev;
@@ -425,6 +425,7 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 	 * fragment.
 	 */
 
+	err = -EINVAL;
 	/* Find out where to put this fragment.  */
 	prev_tail = qp->q.fragments_tail;
 	if (!prev_tail)
@@ -433,7 +434,7 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 		/* This is the common case: skb goes to the end. */
 		/* Detect and discard overlaps. */
 		if (offset < prev_tail->ip_defrag_offset + prev_tail->len)
-			goto discard_qp;
+			goto overlap;
 		if (offset == prev_tail->ip_defrag_offset + prev_tail->len)
 			ip4_frag_append_to_last_run(&qp->q, skb);
 		else
@@ -456,7 +457,7 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 				 end <= skb1_run_end)
 				goto err; /* No new data, potential duplicate */
 			else
-				goto discard_qp; /* Found an overlap */
+				goto overlap; /* Found an overlap */
 		} while (*rbn);
 		/* Here we have parent properly set, and rbn pointing to
 		 * one of its NULL left/right children. Insert skb.
@@ -493,16 +494,18 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 		skb->_skb_refdst = 0UL;
 		err = ip_frag_reasm(qp, skb, prev_tail, dev);
 		skb->_skb_refdst = orefdst;
+		if (err)
+			inet_frag_kill(&qp->q);
 		return err;
 	}
 
 	skb_dst_drop(skb);
 	return -EINPROGRESS;
 
+overlap:
+	__IP_INC_STATS(net, IPSTATS_MIB_REASM_OVERLAPS);
 discard_qp:
 	inet_frag_kill(&qp->q);
-	err = -EINVAL;
-	__IP_INC_STATS(net, IPSTATS_MIB_REASM_OVERLAPS);
 err:
 	kfree_skb(skb);
 	return err;
-- 
2.20.1.321.g9e740568ce-goog