linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v9 0/5] skb_to_sgvec hardening
@ 2017-05-23 16:05 Jason A. Donenfeld
  2017-05-23 16:05 ` [PATCH net-next v9 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Jason A. Donenfeld @ 2017-05-23 16:05 UTC (permalink / raw)
  To: netdev, linux-kernel, davem; +Cc: Jason A. Donenfeld

The recent bug with macsec and historical one with virtio have
indicated that letting skb_to_sgvec trounce all over an sglist
without checking the length is probably a bad idea. And it's not
necessary either: an sglist already explicitly marks its last
item, and the initialization functions are diligent in doing so.
Thus there's a clear way of avoiding future overflows.

So, this patchset, from a high level, makes skb_to_sgvec return
a potential error code, and then adjusts all callers to check
for the error code. There are two situations in which skb_to_sgvec
might return such an error:

   1) When the passed in sglist is too small; and
   2) When the passed in skbuff is too deeply nested.

So, the first patch in this series handles the issues with
skb_to_sgvec directly, and the remaining ones then handle the call
sites.

Changes v8->v9:
   - Return correct errno in rxrpc, thanks to feedback from Dave Howells.

Jason A. Donenfeld (5):
  skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  ipsec: check return value of skb_to_sgvec always
  rxrpc: check return value of skb_to_sgvec always
  macsec: check return value of skb_to_sgvec always
  virtio_net: check return value of skb_to_sgvec always

 drivers/net/macsec.c     | 13 ++++++++--
 drivers/net/virtio_net.c |  9 +++++--
 include/linux/skbuff.h   |  8 +++---
 net/core/skbuff.c        | 65 +++++++++++++++++++++++++++++++-----------------
 net/ipv4/ah4.c           |  8 ++++--
 net/ipv4/esp4.c          | 20 +++++++++------
 net/ipv6/ah6.c           |  8 ++++--
 net/ipv6/esp6.c          | 20 +++++++++------
 net/rxrpc/rxkad.c        | 19 ++++++++++----
 9 files changed, 116 insertions(+), 54 deletions(-)

-- 
2.13.0

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

* [PATCH net-next v9 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
  2017-05-23 16:05 [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
@ 2017-05-23 16:05 ` Jason A. Donenfeld
  2017-05-23 16:05 ` [PATCH net-next v9 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Jason A. Donenfeld @ 2017-05-23 16:05 UTC (permalink / raw)
  To: netdev, linux-kernel, davem
  Cc: Jason A. Donenfeld, Steffen Klassert, Herbert Xu, David Howells,
	Sabrina Dubroca, Michael S. Tsirkin, Jason Wang

This is a defense-in-depth measure in response to bugs like
4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec"). There's
not only a potential overflow of sglist items, but also a stack overflow
potential, so we fix this by limiting the amount of recursion this function
is allowed to do. Not actually providing a bounded base case is a future
disaster that we can easily avoid here.

As a small matter of house keeping, we take this opportunity to move the
documentation comment over the actual function the documentation is for.

While this could be implemented by using an explicit stack of skbuffs,
when implementing this, the function complexity increased considerably,
and I don't think such complexity and bloat is actually worth it. So,
instead I built this and tested it on x86, x86_64, ARM, ARM64, and MIPS,
and measured the stack usage there. I also reverted the recent MIPS
changes that give it a separate IRQ stack, so that I could experience
some worst-case situations. I found that limiting it to 24 layers deep
yielded a good stack usage with room for safety, as well as being much
deeper than any driver actually ever creates.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Howells <dhowells@redhat.com>
Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 include/linux/skbuff.h |  8 +++----
 net/core/skbuff.c      | 65 ++++++++++++++++++++++++++++++++------------------
 2 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a098d95b3d84..f351df28942d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1001,10 +1001,10 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb,
 				     unsigned int headroom);
 struct sk_buff *skb_copy_expand(const struct sk_buff *skb, int newheadroom,
 				int newtailroom, gfp_t priority);
-int skb_to_sgvec_nomark(struct sk_buff *skb, struct scatterlist *sg,
-			int offset, int len);
-int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset,
-		 int len);
+int __must_check skb_to_sgvec_nomark(struct sk_buff *skb, struct scatterlist *sg,
+				     int offset, int len);
+int __must_check skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg,
+			      int offset, int len);
 int skb_cow_data(struct sk_buff *skb, int tailbits, struct sk_buff **trailer);
 int skb_pad(struct sk_buff *skb, int pad);
 #define dev_kfree_skb(a)	consume_skb(a)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 346d3e85dfbc..ec33811559e3 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3482,24 +3482,18 @@ void __init skb_init(void)
 						NULL);
 }
 
-/**
- *	skb_to_sgvec - Fill a scatter-gather list from a socket buffer
- *	@skb: Socket buffer containing the buffers to be mapped
- *	@sg: The scatter-gather list to map into
- *	@offset: The offset into the buffer's contents to start mapping
- *	@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.
- */
 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 >= 24))
+		return -EMSGSIZE;
+
 	if (copy > 0) {
 		if (copy > len)
 			copy = len;
@@ -3518,6 +3512,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;
@@ -3532,16 +3528,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;
@@ -3552,6 +3554,31 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 	return elt;
 }
 
+/**
+ *	skb_to_sgvec - Fill a scatter-gather list from a socket buffer
+ *	@skb: Socket buffer containing the buffers to be mapped
+ *	@sg: The scatter-gather list to map into
+ *	@offset: The offset into the buffer's contents to start mapping
+ *	@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. Returns either
+ *	the number of scatterlist items used, or -EMSGSIZE if the contents
+ *	could not fit.
+ */
+int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
+{
+	int nsg = __skb_to_sgvec(skb, sg, offset, len, 0);
+
+	if (nsg <= 0)
+		return nsg;
+
+	sg_mark_end(&sg[nsg - 1]);
+
+	return nsg;
+}
+EXPORT_SYMBOL_GPL(skb_to_sgvec);
+
 /* As compared with skb_to_sgvec, skb_to_sgvec_nomark only map skb to given
  * sglist without mark the sg which contain last skb data as the end.
  * So the caller can mannipulate sg list as will when padding new data after
@@ -3574,19 +3601,11 @@ __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);
 
-	sg_mark_end(&sg[nsg - 1]);
-
-	return nsg;
-}
-EXPORT_SYMBOL_GPL(skb_to_sgvec);
 
 /**
  *	skb_cow_data - Check that a socket buffer's data buffers are writable
-- 
2.13.0

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

* [PATCH net-next v9 2/5] ipsec: check return value of skb_to_sgvec always
  2017-05-23 16:05 [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
  2017-05-23 16:05 ` [PATCH net-next v9 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
@ 2017-05-23 16:05 ` Jason A. Donenfeld
  2017-05-23 16:05 ` [PATCH net-next v9 3/5] rxrpc: " Jason A. Donenfeld
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Jason A. Donenfeld @ 2017-05-23 16:05 UTC (permalink / raw)
  To: netdev, linux-kernel, davem
  Cc: Jason A. Donenfeld, Steffen Klassert, Herbert Xu

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
 net/ipv4/ah4.c  |  8 ++++++--
 net/ipv4/esp4.c | 20 +++++++++++++-------
 net/ipv6/ah6.c  |  8 ++++++--
 net/ipv6/esp6.c | 20 +++++++++++++-------
 4 files changed, 38 insertions(+), 18 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 65cc02bd82bc..392432860bb9 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -374,9 +374,11 @@ int esp_output_tail(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
 	esp->esph = esph;
 
 	sg_init_table(sg, esp->nfrags);
-	skb_to_sgvec(skb, sg,
-		     (unsigned char *)esph - skb->data,
-		     assoclen + ivlen + esp->clen + alen);
+	err = skb_to_sgvec(skb, sg,
+		           (unsigned char *)esph - skb->data,
+		           assoclen + ivlen + esp->clen + alen);
+	if (unlikely(err < 0))
+		goto error;
 
 	if (!esp->inplace) {
 		int allocsize;
@@ -400,9 +402,11 @@ int esp_output_tail(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
 		spin_unlock_bh(&x->lock);
 
 		sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
-		skb_to_sgvec(skb, dsg,
-			     (unsigned char *)esph - skb->data,
-			     assoclen + ivlen + esp->clen + alen);
+		err = skb_to_sgvec(skb, dsg,
+			           (unsigned char *)esph - skb->data,
+			           assoclen + ivlen + esp->clen + alen);
+		if (unlikely(err < 0))
+			goto error;
 	}
 
 	if ((x->props.flags & XFRM_STATE_ESN))
@@ -687,7 +691,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 1fe99ba8066c..2ede4e459c4e 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -346,9 +346,11 @@ int esp6_output_tail(struct xfrm_state *x, struct sk_buff *skb, struct esp_info
 	esph = esp_output_set_esn(skb, x, ip_esp_hdr(skb), seqhi);
 
 	sg_init_table(sg, esp->nfrags);
-	skb_to_sgvec(skb, sg,
-		     (unsigned char *)esph - skb->data,
-		     assoclen + ivlen + esp->clen + alen);
+	err = skb_to_sgvec(skb, sg,
+		           (unsigned char *)esph - skb->data,
+		           assoclen + ivlen + esp->clen + alen);
+	if (unlikely(err < 0))
+		goto error;
 
 	if (!esp->inplace) {
 		int allocsize;
@@ -372,9 +374,11 @@ int esp6_output_tail(struct xfrm_state *x, struct sk_buff *skb, struct esp_info
 		spin_unlock_bh(&x->lock);
 
 		sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
-		skb_to_sgvec(skb, dsg,
-			     (unsigned char *)esph - skb->data,
-			     assoclen + ivlen + esp->clen + alen);
+		err = skb_to_sgvec(skb, dsg,
+			           (unsigned char *)esph - skb->data,
+			           assoclen + ivlen + esp->clen + alen);
+		if (unlikely(err < 0))
+			goto error;
 	}
 
 	if ((x->props.flags & XFRM_STATE_ESN))
@@ -618,7 +622,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.13.0

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

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

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: David Howells <dhowells@redhat.com>
---
 net/rxrpc/rxkad.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 1bb9b2ccc267..29fe20ad04aa 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -227,7 +227,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);
 
@@ -324,7 +326,7 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
 	bool aborted;
 	u32 data_size, buf;
 	u16 check;
-	int nsg;
+	int nsg, ret;
 
 	_enter("");
 
@@ -342,7 +344,9 @@ 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);
+	ret = skb_to_sgvec(skb, sg, offset, 8);
+	if (unlikely(ret < 0))
+		return ret;
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));
@@ -409,7 +413,7 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
 	bool aborted;
 	u32 data_size, buf;
 	u16 check;
-	int nsg;
+	int nsg, ret;
 
 	_enter(",{%d}", skb->len);
 
@@ -434,7 +438,12 @@ 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);
+	ret = skb_to_sgvec(skb, sg, offset, len);
+	if (unlikely(ret < 0)) {
+		if (sg != _sg)
+			kfree(sg);
+		return ret;
+	}
 
 	/* decrypt from the session key */
 	token = call->conn->params.key->payload.data[0];
