linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
@ 2017-04-25 14:08 Jason A. Donenfeld
  2017-04-25 14:08 ` [PATCH 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
                   ` (6 more replies)
  0 siblings, 7 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 14:08 UTC (permalink / raw)
  To: netdev, linux-kernel, davem, David.Laight, kernel-hardening
  Cc: Jason A. Donenfeld

This is a defense-in-depth measure in response to bugs like
4d6fa57b4dab0d77f4d8e9d9c73d1e63f6fe8fee.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/core/skbuff.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..3c2a7f323722 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3489,7 +3489,9 @@ void __init skb_init(void)
  *	@len: Length of buffer space to be mapped
  *
  *	Fill the specified scatter-gather list with mappings/pointers into a
- *	region of the buffer space attached to a socket buffer.
+ *	region of the buffer space attached to a socket buffer. Returns either
+ *	the number of scatterlist items used, or -EMSGSIZE if the contents
+ *	could not fit.
  */
 static int
 __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
@@ -3512,6 +3514,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
 		int end;
 
+		if (elt && sg_is_last(&sg[elt - 1]))
+			return -EMSGSIZE;
+
 		WARN_ON(start > offset + len);
 
 		end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
@@ -3535,6 +3540,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 
 		WARN_ON(start > offset + len);
 
+		if (elt && sg_is_last(&sg[elt - 1]))
+			return -EMSGSIZE;
+
 		end = start + frag_iter->len;
 		if ((copy = end - offset) > 0) {
 			if (copy > len)
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/5] ipsec: check return value of skb_to_sgvec always
  2017-04-25 14:08 [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
@ 2017-04-25 14:08 ` Jason A. Donenfeld
  2017-04-25 14:08 ` [PATCH 3/5] rxrpc: " Jason A. Donenfeld
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 14:08 UTC (permalink / raw)
  To: netdev, linux-kernel, davem, David.Laight, kernel-hardening
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/ipv4/ah4.c  |  8 ++++++--
 net/ipv4/esp4.c | 30 ++++++++++++++++++++----------
 net/ipv6/ah6.c  |  8 ++++++--
 net/ipv6/esp6.c | 31 +++++++++++++++++++++----------
 4 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 22377c8ff14b..e8f862358518 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -220,7 +220,9 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
@@ -393,7 +395,9 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
 	skb_push(skb, ihl);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index b1e24446e297..42cb09cc8533 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -360,9 +360,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 			esph = esp_output_set_extra(skb, esph, extra);
 
 			sg_init_table(sg, nfrags);
-			skb_to_sgvec(skb, sg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, sg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
+			if (unlikely(err < 0)) {
+				spin_unlock_bh(&x->lock);
+				goto error;
+			}
 
 			allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);
 
@@ -381,11 +385,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 			pfrag->offset = pfrag->offset + allocsize;
 
 			sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
-			skb_to_sgvec(skb, dsg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, dsg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
 
 			spin_unlock_bh(&x->lock);
+			if (unlikely(err < 0))
+				goto error;
 
 			goto skip_cow2;
 		}
@@ -422,9 +428,11 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph = esp_output_set_extra(skb, esph, extra);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg,
-		     (unsigned char *)esph - skb->data,
-		     assoclen + ivlen + clen + alen);
+	err = skb_to_sgvec(skb, sg,
+		           (unsigned char *)esph - skb->data,
+		           assoclen + ivlen + clen + alen);
+	if (unlikely(err < 0))
+		goto error;
 
 skip_cow2:
 	if ((x->props.flags & XFRM_STATE_ESN))
@@ -658,7 +666,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 	esp_input_set_header(skb, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	err = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out;
 
 	skb->ip_summed = CHECKSUM_NONE;
 
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index dda6035e3b84..755f38271dd5 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -423,7 +423,9 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
@@ -606,7 +608,9 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
 	ip6h->hop_limit   = 0;
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index ff54faa75631..017e2c2d36e1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -339,9 +339,13 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 			esph = esp_output_set_esn(skb, esph, seqhi);
 
 			sg_init_table(sg, nfrags);
-			skb_to_sgvec(skb, sg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, sg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
+			if (unlikely(err < 0)) {
+				spin_unlock_bh(&x->lock);
+				goto error;
+			}
 
 			allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);
 
@@ -360,12 +364,15 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 			pfrag->offset = pfrag->offset + allocsize;
 
 			sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
-			skb_to_sgvec(skb, dsg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, dsg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
 
 			spin_unlock_bh(&x->lock);
 
+			if (unlikely(err < 0))
+				goto error;
+
 			goto skip_cow2;
 		}
 	}
@@ -403,9 +410,11 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph = esp_output_set_esn(skb, esph, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg,
-		     (unsigned char *)esph - skb->data,
-		     assoclen + ivlen + clen + alen);
+	err = skb_to_sgvec(skb, sg,
+		           (unsigned char *)esph - skb->data,
+		           assoclen + ivlen + clen + alen);
+	if (unlikely(err < 0))
+		goto error;
 
 skip_cow2:
 	if ((x->props.flags & XFRM_STATE_ESN))
