Message-ID: <u0QFePiYSfxBeUsNVFRhPjsGViwg-pXLIApJaVLdUICuvLTQg5y5-rdNhh9lPcDsyO24c7wXxy5m6b6dK0aB6kqR0ypk8X9ekiLe3NQ3ICY=@protonmail.com>
Date:   Tue, 31 Dec 2019 01:19:27 +0000
From:   Ttttabcd <ttttabcd@...tonmail.com>
To:     Netdev <netdev@...r.kernel.org>
Cc:     David Miller <davem@...emloft.net>,
        "kuznet@....inr.ac.ru" <kuznet@....inr.ac.ru>,
        "yoshfuji@...ux-ipv6.org" <yoshfuji@...ux-ipv6.org>
Subject: [PATCH] fragment: Improved handling of incorrect IP fragments

In both ip6_frag_queue and ip6_frag_reasm, it is checked whether it is an
Oversized IPv6 packet, which is duplicated. The original code logic will
only be processed in ip6_frag_queue. The code of ip6_frag_reasm will not
be executed. Now change it to only process in ip6_frag_queue and output
the prompt information.

In ip6_frag_queue, set the prob_offset pointer to notify ipv6_frag_rcv
to send the ICMPv6 parameter problem message. The logic is not concise,
I merged the part that sent ICMPv6 messages into ip6_frag_queue.

In the original logic, receiving oversized IPv6 fragments and receiving
IPv6 fragments whose end is not an integral multiple of 8 bytes both
returns -1. This is inconsistent with other incorrect IPv6 fragmentation
processing. For example, in other logic, end == offset will goto discard_fq
directly. I think that receiving any incorrect IPv6 fragment means that the
fragment processing has failed as a whole, and the data carried in the
fragments is likely to be wrong. I think we should also execute goto
discard_fq to release this fragments queue.

Goto discard_fq at the end of the label send_param_prob, release the
fragment queue. Since icmpv6_param_prob will call kfree_skb, it will also
be kfree_skb in the label err, which will cause repeated free,
so I added skb_get.

I also made similar changes in IPv4 fragmentation processing.

It is not good to use 65535 values directly,
I added the IPV4_MAX_TOT_LEN macro.

The oversized check in IPv4 fragment processing is in the ip_frag_reasm
of the reassembly fragment. This is too late. The incorrect IP fragment
has been inserted into the fragment queue. I modified it in ip_frag_queue.
I changed the original net_info_ratelimited to net_dbg_ratelimited to make
the debugging information more controllable.

I also modified goto discard_qp directly
when the end is not an integer multiple of 8 bytes.

Signed-off-by: AK Deng <ttttabcd@...tonmail.com>
---
 include/net/ip.h       |  2 ++
 net/ipv4/ip_fragment.c | 20 +++++++++----------
 net/ipv6/reassembly.c  | 45 ++++++++++++++++++++----------------------
 3 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 5b317c9f4470..43e9dc51852b 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -34,6 +34,8 @@
 #define IPV4_MAX_PMTU		65535U		/* RFC 2675, Section 5.1 */
 #define IPV4_MIN_MTU		68			/* RFC 791 */

+#define IPV4_MAX_TOT_LEN		65535
+
 extern unsigned int sysctl_fib_sync_mem;
 extern unsigned int sysctl_fib_sync_mem_min;
 extern unsigned int sysctl_fib_sync_mem_max;
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index cfeb8890f94e..baee3383d0ac 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -300,6 +300,12 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 	end = offset + skb->len - skb_network_offset(skb) - ihl;
 	err = -EINVAL;