-- 
2.13.0

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

* [PATCH net-next v9 4/5] macsec: check return value of skb_to_sgvec always
  2017-05-23 16:05 [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
                   ` (2 preceding siblings ...)
  2017-05-23 16:05 ` [PATCH net-next v9 3/5] rxrpc: " Jason A. Donenfeld
@ 2017-05-23 16:05 ` Jason A. Donenfeld
  2017-05-23 16:05 ` [PATCH net-next v9 5/5] virtio_net: " Jason A. Donenfeld
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Jason A. Donenfeld @ 2017-05-23 16:05 UTC (permalink / raw)
  To: netdev, linux-kernel, davem; +Cc: Jason A. Donenfeld, Sabrina Dubroca

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Sabrina Dubroca <sd@queasysnail.net>
---
 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 cdc347be68f2..dfcb1e9d2ab2 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -742,7 +742,12 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 	macsec_fill_iv(iv, secy->sci, pn);
 
 	sg_init_table(sg, ret);
-	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) -
@@ -952,7 +957,11 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
 	macsec_fill_iv(iv, sci, ntohl(hdr->packet_number));
 
 	sg_init_table(sg, ret);
-	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.13.0

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

* [PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-05-23 16:05 [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
                   ` (3 preceding siblings ...)
  2017-05-23 16:05 ` [PATCH net-next v9 4/5] macsec: " Jason A. Donenfeld
@ 2017-05-23 16:05 ` Jason A. Donenfeld
  2017-05-24  9:41   ` Sergei Shtylyov
  2017-05-23 16:24 ` [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
  2017-05-24 15:38 ` [PATCH net-next v9 3/5] rxrpc: check return value of skb_to_sgvec always David Howells
  6 siblings, 1 reply; 13+ messages in thread
From: Jason A. Donenfeld @ 2017-05-23 16:05 UTC (permalink / raw)
  To: netdev, linux-kernel, davem
  Cc: Jason A. Donenfeld, Michael S. Tsirkin, Jason Wang

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9320d96a1632..13fbe4b349c2 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1150,7 +1150,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;
 
@@ -1177,11 +1177,16 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	if (can_push) {
 		__skb_push(skb, hdr_len);
 		num_sg = skb_to_sgvec(skb, sq->sg, 0, skb->len);
+                if (unlikely(num_sg < 0))
+			return num_sg;
 		/* Pull header back to avoid skew in tx bytes calculations. */
 		__skb_pull(skb, hdr_len);
 	} else {
 		sg_set_buf(sq->sg, hdr, hdr_len);
-		num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
+		num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len);
+		if (unlikely(num_sg < 0))
+			return num_sg;
+		num_sg++;
 	}
 	return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
 }