@@ -600,7 +609,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 	esp_input_set_header(skb, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0))
+		goto out;
 
 	skb->ip_summed = CHECKSUM_NONE;
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 3/5] rxrpc: check return value of skb_to_sgvec always
  2017-04-25 14:08 [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
  2017-04-25 14:08 ` [PATCH 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
@ 2017-04-25 14:08 ` Jason A. Donenfeld
  2017-04-25 14:08 ` [PATCH 4/5] macsec: " Jason A. Donenfeld
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 14:08 UTC (permalink / raw)
  To: netdev, linux-kernel, davem, David.Laight, kernel-hardening
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/rxrpc/rxkad.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..dcf46c9c3ece 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -229,7 +229,9 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
 	len &= ~(call->conn->size_align - 1);
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, 0, len);
+	err = skb_to_sgvec(skb, sg, 0, len);
+	if (unlikely(err < 0))
+		goto out;
 	skcipher_request_set_crypt(req, sg, sg, len, iv.x);
 	crypto_skcipher_encrypt(req);
 
@@ -342,7 +344,8 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
 		goto nomem;
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, offset, 8);
+	if (unlikely(skb_to_sgvec(skb, sg, offset, 8) < 0))
+		goto nomem;
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));
@@ -429,7 +432,8 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
 	}
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, offset, len);
+	if (unlikely(skb_to_sgvec(skb, sg, offset, len) < 0))
+		goto nomem;
 
 	/* decrypt from the session key */
 	token = call->conn->params.key->payload.data[0];
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 4/5] macsec: check return value of skb_to_sgvec always
  2017-04-25 14:08 [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
  2017-04-25 14:08 ` [PATCH 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
  2017-04-25 14:08 ` [PATCH 3/5] rxrpc: " Jason A. Donenfeld
@ 2017-04-25 14:08 ` Jason A. Donenfeld
  2017-04-25 14:08 ` [PATCH 5/5] virtio_net: " Jason A. Donenfeld
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 14:08 UTC (permalink / raw)
  To: netdev, linux-kernel, davem, David.Laight, kernel-hardening
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/macsec.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..d846f42b99ec 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -733,7 +733,12 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 	macsec_fill_iv(iv, secy->sci, pn);
 
 	sg_init_table(sg, MAX_SKB_FRAGS + 1);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0)) {
+		macsec_txsa_put(tx_sa);
+		kfree_skb(skb);
+		return ERR_PTR(ret);
+	}
 
 	if (tx_sc->encrypt) {
 		int len = skb->len - macsec_hdr_len(sci_present) -
@@ -937,7 +942,11 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
 	macsec_fill_iv(iv, sci, ntohl(hdr->packet_number));
 
 	sg_init_table(sg, MAX_SKB_FRAGS + 1);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0)) {
+		kfree_skb(skb);
+		return ERR_PTR(ret);
+	}
 
 	if (hdr->tci_an & MACSEC_TCI_E) {
 		/* confidentiality: ethernet + macsec header
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-04-25 14:08 [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                   ` (2 preceding siblings ...)
  2017-04-25 14:08 ` [PATCH 4/5] macsec: " Jason A. Donenfeld
@ 2017-04-25 14:08 ` Jason A. Donenfeld
  2017-04-25 14:16 ` [PATCH v2 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 14:08 UTC (permalink / raw)
  To: netdev, linux-kernel, davem, David.Laight, kernel-hardening
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/virtio_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f36584616e7d..1709fd0b4bf7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1081,7 +1081,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 	struct virtnet_info *vi = sq->vq->vdev->priv;
-	unsigned num_sg;
+	int num_sg;
 	unsigned hdr_len = vi->hdr_len;
 	bool can_push;
 
@@ -1114,6 +1114,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 		sg_set_buf(sq->sg, hdr, hdr_len);
 		num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
 	}
+	if (unlikely(num_sg < 0))
+		return num_sg;
 	return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
 }
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 14:08 [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                   ` (3 preceding siblings ...)
  2017-04-25 14:08 ` [PATCH 5/5] virtio_net: " Jason A. Donenfeld
@ 2017-04-25 14:16 ` Jason A. Donenfeld
  2017-04-25 14:21   ` [PATCH v3 " Jason A. Donenfeld
  2017-04-25 14:47 ` [PATCH " David Miller
  2017-04-25 15:42 ` [PATCH " Sergei Shtylyov
  6 siblings, 1 reply; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 14:16 UTC (permalink / raw)
  To: netdev, linux-kernel, davem, David.Laight, kernel-hardening
  Cc: Jason A. Donenfeld

This is a defense-in-depth measure in response to bugs like
4d6fa57b4dab0d77f4d8e9d9c73d1e63f6fe8fee.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/core/skbuff.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..7ed2cdf54c0a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3489,7 +3489,9 @@ void __init skb_init(void)
  *	@len: Length of buffer space to be mapped
  *
  *	Fill the specified scatter-gather list with mappings/pointers into a
- *	region of the buffer space attached to a socket buffer.
+ *	region of the buffer space attached to a socket buffer. Returns either
+ *	the number of scatterlist items used, or -EMSGSIZE if the contents
+ *	could not fit.
  */
 static int
 __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
@@ -3512,6 +3514,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
 		int end;
 
+		if (elt && sg_is_last(&sg[elt - 1]))
+			return -EMSGSIZE;
+
 		WARN_ON(start > offset + len);
 
 		end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
@@ -3535,6 +3540,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 
 		WARN_ON(start > offset + len);
 
+		if (elt && sg_is_last(&sg[elt - 1]))
+			return -EMSGSIZE;
+
 		end = start + frag_iter->len;
 		if ((copy = end - offset) > 0) {
 			if (copy > len)
@@ -3581,6 +3589,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
 {
 	int nsg = __skb_to_sgvec(skb, sg, offset, len);
 
+	if (nsg < 0)
+		return nsg;
+
 	sg_mark_end(&sg[nsg - 1]);
 
 	return nsg;
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v3 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 14:16 ` [PATCH v2 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
@ 2017-04-25 14:21   ` Jason A. Donenfeld
  0 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 14:21 UTC (permalink / raw)
  To: netdev, linux-kernel, davem, David.Laight, kernel-hardening
  Cc: Jason A. Donenfeld

This is a defense-in-depth measure in response to bugs like
4d6fa57b4dab0d77f4d8e9d9c73d1e63f6fe8fee.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Sorry for the completely stupid amount of churn - v1,v2,v3 in the span of
two minutes. It's just that after noticing first that nsg needs to be checked,
I also noticed something a bit worse: that there was a bug (exploitable?) where
if skb_to_sgvec was called with empty values, there would be an out-of-bounds
write into sg[0 - 1]. So, this third (and hopefully final!) patch fixes that
bug while we're at it.

 net/core/skbuff.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..d103134deddb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3489,7 +3489,9 @@ void __init skb_init(void)
  *	@len: Length of buffer space to be mapped
  *
  *	Fill the specified scatter-gather list with mappings/pointers into a
- *	region of the buffer space attached to a socket buffer.
+ *	region of the buffer space attached to a socket buffer. Returns either
+ *	the number of scatterlist items used, or -EMSGSIZE if the contents
+ *	could not fit.
  */
 static int
 __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
@@ -3512,6 +3514,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
 		int end;
 
+		if (elt && sg_is_last(&sg[elt - 1]))
+			return -EMSGSIZE;
+
 		WARN_ON(start > offset + len);
 
 		end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
@@ -3535,6 +3540,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 
 		WARN_ON(start > offset + len);
 
+		if (elt && sg_is_last(&sg[elt - 1]))
+			return -EMSGSIZE;
+
 		end = start + frag_iter->len;
 		if ((copy = end - offset) > 0) {
 			if (copy > len)
@@ -3581,6 +3589,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
 {
 	int nsg = __skb_to_sgvec(skb, sg, offset, len);
 
+	if (nsg <= 0)
+		return nsg;
+
 	sg_mark_end(&sg[nsg - 1]);
 
 	return nsg;
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 14:08 [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                   ` (4 preceding siblings ...)
  2017-04-25 14:16 ` [PATCH v2 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
@ 2017-04-25 14:47 ` David Miller
  2017-04-25 15:04   ` [PATCH v4 " Jason A. Donenfeld
  2017-04-25 15:42 ` [PATCH " Sergei Shtylyov
  6 siblings, 1 reply; 30+ messages in thread
From: David Miller @ 2017-04-25 14:47 UTC (permalink / raw)
  To: Jason; +Cc: netdev, linux-kernel, David.Laight, kernel-hardening

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Tue, 25 Apr 2017 16:08:05 +0200

> This is a defense-in-depth measure in response to bugs like
> 4d6fa57b4dab0d77f4d8e9d9c73d1e63f6fe8fee.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Please refer to commits in the form:

$(SHA1_ID) ("Commit header line.")

That is, 12 bytes of SHA1_ID followed by the commit header line text
in both double quotes and parenthesis, like this:

4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec")

Otherwise when changes get backported or applied to different trees,
they have different SHA1_ID values.  The commit header text removes
any and all ambiguity.

Thank you.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v4 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 14:47 ` [PATCH " David Miller
@ 2017-04-25 15:04   ` Jason A. Donenfeld
  2017-04-25 15:17     ` David Miller
  0 siblings, 1 reply; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 15:04 UTC (permalink / raw)
  To: netdev, linux-kernel, davem, David.Laight, kernel-hardening
  Cc: Jason A. Donenfeld

This is a defense-in-depth measure in response to bugs like
4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec")

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
v4 fixes the commit message and moves the check into the inner-most if.

 net/core/skbuff.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..e75640006d78 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3489,7 +3489,9 @@ void __init skb_init(void)
  *	@len: Length of buffer space to be mapped
  *
  *	Fill the specified scatter-gather list with mappings/pointers into a
- *	region of the buffer space attached to a socket buffer.
+ *	region of the buffer space attached to a socket buffer. Returns either
+ *	the number of scatterlist items used, or -EMSGSIZE if the contents
+ *	could not fit.
  */
 static int
 __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
@@ -3517,6 +3519,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 		end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
 		if ((copy = end - offset) > 0) {
 			skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+			if (elt && sg_is_last(&sg[elt - 1]))
+				return -EMSGSIZE;
 
 			if (copy > len)
 				copy = len;
@@ -3537,6 +3541,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 
 		end = start + frag_iter->len;
 		if ((copy = end - offset) > 0) {
+			if (elt && sg_is_last(&sg[elt - 1]))
+				return -EMSGSIZE;
+
 			if (copy > len)
 				copy = len;
 			elt += __skb_to_sgvec(frag_iter, sg+elt, offset - start,
@@ -3581,6 +3588,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
 {
 	int nsg = __skb_to_sgvec(skb, sg, offset, len);
 
+	if (nsg <= 0)
+		return nsg;
+
 	sg_mark_end(&sg[nsg - 1]);
 
 	return nsg;
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 15:04   ` [PATCH v4 " Jason A. Donenfeld
@ 2017-04-25 15:17     ` David Miller
  2017-04-25 15:52       ` [PATCH v5 " Jason A. Donenfeld
  0 siblings, 1 reply; 30+ messages in thread
From: David Miller @ 2017-04-25 15:17 UTC (permalink / raw)
  To: Jason; +Cc: netdev, linux-kernel, David.Laight, kernel-hardening


John this isn't how it works.

When you submit new versions of a patch that are part of a patch
series, you must resubmit the entire series not just the patches which
are changing.

Thanks.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 14:08 [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                   ` (5 preceding siblings ...)
  2017-04-25 14:47 ` [PATCH " David Miller
@ 2017-04-25 15:42 ` Sergei Shtylyov
  6 siblings, 0 replies; 30+ messages in thread
From: Sergei Shtylyov @ 2017-04-25 15:42 UTC (permalink / raw)
  To: Jason A. Donenfeld, netdev, linux-kernel, davem, David.Laight,
	kernel-hardening

Hello!

On 04/25/2017 05:08 PM, Jason A. Donenfeld wrote:

> This is a defense-in-depth measure in response to bugs like
> 4d6fa57b4dab0d77f4d8e9d9c73d1e63f6fe8fee.

    You need to also specify the summary line enclosed in (""). And it's 
enough to specify 12 digits of SHA1 ID...

> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v5 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 15:17     ` David Miller
@ 2017-04-25 15:52       ` Jason A. Donenfeld
  2017-04-25 15:52         ` [PATCH v5 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
                           ` (4 more replies)
  0 siblings, 5 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

This is a defense-in-depth measure in response to bugs like
4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec")

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
This is a resend of v4 with all the other child commits along with it.

 net/core/skbuff.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..e75640006d78 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3489,7 +3489,9 @@ void __init skb_init(void)
  *	@len: Length of buffer space to be mapped
  *
  *	Fill the specified scatter-gather list with mappings/pointers into a
- *	region of the buffer space attached to a socket buffer.
+ *	region of the buffer space attached to a socket buffer. Returns either
+ *	the number of scatterlist items used, or -EMSGSIZE if the contents
+ *	could not fit.
  */
 static int
 __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
@@ -3517,6 +3519,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 		end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
 		if ((copy = end - offset) > 0) {
 			skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+			if (elt && sg_is_last(&sg[elt - 1]))
+				return -EMSGSIZE;
 
 			if (copy > len)
 				copy = len;
@@ -3537,6 +3541,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 
 		end = start + frag_iter->len;
 		if ((copy = end - offset) > 0) {
+			if (elt && sg_is_last(&sg[elt - 1]))
+				return -EMSGSIZE;
+
 			if (copy > len)
 				copy = len;
 			elt += __skb_to_sgvec(frag_iter, sg+elt, offset - start,
@@ -3581,6 +3588,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
 {
 	int nsg = __skb_to_sgvec(skb, sg, offset, len);
 
+	if (nsg <= 0)
+		return nsg;
+
 	sg_mark_end(&sg[nsg - 1]);
 
 	return nsg;
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v5 2/5] ipsec: check return value of skb_to_sgvec always
  2017-04-25 15:52       ` [PATCH v5 " Jason A. Donenfeld
@ 2017-04-25 15:52         ` Jason A. Donenfeld
  2017-04-25 15:52         ` [PATCH v5 3/5] rxrpc: " Jason A. Donenfeld
                           ` (3 subsequent siblings)
  4 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/ipv4/ah4.c  |  8 ++++++--
 net/ipv4/esp4.c | 30 ++++++++++++++++++++----------
 net/ipv6/ah6.c  |  8 ++++++--
 net/ipv6/esp6.c | 31 +++++++++++++++++++++----------
 4 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 22377c8ff14b..e8f862358518 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -220,7 +220,9 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
@@ -393,7 +395,9 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
 	skb_push(skb, ihl);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index b1e24446e297..42cb09cc8533 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -360,9 +360,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 			esph = esp_output_set_extra(skb, esph, extra);
 
 			sg_init_table(sg, nfrags);
-			skb_to_sgvec(skb, sg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, sg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
+			if (unlikely(err < 0)) {
+				spin_unlock_bh(&x->lock);
+				goto error;
+			}
 
 			allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);
 
@@ -381,11 +385,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 			pfrag->offset = pfrag->offset + allocsize;
 
 			sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
-			skb_to_sgvec(skb, dsg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, dsg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
 
 			spin_unlock_bh(&x->lock);
+			if (unlikely(err < 0))
+				goto error;
 
 			goto skip_cow2;
 		}
@@ -422,9 +428,11 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph = esp_output_set_extra(skb, esph, extra);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg,
-		     (unsigned char *)esph - skb->data,
-		     assoclen + ivlen + clen + alen);
+	err = skb_to_sgvec(skb, sg,
+		           (unsigned char *)esph - skb->data,
+		           assoclen + ivlen + clen + alen);
+	if (unlikely(err < 0))
+		goto error;
 
 skip_cow2:
 	if ((x->props.flags & XFRM_STATE_ESN))
@@ -658,7 +666,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 	esp_input_set_header(skb, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	err = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out;
 
 	skb->ip_summed = CHECKSUM_NONE;
 
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index dda6035e3b84..755f38271dd5 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -423,7 +423,9 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
@@ -606,7 +608,9 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
 	ip6h->hop_limit   = 0;
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index ff54faa75631..017e2c2d36e1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -339,9 +339,13 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 			esph = esp_output_set_esn(skb, esph, seqhi);
 
 			sg_init_table(sg, nfrags);
-			skb_to_sgvec(skb, sg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, sg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
+			if (unlikely(err < 0)) {
+				spin_unlock_bh(&x->lock);
+				goto error;
+			}
 
 			allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);
 
@@ -360,12 +364,15 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 			pfrag->offset = pfrag->offset + allocsize;
 
 			sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
-			skb_to_sgvec(skb, dsg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, dsg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
 
 			spin_unlock_bh(&x->lock);
 
+			if (unlikely(err < 0))
+				goto error;
+
 			goto skip_cow2;
 		}
 	}
@@ -403,9 +410,11 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph = esp_output_set_esn(skb, esph, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg,
-		     (unsigned char *)esph - skb->data,
-		     assoclen + ivlen + clen + alen);
+	err = skb_to_sgvec(skb, sg,
+		           (unsigned char *)esph - skb->data,
+		           assoclen + ivlen + clen + alen);
+	if (unlikely(err < 0))
+		goto error;
 
 skip_cow2:
 	if ((x->props.flags & XFRM_STATE_ESN))
@@ -600,7 +609,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 	esp_input_set_header(skb, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0))
+		goto out;
 
 	skb->ip_summed = CHECKSUM_NONE;
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v5 3/5] rxrpc: check return value of skb_to_sgvec always
  2017-04-25 15:52       ` [PATCH v5 " Jason A. Donenfeld
  2017-04-25 15:52         ` [PATCH v5 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
@ 2017-04-25 15:52         ` Jason A. Donenfeld
  2017-04-25 15:52         ` [PATCH v5 4/5] macsec: " Jason A. Donenfeld
                           ` (2 subsequent siblings)
  4 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/rxrpc/rxkad.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..dcf46c9c3ece 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -229,7 +229,9 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
 	len &= ~(call->conn->size_align - 1);
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, 0, len);
+	err = skb_to_sgvec(skb, sg, 0, len);
+	if (unlikely(err < 0))
+		goto out;
 	skcipher_request_set_crypt(req, sg, sg, len, iv.x);
 	crypto_skcipher_encrypt(req);
 
@@ -342,7 +344,8 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
 		goto nomem;
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, offset, 8);
+	if (unlikely(skb_to_sgvec(skb, sg, offset, 8) < 0))
+		goto nomem;
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));
@@ -429,7 +432,8 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
 	}
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, offset, len);
+	if (unlikely(skb_to_sgvec(skb, sg, offset, len) < 0))
+		goto nomem;
 
 	/* decrypt from the session key */
 	token = call->conn->params.key->payload.data[0];
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v5 4/5] macsec: check return value of skb_to_sgvec always
  2017-04-25 15:52       ` [PATCH v5 " Jason A. Donenfeld
  2017-04-25 15:52         ` [PATCH v5 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
  2017-04-25 15:52         ` [PATCH v5 3/5] rxrpc: " Jason A. Donenfeld
@ 2017-04-25 15:52         ` Jason A. Donenfeld
  2017-04-25 15:52         ` [PATCH v5 5/5] virtio_net: " Jason A. Donenfeld
  2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
  4 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/macsec.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..d846f42b99ec 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -733,7 +733,12 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 	macsec_fill_iv(iv, secy->sci, pn);
 
 	sg_init_table(sg, MAX_SKB_FRAGS + 1);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0)) {
+		macsec_txsa_put(tx_sa);
+		kfree_skb(skb);
+		return ERR_PTR(ret);
+	}
 
 	if (tx_sc->encrypt) {
 		int len = skb->len - macsec_hdr_len(sci_present) -
@@ -937,7 +942,11 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
 	macsec_fill_iv(iv, sci, ntohl(hdr->packet_number));
 
 	sg_init_table(sg, MAX_SKB_FRAGS + 1);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0)) {
+		kfree_skb(skb);
+		return ERR_PTR(ret);
+	}
 
 	if (hdr->tci_an & MACSEC_TCI_E) {
 		/* confidentiality: ethernet + macsec header
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v5 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-04-25 15:52       ` [PATCH v5 " Jason A. Donenfeld
                           ` (2 preceding siblings ...)
  2017-04-25 15:52         ` [PATCH v5 4/5] macsec: " Jason A. Donenfeld
@ 2017-04-25 15:52         ` Jason A. Donenfeld
  2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
  4 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/virtio_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f36584616e7d..1709fd0b4bf7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1081,7 +1081,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 	struct virtnet_info *vi = sq->vq->vdev->priv;
-	unsigned num_sg;
+	int num_sg;
 	unsigned hdr_len = vi->hdr_len;
 	bool can_push;
 
@@ -1114,6 +1114,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 		sg_set_buf(sq->sg, hdr, hdr_len);
 		num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
 	}
+	if (unlikely(num_sg < 0))
+		return num_sg;
 	return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
 }
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 15:52       ` [PATCH v5 " Jason A. Donenfeld
                           ` (3 preceding siblings ...)
  2017-04-25 15:52         ` [PATCH v5 5/5] virtio_net: " Jason A. Donenfeld
@ 2017-04-25 18:47         ` Jason A. Donenfeld
  2017-04-25 18:47           ` [PATCH v6 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
                             ` (5 more replies)
  4 siblings, 6 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 18:47 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

This is a defense-in-depth measure in response to bugs like
4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec"). While
we're at it, we also limit the amount of recursion this function is
allowed to do. Not actually providing a bounded base case is a future
diaster that we can easily avoid here.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v5->v6:
  * Use unlikely() for the rare overflow conditions.
  * Also bound recursion, since this is a potential disaster we can avert.

 net/core/skbuff.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..24fb53f8534e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3489,16 +3489,22 @@ void __init skb_init(void)
  *	@len: Length of buffer space to be mapped
  *
  *	Fill the specified scatter-gather list with mappings/pointers into a
- *	region of the buffer space attached to a socket buffer.
+ *	region of the buffer space attached to a socket buffer. Returns either
+ *	the number of scatterlist items used, or -EMSGSIZE if the contents
+ *	could not fit.
  */
 static int
-__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
+__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
+	       unsigned int recursion_level)
 {
 	int start = skb_headlen(skb);
 	int i, copy = start - offset;
 	struct sk_buff *frag_iter;
 	int elt = 0;
 
+	if (unlikely(recursion_level >= 32))
+		return -EMSGSIZE;
+
 	if (copy > 0) {
 		if (copy > len)
 			copy = len;
@@ -3517,6 +3523,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 		end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
 		if ((copy = end - offset) > 0) {
 			skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+			if (unlikely(elt && sg_is_last(&sg[elt - 1])))
+				return -EMSGSIZE;
 
 			if (copy > len)
 				copy = len;
@@ -3531,16 +3539,22 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 	}
 
 	skb_walk_frags(skb, frag_iter) {
-		int end;
+		int end, ret;
 
 		WARN_ON(start > offset + len);
 
 		end = start + frag_iter->len;
 		if ((copy = end - offset) > 0) {
+			if (unlikely(elt && sg_is_last(&sg[elt - 1])))
+				return -EMSGSIZE;
+
 			if (copy > len)
 				copy = len;
-			elt += __skb_to_sgvec(frag_iter, sg+elt, offset - start,
-					      copy);
+			ret = __skb_to_sgvec(frag_iter, sg+elt, offset - start,
+					      copy, recursion_level + 1);
+			if (unlikely(ret < 0))
+				return ret;
+			elt += ret;
 			if ((len -= copy) == 0)
 				return elt;
 			offset += copy;
@@ -3573,13 +3587,16 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 int skb_to_sgvec_nomark(struct sk_buff *skb, struct scatterlist *sg,
 			int offset, int len)
 {
-	return __skb_to_sgvec(skb, sg, offset, len);
+	return __skb_to_sgvec(skb, sg, offset, len, 0);
 }
 EXPORT_SYMBOL_GPL(skb_to_sgvec_nomark);
 
 int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 {
-	int nsg = __skb_to_sgvec(skb, sg, offset, len);
+	int nsg = __skb_to_sgvec(skb, sg, offset, len, 0);
+
+	if (nsg <= 0)
+		return nsg;
 
 	sg_mark_end(&sg[nsg - 1]);
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v6 2/5] ipsec: check return value of skb_to_sgvec always
  2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
@ 2017-04-25 18:47           ` Jason A. Donenfeld
  2017-04-25 18:47           ` [PATCH v6 3/5] rxrpc: " Jason A. Donenfeld
                             ` (4 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 18:47 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/ipv4/ah4.c  |  8 ++++++--
 net/ipv4/esp4.c | 30 ++++++++++++++++++++----------
 net/ipv6/ah6.c  |  8 ++++++--
 net/ipv6/esp6.c | 31 +++++++++++++++++++++----------
 4 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 22377c8ff14b..e8f862358518 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -220,7 +220,9 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
@@ -393,7 +395,9 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
 	skb_push(skb, ihl);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index b1e24446e297..42cb09cc8533 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -360,9 +360,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 			esph = esp_output_set_extra(skb, esph, extra);
 
 			sg_init_table(sg, nfrags);
-			skb_to_sgvec(skb, sg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, sg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
+			if (unlikely(err < 0)) {
+				spin_unlock_bh(&x->lock);
+				goto error;
+			}
 
 			allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);
 
@@ -381,11 +385,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 			pfrag->offset = pfrag->offset + allocsize;
 
 			sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
-			skb_to_sgvec(skb, dsg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, dsg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
 
 			spin_unlock_bh(&x->lock);
+			if (unlikely(err < 0))
+				goto error;
 
 			goto skip_cow2;
 		}
@@ -422,9 +428,11 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph = esp_output_set_extra(skb, esph, extra);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg,
-		     (unsigned char *)esph - skb->data,
-		     assoclen + ivlen + clen + alen);
+	err = skb_to_sgvec(skb, sg,
+		           (unsigned char *)esph - skb->data,
+		           assoclen + ivlen + clen + alen);
+	if (unlikely(err < 0))
+		goto error;
 
 skip_cow2:
 	if ((x->props.flags & XFRM_STATE_ESN))
@@ -658,7 +666,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 	esp_input_set_header(skb, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	err = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out;
 
 	skb->ip_summed = CHECKSUM_NONE;
 
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index dda6035e3b84..755f38271dd5 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -423,7 +423,9 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
@@ -606,7 +608,9 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
 	ip6h->hop_limit   = 0;
 
 	sg_init_table(sg, nfrags + sglists);
-	skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+	if (unlikely(err < 0))
+		goto out_free;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
 		/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index ff54faa75631..017e2c2d36e1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -339,9 +339,13 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 			esph = esp_output_set_esn(skb, esph, seqhi);
 
 			sg_init_table(sg, nfrags);
-			skb_to_sgvec(skb, sg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, sg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
+			if (unlikely(err < 0)) {
+				spin_unlock_bh(&x->lock);
+				goto error;
+			}
 
 			allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);
 
@@ -360,12 +364,15 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 			pfrag->offset = pfrag->offset + allocsize;
 
 			sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
-			skb_to_sgvec(skb, dsg,
-				     (unsigned char *)esph - skb->data,
-				     assoclen + ivlen + clen + alen);
+			err = skb_to_sgvec(skb, dsg,
+				           (unsigned char *)esph - skb->data,
+				           assoclen + ivlen + clen + alen);
 
 			spin_unlock_bh(&x->lock);
 
+			if (unlikely(err < 0))
+				goto error;
+
 			goto skip_cow2;
 		}
 	}
@@ -403,9 +410,11 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph = esp_output_set_esn(skb, esph, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg,
-		     (unsigned char *)esph - skb->data,
-		     assoclen + ivlen + clen + alen);
+	err = skb_to_sgvec(skb, sg,
+		           (unsigned char *)esph - skb->data,
+		           assoclen + ivlen + clen + alen);
+	if (unlikely(err < 0))
+		goto error;
 
 skip_cow2:
 	if ((x->props.flags & XFRM_STATE_ESN))
@@ -600,7 +609,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 	esp_input_set_header(skb, seqhi);
 
 	sg_init_table(sg, nfrags);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0))
+		goto out;
 
 	skb->ip_summed = CHECKSUM_NONE;
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always
  2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
  2017-04-25 18:47           ` [PATCH v6 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
@ 2017-04-25 18:47           ` Jason A. Donenfeld
  2017-04-28 11:41             ` Sabrina Dubroca
  2017-04-25 18:47           ` [PATCH v6 4/5] macsec: " Jason A. Donenfeld
                             ` (3 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 18:47 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/rxrpc/rxkad.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..dcf46c9c3ece 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -229,7 +229,9 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
 	len &= ~(call->conn->size_align - 1);
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, 0, len);
+	err = skb_to_sgvec(skb, sg, 0, len);
+	if (unlikely(err < 0))
+		goto out;
 	skcipher_request_set_crypt(req, sg, sg, len, iv.x);
 	crypto_skcipher_encrypt(req);
 
@@ -342,7 +344,8 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
 		goto nomem;
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, offset, 8);
+	if (unlikely(skb_to_sgvec(skb, sg, offset, 8) < 0))
+		goto nomem;
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));
@@ -429,7 +432,8 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
 	}
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, offset, len);
+	if (unlikely(skb_to_sgvec(skb, sg, offset, len) < 0))
+		goto nomem;
 
 	/* decrypt from the session key */
 	token = call->conn->params.key->payload.data[0];
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v6 4/5] macsec: check return value of skb_to_sgvec always
  2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
  2017-04-25 18:47           ` [PATCH v6 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
  2017-04-25 18:47           ` [PATCH v6 3/5] rxrpc: " Jason A. Donenfeld
@ 2017-04-25 18:47           ` Jason A. Donenfeld
  2017-04-25 18:47           ` [PATCH v6 5/5] virtio_net: " Jason A. Donenfeld
                             ` (2 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 18:47 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/macsec.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..d846f42b99ec 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -733,7 +733,12 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 	macsec_fill_iv(iv, secy->sci, pn);
 
 	sg_init_table(sg, MAX_SKB_FRAGS + 1);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0)) {
+		macsec_txsa_put(tx_sa);
+		kfree_skb(skb);
+		return ERR_PTR(ret);
+	}
 
 	if (tx_sc->encrypt) {
 		int len = skb->len - macsec_hdr_len(sci_present) -
@@ -937,7 +942,11 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
 	macsec_fill_iv(iv, sci, ntohl(hdr->packet_number));
 
 	sg_init_table(sg, MAX_SKB_FRAGS + 1);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0)) {
+		kfree_skb(skb);
+		return ERR_PTR(ret);
+	}
 
 	if (hdr->tci_an & MACSEC_TCI_E) {
 		/* confidentiality: ethernet + macsec header
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v6 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                             ` (2 preceding siblings ...)
  2017-04-25 18:47           ` [PATCH v6 4/5] macsec: " Jason A. Donenfeld
@ 2017-04-25 18:47           ` Jason A. Donenfeld
  2017-04-27  9:21           ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
  2017-04-28 16:18           ` Sabrina Dubroca
  5 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-25 18:47 UTC (permalink / raw)
  To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
  Cc: Jason A. Donenfeld

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/virtio_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f36584616e7d..1709fd0b4bf7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1081,7 +1081,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 	struct virtnet_info *vi = sq->vq->vdev->priv;
-	unsigned num_sg;
+	int num_sg;
 	unsigned hdr_len = vi->hdr_len;
 	bool can_push;
 
@@ -1114,6 +1114,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 		sg_set_buf(sq->sg, hdr, hdr_len);
 		num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
 	}
+	if (unlikely(num_sg < 0))
+		return num_sg;
 	return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
 }
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                             ` (3 preceding siblings ...)
  2017-04-25 18:47           ` [PATCH v6 5/5] virtio_net: " Jason A. Donenfeld
@ 2017-04-27  9:21           ` Jason A. Donenfeld
  2017-04-27 11:30             ` Sabrina Dubroca
  2017-04-27 15:54             ` David Miller
  2017-04-28 16:18           ` Sabrina Dubroca
  5 siblings, 2 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-27  9:21 UTC (permalink / raw)
  To: Netdev, LKML, David Laight, kernel-hardening, David Miller
  Cc: Jason A. Donenfeld

Hey Dave,

David Laight and I have been discussing offlist. It occurred to both
of us that this could just be turned into a loop because perhaps this
is actually just tail-recursive. Upon further inspection, however, the
way the current algorithm works, it's possible that each of the
fraglist skbs has its own fraglist, which would make this into tree
recursion, which is why in the first place I wanted to place that
limit on it. If that's the case, then the patch I proposed above is
the best way forward. However, perhaps there's the chance that
fraglist skbs having separate fraglists are actually forbidden? Is
this the case? Are there other parts of the API that enforce this
contract? Is it something we could safely rely on here? If you say
yes, I'll send a v7 that makes this into a non-recursive loop.

Regards,
Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-27  9:21           ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
@ 2017-04-27 11:30             ` Sabrina Dubroca
  2017-04-27 12:04               ` Jason A. Donenfeld
  2017-04-27 15:54             ` David Miller
  1 sibling, 1 reply; 30+ messages in thread
From: Sabrina Dubroca @ 2017-04-27 11:30 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Netdev, LKML, David Laight, kernel-hardening, David Miller

2017-04-27, 11:21:51 +0200, Jason A. Donenfeld wrote:
> However, perhaps there's the chance that fraglist skbs having
> separate fraglists are actually forbidden? Is this the case?

Hmm, I think this can actually happen:

    /*  net/ipv4/ip_fragment.c  */
    static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
    			 struct net_device *dev)
    {
    
    ...
    
    	/* 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_nomem;
    		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(qp->q.net, clone->truesize);
    	}
    
    ...
    }
    

You can test that with a vxlan tunnel on top of a vxlan tunnel ("real"
MTU is 1500, first tunnel MTU set to 10000, second tunnel MTU set to
40000 -- or anything, as long as they both get fragmented).

-- 
Sabrina

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-27 11:30             ` Sabrina Dubroca
@ 2017-04-27 12:04               ` Jason A. Donenfeld
  2017-04-27 14:54                 ` David Laight
  0 siblings, 1 reply; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-27 12:04 UTC (permalink / raw)
  To: Sabrina Dubroca
  Cc: Netdev, LKML, David Laight, kernel-hardening, David Miller

On Thu, Apr 27, 2017 at 1:30 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> Hmm, I think this can actually happen:

Alright, perhaps better to err on the side of caution, then.

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread

* RE: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-27 12:04               ` Jason A. Donenfeld
@ 2017-04-27 14:54                 ` David Laight
  0 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2017-04-27 14:54 UTC (permalink / raw)
  To: 'Jason A. Donenfeld', Sabrina Dubroca
  Cc: Netdev, LKML, kernel-hardening, David Miller

From: Jason A. Donenfeld
> On Thu, Apr 27, 2017 at 1:30 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> > Hmm, I think this can actually happen:
> 
> Alright, perhaps better to err on the side of caution, then.

You only need to recurse if both pointers are set.

	David

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-27  9:21           ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
  2017-04-27 11:30             ` Sabrina Dubroca
@ 2017-04-27 15:54             ` David Miller
  1 sibling, 0 replies; 30+ messages in thread
From: David Miller @ 2017-04-27 15:54 UTC (permalink / raw)
  To: Jason; +Cc: netdev, linux-kernel, David.Laight, kernel-hardening

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Thu, 27 Apr 2017 11:21:51 +0200

> Hey Dave,
> 
> David Laight and I have been discussing offlist. It occurred to both
> of us that this could just be turned into a loop because perhaps this
> is actually just tail-recursive. Upon further inspection, however, the
> way the current algorithm works, it's possible that each of the
> fraglist skbs has its own fraglist, which would make this into tree
> recursion, which is why in the first place I wanted to place that
> limit on it. If that's the case, then the patch I proposed above is
> the best way forward. However, perhaps there's the chance that
> fraglist skbs having separate fraglists are actually forbidden? Is
> this the case? Are there other parts of the API that enforce this
> contract? Is it something we could safely rely on here? If you say
> yes, I'll send a v7 that makes this into a non-recursive loop.

As Sabrina showed, it can happen.  There are no such restrictions on
the geometry of an SKB.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always
  2017-04-25 18:47           ` [PATCH v6 3/5] rxrpc: " Jason A. Donenfeld
@ 2017-04-28 11:41             ` Sabrina Dubroca
  2017-04-28 13:29               ` Jason A. Donenfeld
  0 siblings, 1 reply; 30+ messages in thread
From: Sabrina Dubroca @ 2017-04-28 11:41 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: netdev, linux-kernel, David.Laight, kernel-hardening, davem

2017-04-25, 20:47:32 +0200, Jason A. Donenfeld wrote:
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  net/rxrpc/rxkad.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
> index 4374e7b9c7bf..dcf46c9c3ece 100644
> --- a/net/rxrpc/rxkad.c
> +++ b/net/rxrpc/rxkad.c
[...]
> @@ -429,7 +432,8 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
>  	}
>  

Adding a few more lines of context:

	sg = _sg;
	if (unlikely(nsg > 4)) {
		sg = kmalloc(sizeof(*sg) * nsg, GFP_NOIO);
		if (!sg)
			goto nomem;
	}

>  	sg_init_table(sg, nsg);
> -	skb_to_sgvec(skb, sg, offset, len);
> +	if (unlikely(skb_to_sgvec(skb, sg, offset, len) < 0))
> +		goto nomem;

You're leaking sg when nsg > 4, you'll need to add this:

	if (sg != _sg)
		kfree(sg);



BTW, when you resubmit, please Cc: the maintainers of the files you're
changing for each patch, so that they can review this stuff. And send
patch 1 to all of them, otherwise they might be surprised that we even
need <0 checking after calls to skb_to_sgvec.

You might also want to add a cover letter.

-- 
Sabrina

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always
  2017-04-28 11:41             ` Sabrina Dubroca
@ 2017-04-28 13:29               ` Jason A. Donenfeld
  0 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-28 13:29 UTC (permalink / raw)
  To: Sabrina Dubroca
  Cc: Netdev, LKML, David Laight, kernel-hardening, David Miller

Hi Sabrina,

Thanks for the review.

On Fri, Apr 28, 2017 at 1:41 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> >       sg_init_table(sg, nsg);
> > -     skb_to_sgvec(skb, sg, offset, len);
> > +     if (unlikely(skb_to_sgvec(skb, sg, offset, len) < 0))
> > +             goto nomem;
>
> You're leaking sg when nsg > 4, you'll need to add this:
>
>         if (sg != _sg)
>                 kfree(sg);

Nice catch. I'll fix this up in the next revision.


>
>
>
> BTW, when you resubmit, please Cc: the maintainers of the files you're
> changing for each patch, so that they can review this stuff. And send
> patch 1 to all of them, otherwise they might be surprised that we even
> need <0 checking after calls to skb_to_sgvec.
>
> You might also want to add a cover letter.

Both good ideas. Will do.

Jason

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                             ` (4 preceding siblings ...)
  2017-04-27  9:21           ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
@ 2017-04-28 16:18           ` Sabrina Dubroca
  2017-04-28 22:47             ` Jason A. Donenfeld
  5 siblings, 1 reply; 30+ messages in thread
From: Sabrina Dubroca @ 2017-04-28 16:18 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: netdev, linux-kernel, David.Laight, kernel-hardening, davem

2017-04-25, 20:47:30 +0200, Jason A. Donenfeld wrote:
> This is a defense-in-depth measure in response to bugs like
> 4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec"). While
> we're at it, we also limit the amount of recursion this function is
> allowed to do. Not actually providing a bounded base case is a future
> diaster that we can easily avoid here.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Changes v5->v6:
>   * Use unlikely() for the rare overflow conditions.
>   * Also bound recursion, since this is a potential disaster we can avert.
> 
>  net/core/skbuff.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f86bf69cfb8d..24fb53f8534e 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3489,16 +3489,22 @@ void __init skb_init(void)
>   *	@len: Length of buffer space to be mapped
>   *
>   *	Fill the specified scatter-gather list with mappings/pointers into a
> - *	region of the buffer space attached to a socket buffer.
> + *	region of the buffer space attached to a socket buffer. Returns either
> + *	the number of scatterlist items used, or -EMSGSIZE if the contents
> + *	could not fit.
>   */

One small thing here: since you're touching this comment, could you
move it next to skb_to_sgvec, since that's the function it's supposed
to document?

Thanks!

>  static int
> -__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
> +__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
> +	       unsigned int recursion_level)

-- 
Sabrina

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-04-28 16:18           ` Sabrina Dubroca
@ 2017-04-28 22:47             ` Jason A. Donenfeld
  0 siblings, 0 replies; 30+ messages in thread
From: Jason A. Donenfeld @ 2017-04-28 22:47 UTC (permalink / raw)
  To: Sabrina Dubroca
  Cc: Netdev, LKML, David Laight, kernel-hardening, David Miller

Hi Sabrina,

On Fri, Apr 28, 2017 at 6:18 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> One small thing here: since you're touching this comment, could you
> move it next to skb_to_sgvec, since that's the function it's supposed
> to document?

Done. I'll wait until next week to resubmit, to give some more time
for comments, but my current living copy of this series is here:
https://git.zx2c4.com/linux-dev/log/?h=jd/safe-skb-vec

One thing I'm considering, after discussing with David Laight, is the
potential of just using an explicit stack array for pushing and
popping skbs, rather than using the call stack. While this increases
complexity, which I'm opposed to, David makes the point that on some
architectures, the stack frame is rather large, and 32 function calls
of recursion might not be a good idea. Any opinons on this? Overkill
and simplicity is preferred? Or in fact best practice? (Either way,
I'll do a trial implementation of it to get an idea of how the end
result feels.)

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2017-04-28 22:47 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-25 14:08 [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
2017-04-25 14:08 ` [PATCH 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
2017-04-25 14:08 ` [PATCH 3/5] rxrpc: " Jason A. Donenfeld
2017-04-25 14:08 ` [PATCH 4/5] macsec: " Jason A. Donenfeld
2017-04-25 14:08 ` [PATCH 5/5] virtio_net: " Jason A. Donenfeld
2017-04-25 14:16 ` [PATCH v2 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
2017-04-25 14:21   ` [PATCH v3 " Jason A. Donenfeld
2017-04-25 14:47 ` [PATCH " David Miller
2017-04-25 15:04   ` [PATCH v4 " Jason A. Donenfeld
2017-04-25 15:17     ` David Miller
2017-04-25 15:52       ` [PATCH v5 " Jason A. Donenfeld
2017-04-25 15:52         ` [PATCH v5 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
2017-04-25 15:52         ` [PATCH v5 3/5] rxrpc: " Jason A. Donenfeld
2017-04-25 15:52         ` [PATCH v5 4/5] macsec: " Jason A. Donenfeld
2017-04-25 15:52         ` [PATCH v5 5/5] virtio_net: " Jason A. Donenfeld
2017-04-25 18:47         ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
2017-04-25 18:47           ` [PATCH v6 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
2017-04-25 18:47           ` [PATCH v6 3/5] rxrpc: " Jason A. Donenfeld
2017-04-28 11:41             ` Sabrina Dubroca
2017-04-28 13:29               ` Jason A. Donenfeld
2017-04-25 18:47           ` [PATCH v6 4/5] macsec: " Jason A. Donenfeld
2017-04-25 18:47           ` [PATCH v6 5/5] virtio_net: " Jason A. Donenfeld
2017-04-27  9:21           ` [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
2017-04-27 11:30             ` Sabrina Dubroca
2017-04-27 12:04               ` Jason A. Donenfeld
2017-04-27 14:54                 ` David Laight
2017-04-27 15:54             ` David Miller
2017-04-28 16:18           ` Sabrina Dubroca
2017-04-28 22:47             ` Jason A. Donenfeld
2017-04-25 15:42 ` [PATCH " Sergei Shtylyov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).