+	if ((unsigned int)end + ihl > IPV4_MAX_TOT_LEN) {
+		net_dbg_ratelimited("ip_frag_queue: Oversized IP packet from %pI4, end = %d\n",
+				    &qp->q.key.v4.saddr, end);
+		goto discard_qp;
+	}
+
 	/* Is this the final fragment? */
 	if ((flags & IP_MF) == 0) {
 		/* If we already have some bits beyond end
@@ -311,11 +317,10 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 		qp->q.flags |= INET_FRAG_LAST_IN;
 		qp->q.len = end;
 	} else {
-		if (end&7) {
-			end &= ~7;
-			if (skb->ip_summed != CHECKSUM_UNNECESSARY)
-				skb->ip_summed = CHECKSUM_NONE;
-		}
+		/* Check if the fragment is rounded to 8 bytes. */
+		if (end & 0x7)
+			goto discard_qp;
+
 		if (end > qp->q.len) {
 			/* Some bits beyond end -> corruption. */
 			if (qp->q.flags & INET_FRAG_LAST_IN)
@@ -423,8 +428,6 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,

 	len = ip_hdrlen(skb) + qp->q.len;
 	err = -E2BIG;
-	if (len > 65535)
-		goto out_oversize;

 	inet_frag_reasm_finish(&qp->q, skb, reasm_data,
 			       ip_frag_coalesce_ok(qp));
@@ -462,9 +465,6 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
 out_nomem:
 	net_dbg_ratelimited("queue_glue: no memory for gluing queue %p\n", qp);
 	err = -ENOMEM;
-	goto out_fail;
-out_oversize:
-	net_info_ratelimited("Oversized IP packet from %pI4\n", &qp->q.key.v4.saddr);
 out_fail:
 	__IP_INC_STATS(net, IPSTATS_MIB_REASMFAILS);
 	return err;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 1f5d4d196dcc..302bf6c26c45 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -102,13 +102,13 @@ fq_find(struct net *net, __be32 id, const struct ipv6hdr *hdr, int iif)
 }

 static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
-			  struct frag_hdr *fhdr, int nhoff,
-			  u32 *prob_offset)
+			  struct frag_hdr *fhdr, int nhoff)
 {
 	struct net *net = dev_net(skb_dst(skb)->dev);
 	int offset, end, fragsize;
 	struct sk_buff *prev_tail;
 	struct net_device *dev;
+	u32 prob_offset = 0;
 	int err = -ENOENT;
 	u8 ecn;

@@ -121,11 +121,10 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
 			((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;
+		prob_offset = (u8 *)&fhdr->frag_off - skb_network_header(skb);
+		net_dbg_ratelimited("ip6_frag_queue: Oversized IPv6 packet from %pI6c, end = %d\n",
+				    &fq->q.key.v6.saddr, end);
+		goto send_param_prob;
 	}

 	ecn = ip6_frag_ecn(ipv6_hdr(skb));
@@ -155,8 +154,8 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
 			/* RFC2460 says always send parameter problem in
 			 * this case. -DaveM
 			 */
-			*prob_offset = offsetof(struct ipv6hdr, payload_len);
-			return -1;
+			prob_offset = offsetof(struct ipv6hdr, payload_len);
+			goto send_param_prob;
 		}
 		if (end > fq->q.len) {
 			/* Some bits beyond end -> corruption. */
@@ -236,6 +235,18 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
 err:
 	kfree_skb(skb);
 	return err;
+send_param_prob:
+		__IP6_INC_STATS(net, __in6_dev_get_safely(skb->dev),
+				IPSTATS_MIB_INHDRERRORS);
+		/* icmpv6_param_prob() calls kfree_skb(skb),
+		 * and the label err also calls kfree_skb(skb),
+		 * so skb_get(skb) here increases the reference count
+		 * to avoid duplicate release
+		 */
+		skb_get(skb);
+		icmpv6_param_prob(skb, ICMPV6_HDR_FIELD, prob_offset);
+		err = -1;
+		goto discard_fq;
 }

 /*
@@ -267,8 +278,6 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *skb,
 	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;

 	/* We have to remove fragment header from datagram and to relocate
 	 * header in order to calculate ICV correctly. */
@@ -303,9 +312,6 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *skb,
 	fq->q.last_run_head = NULL;
 	return 1;

-out_oversize:
-	net_dbg_ratelimited("ip6_frag_reasm: payload len = %d\n", payload_len);
-	goto out_fail;
 out_oom:
 	net_dbg_ratelimited("ip6_frag_reasm: no memory for reassembly\n");
 out_fail:
@@ -354,23 +360,14 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
 	iif = skb->dev ? skb->dev->ifindex : 0;
 	fq = fq_find(net, fhdr->identification, hdr, iif);
 	if (fq) {
-		u32 prob_offset = 0;
 		int ret;
-
 		spin_lock(&fq->q.lock);

 		fq->iif = iif;
-		ret = ip6_frag_queue(fq, skb, fhdr, IP6CB(skb)->nhoff,
-				     &prob_offset);
+		ret = ip6_frag_queue(fq, skb, fhdr, IP6CB(skb)->nhoff);

 		spin_unlock(&fq->q.lock);
 		inet_frag_put(&fq->q);
-		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.24.0

Powered by blists - more mailing lists