-- 
2.13.0

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

* Re: [PATCH net-next v9 0/5] skb_to_sgvec hardening
  2017-05-23 16:05 [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
                   ` (4 preceding siblings ...)
  2017-05-23 16:05 ` [PATCH net-next v9 5/5] virtio_net: " Jason A. Donenfeld
@ 2017-05-23 16:24 ` Jason A. Donenfeld
  2017-05-24 15:38 ` [PATCH net-next v9 3/5] rxrpc: check return value of skb_to_sgvec always David Howells
  6 siblings, 0 replies; 13+ messages in thread
From: Jason A. Donenfeld @ 2017-05-23 16:24 UTC (permalink / raw)
  To: David Miller; +Cc: LKML, Netdev

Hi List,

Could somebody do a holistic review of the series, or at least on
individual commits that seem fine, and sign off on it, so that this
can actually be merged? We're now at v9. I hope we can get this merged
now, but if not, I'd like for v10 to finally land these changes.

Regards,
Jason

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

* Re: [PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-05-23 16:05 ` [PATCH net-next v9 5/5] virtio_net: " Jason A. Donenfeld
@ 2017-05-24  9:41   ` Sergei Shtylyov
  2017-05-24 11:34     ` Jason A. Donenfeld
  0 siblings, 1 reply; 13+ messages in thread
From: Sergei Shtylyov @ 2017-05-24  9:41 UTC (permalink / raw)
  To: Jason A. Donenfeld, netdev, linux-kernel, davem
  Cc: Michael S. Tsirkin, Jason Wang

Hello!

On 5/23/2017 7:05 PM, Jason A. Donenfeld wrote:

> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9320d96a1632..13fbe4b349c2 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1150,7 +1150,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;
>
> @@ -1177,11 +1177,16 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>  	if (can_push) {
>  		__skb_push(skb, hdr_len);
>  		num_sg = skb_to_sgvec(skb, sq->sg, 0, skb->len);
> +                if (unlikely(num_sg < 0))

    Please indent with tabs, like above and below.

> +			return num_sg;
>  		/* Pull header back to avoid skew in tx bytes calculations. */
>  		__skb_pull(skb, hdr_len);
[...]

MBR, Sergei

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

* Re: [PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-05-24  9:41   ` Sergei Shtylyov
@ 2017-05-24 11:34     ` Jason A. Donenfeld
  2017-05-24 16:41       ` Sergei Shtylyov
  0 siblings, 1 reply; 13+ messages in thread
From: Jason A. Donenfeld @ 2017-05-24 11:34 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Netdev, LKML, David Miller, Michael S. Tsirkin, Jason Wang

I'm shocked this somehow made it into the commit. I wonder how that happened?

Anyway, fixed in my git repo, and will be part of the next series.
(Unless DaveM wants to fix it up trivially when/if he merges this v9,
which would be faster.)

Barring that, does this look good to you? Could I have your signed-off-by?

Regards,
Jason

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

* Re: [PATCH net-next v9 3/5] rxrpc: check return value of skb_to_sgvec always
  2017-05-23 16:05 [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
                   ` (5 preceding siblings ...)
  2017-05-23 16:24 ` [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
@ 2017-05-24 15:38 ` David Howells
  6 siblings, 0 replies; 13+ messages in thread
From: David Howells @ 2017-05-24 15:38 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: dhowells, netdev, linux-kernel, davem

Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: David Howells <dhowells@redhat.com>

Acked-by: David Howells <dhowells@redhat.com>

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

* Re: [PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-05-24 11:34     ` Jason A. Donenfeld
@ 2017-05-24 16:41       ` Sergei Shtylyov
  2017-05-24 20:39         ` Jason A. Donenfeld
  0 siblings, 1 reply; 13+ messages in thread
From: Sergei Shtylyov @ 2017-05-24 16:41 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Netdev, LKML, David Miller, Michael S. Tsirkin, Jason Wang

On 05/24/2017 02:34 PM, Jason A. Donenfeld wrote:

> I'm shocked this somehow made it into the commit. I wonder how that happened?

    Sorry for not noticing this when it first appeared.

> Anyway, fixed in my git repo, and will be part of the next series.
> (Unless DaveM wants to fix it up trivially when/if he merges this v9,
> which would be faster.)
>
> Barring that, does this look good to you? Could I have your signed-off-by?

    I've only looked on the last 2 patches. You can add my:

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

if you want. :-)

> Regards,
> Jason

MBR, Sergei

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

* Re: [PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-05-24 16:41       ` Sergei Shtylyov
@ 2017-05-24 20:39         ` Jason A. Donenfeld
  2017-05-24 20:46           ` Sergei Shtylyov
  0 siblings, 1 reply; 13+ messages in thread
From: Jason A. Donenfeld @ 2017-05-24 20:39 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Netdev, LKML, David Miller, Michael S. Tsirkin, Jason Wang

On Wed, May 24, 2017 at 6:41 PM, Sergei Shtylyov
>    I've only looked on the last 2 patches. You can add my:
>
> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> if you want. :-)

Will do. For the series, or just for 5/5?

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

* Re: [PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always
  2017-05-24 20:39         ` Jason A. Donenfeld
@ 2017-05-24 20:46           ` Sergei Shtylyov
  0 siblings, 0 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2017-05-24 20:46 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Netdev, LKML, David Miller, Michael S. Tsirkin, Jason Wang

On 05/24/2017 11:39 PM, Jason A. Donenfeld wrote:

>>    I've only looked on the last 2 patches. You can add my:
>>
>> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> if you want. :-)
>
> Will do. For the series, or just for 5/5?

    5/5 only. :-)

MBR, Sergei

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

end of thread, other threads:[~2017-05-24 20:46 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-23 16:05 [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
2017-05-23 16:05 ` [PATCH net-next v9 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow Jason A. Donenfeld
2017-05-23 16:05 ` [PATCH net-next v9 2/5] ipsec: check return value of skb_to_sgvec always Jason A. Donenfeld
2017-05-23 16:05 ` [PATCH net-next v9 3/5] rxrpc: " Jason A. Donenfeld
2017-05-23 16:05 ` [PATCH net-next v9 4/5] macsec: " Jason A. Donenfeld
2017-05-23 16:05 ` [PATCH net-next v9 5/5] virtio_net: " Jason A. Donenfeld
2017-05-24  9:41   ` Sergei Shtylyov
2017-05-24 11:34     ` Jason A. Donenfeld
2017-05-24 16:41       ` Sergei Shtylyov
2017-05-24 20:39         ` Jason A. Donenfeld
2017-05-24 20:46           ` Sergei Shtylyov
2017-05-23 16:24 ` [PATCH net-next v9 0/5] skb_to_sgvec hardening Jason A. Donenfeld
2017-05-24 15:38 ` [PATCH net-next v9 3/5] rxrpc: check return value of skb_to_sgvec always David Howells

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).