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

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

[Zubin Mithra]

From: Peter Oskolkov <posk@google.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@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Zubin Mithra <zsm@chromium.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

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

[Greg KH]

On Tue, Jan 22, 2019 at 09:43:44AM -0800, Zubin Mithra wrote:
> From: Peter Oskolkov <posk@google.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@google.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Zubin Mithra <zsm@chromium.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)

This patch does not apply to the current 4.19.y tree (well, on top of my
latest patches that are queued for the next release).

Can you refresh it after the next 4.19.y release in a few days and
resend it along with a new 4.14.y patch as well?

thanks,

greg k-h