[PATCH net-next v9 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: Steffen Klassert <steffen.klass...@secunet.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@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(>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(s

[PATCH net-next v9 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: Steffen Klassert 
Cc: Herbert Xu 
Cc: "David S. Miller" 
---
 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(>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 +

[PATCH net-next v9 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: David Howells <dhowe...@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(, 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

[PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Jason Wang <jasow...@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

[PATCH net-next v9 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: David Howells 
---
 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(, 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

[PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
---
 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

[PATCH net-next v9 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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 <ja...@zx2c4.com>
Cc: Steffen Klassert <steffen.klass...@secunet.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: David Howells <dhowe...@redhat.com>
Cc: Sabrina Dubroca <s...@queasysnail.net>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Jason Wang <jasow...@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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (unlikely(elt && sg_is_last([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) 

[PATCH net-next v9 0/5] skb_to_sgvec hardening

[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

[PATCH net-next v9 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: Sabrina Dubroca <s...@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

[PATCH net-next v9 0/5] skb_to_sgvec hardening

[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

[PATCH net-next v9 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: Sabrina Dubroca 
---
 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

[PATCH net-next v9 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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 
Cc: Steffen Klassert 
Cc: Herbert Xu 
Cc: "David S. Miller" 
Cc: David Howells 
Cc: Sabrina Dubroca 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
---
 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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (unlikely(elt && sg_is_last([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([elt - 1])))
+   return -EMSGSIZE;
+
if (copy > len)
copy = len;

Re: [PATCH v8 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

On Mon, May 15, 2017 at 3:11 PM, David Howells  wrote:
> skb_to_sgvec() can return -EMSGSIZE in some circumstances.  You shouldn't
> return -ENOMEM here in such a case.

Noted. I'll fix this up for the next round.

Re: [PATCH v8 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

On Mon, May 15, 2017 at 3:11 PM, David Howells  wrote:
> skb_to_sgvec() can return -EMSGSIZE in some circumstances.  You shouldn't
> return -ENOMEM here in such a case.

Noted. I'll fix this up for the next round.

Re: [PATCH v8 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

On Mon, May 15, 2017 at 3:12 PM, David Howells  wrote:
> Is there a reason you moved skb_to_sgvec() in the file rather than just moving
> the comment to it (since you moved the comment anyway)?

1) Because it's easier to understand skb_to_sgvec_nomark as a variant
of skb_to_sgvec, so I'd rather skb_to_sgvec to be first when reading.
2) Because skb_to_sgvec relies on the return value of __skb_to_sgvec,
and so when assessing it, it's sometimes nice to be able to look at
why it will return different things. In that case, it's easier to have
both functions within the same view without scrolling.

It's the little things that make life easier sometimes.

Re: [PATCH v8 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

On Mon, May 15, 2017 at 3:12 PM, David Howells  wrote:
> Is there a reason you moved skb_to_sgvec() in the file rather than just moving
> the comment to it (since you moved the comment anyway)?

1) Because it's easier to understand skb_to_sgvec_nomark as a variant
of skb_to_sgvec, so I'd rather skb_to_sgvec to be first when reading.
2) Because skb_to_sgvec relies on the return value of __skb_to_sgvec,
and so when assessing it, it's sometimes nice to be able to look at
why it will return different things. In that case, it's easier to have
both functions within the same view without scrolling.

It's the little things that make life easier sometimes.

Re: Implementing Dynamic Rerouting in Kernel

[Jason A. Donenfeld]

On Thu, May 11, 2017 at 6:22 PM, Florian Fainelli  wrote:
> What you are looking for can be done using ipset-dns from Jason:
>
> https://git.zx2c4.com/ipset-dns/about/

Funny to see this project coming up. I actually ported this
functionality into dnsmasq directly a few weeks after writing
ipset-dns, since that's a much more robust place to put this sort of
feature. It's the --ipset option.

Re: Implementing Dynamic Rerouting in Kernel

[Jason A. Donenfeld]

On Thu, May 11, 2017 at 6:22 PM, Florian Fainelli  wrote:
> What you are looking for can be done using ipset-dns from Jason:
>
> https://git.zx2c4.com/ipset-dns/about/

Funny to see this project coming up. I actually ported this
functionality into dnsmasq directly a few weeks after writing
ipset-dns, since that's a much more robust place to put this sort of
feature. It's the --ipset option.

[PATCH v8 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Jason Wang <jasow...@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

[PATCH v8 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
---
 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

[PATCH v8 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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 <ja...@zx2c4.com>
Cc: Steffen Klassert <steffen.klass...@secunet.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: David Howells <dhowe...@redhat.com>
Cc: Sabrina Dubroca <s...@queasysnail.net>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Jason Wang <jasow...@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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (unlikely(elt && sg_is_last([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) 

[PATCH v8 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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 
Cc: Steffen Klassert 
Cc: Herbert Xu 
Cc: "David S. Miller" 
Cc: David Howells 
Cc: Sabrina Dubroca 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
---
 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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (unlikely(elt && sg_is_last([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([elt - 1])))
+   return -EMSGSIZE;
+
if (copy > len)
copy = len;

[PATCH v8 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: David Howells <dhowe...@redhat.com>
---
 net/rxrpc/rxkad.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 1bb9b2ccc267..ecab9334e3c1 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);
 
@@ -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(, 0, sizeof(iv));
@@ -434,7 +437,11 @@ 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)) {
+   if (sg != _sg)
+   kfree(sg);
+   goto nomem;
+   }
 
/* decrypt from the session key */
token = call->conn->params.key->payload.data[0];
-- 
2.13.0

[PATCH v8 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: David Howells 
---
 net/rxrpc/rxkad.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 1bb9b2ccc267..ecab9334e3c1 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);
 
@@ -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(, 0, sizeof(iv));
@@ -434,7 +437,11 @@ 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)) {
+   if (sg != _sg)
+   kfree(sg);
+   goto nomem;
+   }
 
/* decrypt from the session key */
token = call->conn->params.key->payload.data[0];
-- 
2.13.0

[PATCH v8 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: Sabrina Dubroca <s...@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

[PATCH v8 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: Sabrina Dubroca 
---
 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

[PATCH v8 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: Steffen Klassert <steffen.klass...@secunet.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@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(>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(s

[PATCH v8 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: Steffen Klassert 
Cc: Herbert Xu 
Cc: "David S. Miller" 
---
 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(>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 +

[PATCH v8 0/5] skb_to_sgvec hardening

[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 v7->v8:
   - Added __must_check annotation.
   - Rebased against latest upstream ipsec changes.

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| 13 +++---
 9 files changed, 112 insertions(+), 52 deletions(-)

-- 
2.13.0

[PATCH v8 0/5] skb_to_sgvec hardening

[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 v7->v8:
   - Added __must_check annotation.
   - Rebased against latest upstream ipsec changes.

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| 13 +++---
 9 files changed, 112 insertions(+), 52 deletions(-)

-- 
2.13.0

Re: [PATCH v7 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

On Tue, May 9, 2017 at 3:50 PM, Jason A. Donenfeld <ja...@zx2c4.com> wrote:
> num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;

(The next submission of this will take into account this + 1 here.
https://git.zx2c4.com/linux-dev/log/?h=jd/safe-skb-vec )

Re: [PATCH v7 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

On Tue, May 9, 2017 at 3:50 PM, Jason A. Donenfeld  wrote:
> num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;

(The next submission of this will take into account this + 1 here.
https://git.zx2c4.com/linux-dev/log/?h=jd/safe-skb-vec )

Re: [PATCH v7 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

(The next submission of this ipsec patch will have this rebased over
the latest upstream tree.
https://git.zx2c4.com/linux-dev/log/?h=jd/safe-skb-vec )

Re: [PATCH v7 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

(The next submission of this ipsec patch will have this rebased over
the latest upstream tree.
https://git.zx2c4.com/linux-dev/log/?h=jd/safe-skb-vec )

Re: [PATCH v7 0/5] skb_to_sgvec hardening

[Jason A. Donenfeld]

On Tue, May 9, 2017 at 4:03 PM, Johannes Berg  wrote:
> Perhaps you should add __must_check annotation to the function
> prototype(s)?

Great idea. I've started doing this in my own code. Wasn't sure how
popular it was outside of there, but I'm glad to hear a suggestion of
it now. I'll have this in v8.

Re: [PATCH v7 0/5] skb_to_sgvec hardening

[Jason A. Donenfeld]

On Tue, May 9, 2017 at 4:03 PM, Johannes Berg  wrote:
> Perhaps you should add __must_check annotation to the function
> prototype(s)?

Great idea. I've started doing this in my own code. Wasn't sure how
popular it was outside of there, but I'm glad to hear a suggestion of
it now. I'll have this in v8.

[PATCH v7 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: David Howells <dhowe...@redhat.com>
---
 net/rxrpc/rxkad.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..486026689448 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(, 0, sizeof(iv));
@@ -429,7 +432,11 @@ 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)) {
+   if (sg != _sg)
+   kfree(sg);
+   goto nomem;
+   }
 
/* decrypt from the session key */
token = call->conn->params.key->payload.data[0];
-- 
2.12.2

[PATCH v7 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: David Howells 
---
 net/rxrpc/rxkad.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..486026689448 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(, 0, sizeof(iv));
@@ -429,7 +432,11 @@ 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)) {
+   if (sg != _sg)
+   kfree(sg);
+   goto nomem;
+   }
 
/* decrypt from the session key */
token = call->conn->params.key->payload.data[0];
-- 
2.12.2

[PATCH v7 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: Sabrina Dubroca <s...@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 49ce4e9f4a0f..e022e3fcd012 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.12.2

[PATCH v7 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: Sabrina Dubroca 
---
 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 49ce4e9f4a0f..e022e3fcd012 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.12.2

[PATCH v7 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: Steffen Klassert <steffen.klass...@secunet.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@davemloft.net>
---
 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(>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(>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 pay

[PATCH v7 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Jason Wang <jasow...@redhat.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

[PATCH v7 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: Steffen Klassert 
Cc: Herbert Xu 
Cc: "David S. Miller" 
---
 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(>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(>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  

[PATCH v7 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
---
 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

[PATCH v7 0/5] skb_to_sgvec hardening

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

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 |  4 ++-
 net/core/skbuff.c| 65 +++-
 net/ipv4/ah4.c   |  8 --
 net/ipv4/esp4.c  | 30 ++
 net/ipv6/ah6.c   |  8 --
 net/ipv6/esp6.c  | 31 +++
 net/rxrpc/rxkad.c| 13 +++---
 8 files changed, 119 insertions(+), 53 deletions(-)

-- 
2.12.2

[PATCH v7 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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 <ja...@zx2c4.com>
Cc: Steffen Klassert <steffen.klass...@secunet.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: David Howells <dhowe...@redhat.com>
Cc: Sabrina Dubroca <s...@queasysnail.net>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Jason Wang <jasow...@redhat.com>
---
 net/core/skbuff.c | 65 +++
 1 file changed, 42 insertions(+), 23 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..ab51b97d5600 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3481,24 +3481,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;
@@ -3517,6 +3511,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist 
*sg, int offset, int len)
end = start + skb_frag_size(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (unlikely(elt && sg_is_last([elt - 1])))
+   return -EMSGSIZE;
 
if (copy > len)
copy = len;
@@ -3531,16 +3527,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([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;
@@ -3551,6 +3553,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 t

[PATCH v7 0/5] skb_to_sgvec hardening

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

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 |  4 ++-
 net/core/skbuff.c| 65 +++-
 net/ipv4/ah4.c   |  8 --
 net/ipv4/esp4.c  | 30 ++
 net/ipv6/ah6.c   |  8 --
 net/ipv6/esp6.c  | 31 +++
 net/rxrpc/rxkad.c| 13 +++---
 8 files changed, 119 insertions(+), 53 deletions(-)

-- 
2.12.2

[PATCH v7 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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 
Cc: Steffen Klassert 
Cc: Herbert Xu 
Cc: "David S. Miller" 
Cc: David Howells 
Cc: Sabrina Dubroca 
Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
---
 net/core/skbuff.c | 65 +++
 1 file changed, 42 insertions(+), 23 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..ab51b97d5600 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3481,24 +3481,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;
@@ -3517,6 +3511,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist 
*sg, int offset, int len)
end = start + skb_frag_size(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (unlikely(elt && sg_is_last([elt - 1])))
+   return -EMSGSIZE;
 
if (copy > len)
copy = len;
@@ -3531,16 +3527,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([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;
@@ -3551,6 +3553,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

Re: crypto_memneq not backported to 3.10

[Jason A. Donenfeld]

Hey Willy,

On Sun, Apr 9, 2017 at 3:25 PM, Willy Tarreau <w...@1wt.eu> wrote:
>
> Hi Jason,
>
> On Sun, Apr 09, 2017 at 02:59:53PM +0200, Jason A. Donenfeld wrote:
> > Hey Willy,
> >
> > Linux 3.10 is inexplicably missing crypto_memneq, making all crypto
> > mac comparisons use non constant-time comparisons. Bad news bears.
> >
> > 3.12 got these backported with
> > d68e944a8fcb2c6212b38064771c9f5af7b0b92c,
> > afe5a791d374e50a06ada7f4eda4e921e1b77996, and possibly others. I'd
> > suggest following suit, since many people are relying on this kernel
> > to do safe crypto.
>
> Interesting. I remembered seeing some crypto_memneq stuff in the past,
> and in fact there was one patch talking about this but trimmed down to
> only affect other parts since crypto_memneq is indeed not part of 3.10.
>
> I'll check if the 3.12 patches above can be safely backported, and I'll
> have to re-apply the missing part of the one that was trimmed down
> (commit 620c411 ("crypto: more robust crypto_memneq")).

I'm vaguely wondering if you ever decided on backporting this. After I
reported the issue to Ubiquiti -- a random vendor doing ipsec with
3.10 -- they actually released a backport of these functions in a
security update for their stuff. So I imagine others might want this
sort of thing too.

Jason

Re: crypto_memneq not backported to 3.10

[Jason A. Donenfeld]

Hey Willy,

On Sun, Apr 9, 2017 at 3:25 PM, Willy Tarreau  wrote:
>
> Hi Jason,
>
> On Sun, Apr 09, 2017 at 02:59:53PM +0200, Jason A. Donenfeld wrote:
> > Hey Willy,
> >
> > Linux 3.10 is inexplicably missing crypto_memneq, making all crypto
> > mac comparisons use non constant-time comparisons. Bad news bears.
> >
> > 3.12 got these backported with
> > d68e944a8fcb2c6212b38064771c9f5af7b0b92c,
> > afe5a791d374e50a06ada7f4eda4e921e1b77996, and possibly others. I'd
> > suggest following suit, since many people are relying on this kernel
> > to do safe crypto.
>
> Interesting. I remembered seeing some crypto_memneq stuff in the past,
> and in fact there was one patch talking about this but trimmed down to
> only affect other parts since crypto_memneq is indeed not part of 3.10.
>
> I'll check if the 3.12 patches above can be safely backported, and I'll
> have to re-apply the missing part of the one that was trimmed down
> (commit 620c411 ("crypto: more robust crypto_memneq")).

I'm vaguely wondering if you ever decided on backporting this. After I
reported the issue to Ubiquiti -- a random vendor doing ipsec with
3.10 -- they actually released a backport of these functions in a
security update for their stuff. So I imagine others might want this
sort of thing too.

Jason

Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

Hi Sabrina,

On Fri, Apr 28, 2017 at 6:18 PM, Sabrina Dubroca  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.)

Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

Hi Sabrina,

On Fri, Apr 28, 2017 at 6:18 PM, Sabrina Dubroca  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.)

Re: [PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Hi Sabrina,

Thanks for the review.

On Fri, Apr 28, 2017 at 1:41 PM, Sabrina Dubroca  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

Re: [PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Hi Sabrina,

Thanks for the review.

On Fri, Apr 28, 2017 at 1:41 PM, Sabrina Dubroca  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

Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

On Thu, Apr 27, 2017 at 1:30 PM, Sabrina Dubroca  wrote:
> Hmm, I think this can actually happen:

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

Jason

Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

On Thu, Apr 27, 2017 at 1:30 PM, Sabrina Dubroca  wrote:
> Hmm, I think this can actually happen:

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

Jason

Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[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

Re: [PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[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

[PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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(, 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

[PATCH v6 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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(, 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

[PATCH v6 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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(>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(>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 + sglis

[PATCH v6 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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

[PATCH v6 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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

[PATCH v6 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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(>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(>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);
-   s

[PATCH v6 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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

[PATCH v6 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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

[PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[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 <ja...@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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (unlikely(elt && sg_is_last([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([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([nsg - 1]);
 
-- 
2.12.2

[PATCH v6 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[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 
---
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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (unlikely(elt && sg_is_last([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([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([nsg - 1]);
 
-- 
2.12.2

[PATCH v2] macsec: dynamically allocate space for sglist

[Jason A. Donenfeld]

We call skb_cow_data, which is good anyway to ensure we can actually
modify the skb as such (another error from prior). Now that we have the
number of fragments required, we can safely allocate exactly that amount
of memory.

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: Sabrina Dubroca <s...@queasysnail.net>
Cc: secur...@kernel.org
Cc: sta...@vger.kernel.org
---
Changes v1 -> v2:
  - sg_init_table now takes the correct argument.

 drivers/net/macsec.c | 29 ++---
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..49ce4e9f4a0f 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -617,7 +617,8 @@ static void macsec_encrypt_done(struct crypto_async_request 
*base, int err)
 
 static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm,
 unsigned char **iv,
-struct scatterlist **sg)
+struct scatterlist **sg,
+int num_frags)
 {
size_t size, iv_offset, sg_offset;
struct aead_request *req;
@@ -629,7 +630,7 @@ static struct aead_request *macsec_alloc_req(struct 
crypto_aead *tfm,
 
size = ALIGN(size, __alignof__(struct scatterlist));
sg_offset = size;
-   size += sizeof(struct scatterlist) * (MAX_SKB_FRAGS + 1);
+   size += sizeof(struct scatterlist) * num_frags;
 
tmp = kmalloc(size, GFP_ATOMIC);
if (!tmp)
@@ -649,6 +650,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 {
int ret;
struct scatterlist *sg;
+   struct sk_buff *trailer;
unsigned char *iv;
struct ethhdr *eth;
struct macsec_eth_header *hh;
@@ -723,7 +725,14 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
return ERR_PTR(-EINVAL);
}
 
-   req = macsec_alloc_req(tx_sa->key.tfm, , );
+   ret = skb_cow_data(skb, 0, );
+   if (unlikely(ret < 0)) {
+   macsec_txsa_put(tx_sa);
+   kfree_skb(skb);
+   return ERR_PTR(ret);
+   }
+
+   req = macsec_alloc_req(tx_sa->key.tfm, , , ret);
if (!req) {
macsec_txsa_put(tx_sa);
kfree_skb(skb);
@@ -732,7 +741,7 @@ 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);
+   sg_init_table(sg, ret);
skb_to_sgvec(skb, sg, 0, skb->len);
 
if (tx_sc->encrypt) {
@@ -917,6 +926,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
 {
int ret;
struct scatterlist *sg;
+   struct sk_buff *trailer;
unsigned char *iv;
struct aead_request *req;
struct macsec_eth_header *hdr;
@@ -927,7 +937,12 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
if (!skb)
return ERR_PTR(-ENOMEM);
 
-   req = macsec_alloc_req(rx_sa->key.tfm, , );
+   ret = skb_cow_data(skb, 0, );
+   if (unlikely(ret < 0)) {
+   kfree_skb(skb);
+   return ERR_PTR(ret);
+   }
+   req = macsec_alloc_req(rx_sa->key.tfm, , , ret);
if (!req) {
kfree_skb(skb);
return ERR_PTR(-ENOMEM);
@@ -936,7 +951,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
hdr = (struct macsec_eth_header *)skb->data;
macsec_fill_iv(iv, sci, ntohl(hdr->packet_number));
 
-   sg_init_table(sg, MAX_SKB_FRAGS + 1);
+   sg_init_table(sg, ret);
skb_to_sgvec(skb, sg, 0, skb->len);
 
if (hdr->tci_an & MACSEC_TCI_E) {
@@ -2716,7 +2731,7 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 }
 
 #define MACSEC_FEATURES \
-   (NETIF_F_SG | NETIF_F_HIGHDMA)
+   (NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST)
 static struct lock_class_key macsec_netdev_addr_lock_key;
 
 static int macsec_dev_init(struct net_device *dev)
-- 
2.12.2

[PATCH v2] macsec: dynamically allocate space for sglist

[Jason A. Donenfeld]

We call skb_cow_data, which is good anyway to ensure we can actually
modify the skb as such (another error from prior). Now that we have the
number of fragments required, we can safely allocate exactly that amount
of memory.

Signed-off-by: Jason A. Donenfeld 
Cc: Sabrina Dubroca 
Cc: secur...@kernel.org
Cc: sta...@vger.kernel.org
---
Changes v1 -> v2:
  - sg_init_table now takes the correct argument.

 drivers/net/macsec.c | 29 ++---
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..49ce4e9f4a0f 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -617,7 +617,8 @@ static void macsec_encrypt_done(struct crypto_async_request 
*base, int err)
 
 static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm,
 unsigned char **iv,
-struct scatterlist **sg)
+struct scatterlist **sg,
+int num_frags)
 {
size_t size, iv_offset, sg_offset;
struct aead_request *req;
@@ -629,7 +630,7 @@ static struct aead_request *macsec_alloc_req(struct 
crypto_aead *tfm,
 
size = ALIGN(size, __alignof__(struct scatterlist));
sg_offset = size;
-   size += sizeof(struct scatterlist) * (MAX_SKB_FRAGS + 1);
+   size += sizeof(struct scatterlist) * num_frags;
 
tmp = kmalloc(size, GFP_ATOMIC);
if (!tmp)
@@ -649,6 +650,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 {
int ret;
struct scatterlist *sg;
+   struct sk_buff *trailer;
unsigned char *iv;
struct ethhdr *eth;
struct macsec_eth_header *hh;
@@ -723,7 +725,14 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
return ERR_PTR(-EINVAL);
}
 
-   req = macsec_alloc_req(tx_sa->key.tfm, , );
+   ret = skb_cow_data(skb, 0, );
+   if (unlikely(ret < 0)) {
+   macsec_txsa_put(tx_sa);
+   kfree_skb(skb);
+   return ERR_PTR(ret);
+   }
+
+   req = macsec_alloc_req(tx_sa->key.tfm, , , ret);
if (!req) {
macsec_txsa_put(tx_sa);
kfree_skb(skb);
@@ -732,7 +741,7 @@ 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);
+   sg_init_table(sg, ret);
skb_to_sgvec(skb, sg, 0, skb->len);
 
if (tx_sc->encrypt) {
@@ -917,6 +926,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
 {
int ret;
struct scatterlist *sg;
+   struct sk_buff *trailer;
unsigned char *iv;
struct aead_request *req;
struct macsec_eth_header *hdr;
@@ -927,7 +937,12 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
if (!skb)
return ERR_PTR(-ENOMEM);
 
-   req = macsec_alloc_req(rx_sa->key.tfm, , );
+   ret = skb_cow_data(skb, 0, );
+   if (unlikely(ret < 0)) {
+   kfree_skb(skb);
+   return ERR_PTR(ret);
+   }
+   req = macsec_alloc_req(rx_sa->key.tfm, , , ret);
if (!req) {
kfree_skb(skb);
return ERR_PTR(-ENOMEM);
@@ -936,7 +951,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
hdr = (struct macsec_eth_header *)skb->data;
macsec_fill_iv(iv, sci, ntohl(hdr->packet_number));
 
-   sg_init_table(sg, MAX_SKB_FRAGS + 1);
+   sg_init_table(sg, ret);
skb_to_sgvec(skb, sg, 0, skb->len);
 
if (hdr->tci_an & MACSEC_TCI_E) {
@@ -2716,7 +2731,7 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 }
 
 #define MACSEC_FEATURES \
-   (NETIF_F_SG | NETIF_F_HIGHDMA)
+   (NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST)
 static struct lock_class_key macsec_netdev_addr_lock_key;
 
 static int macsec_dev_init(struct net_device *dev)
-- 
2.12.2

[PATCH v5 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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(, 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

[PATCH v5 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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(, 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

[PATCH v5 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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

[PATCH v5 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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

[PATCH v5 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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

[PATCH v5 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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

[PATCH v5 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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(>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(>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 + sglis

[PATCH v5 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[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 <ja...@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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (elt && sg_is_last([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([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([nsg - 1]);
 
return nsg;
-- 
2.12.2

[PATCH v5 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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(>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(>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);
-   s

[PATCH v5 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[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 
---
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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (elt && sg_is_last([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([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([nsg - 1]);
 
return nsg;
-- 
2.12.2

[PATCH] macsec: dynamically allocate space for sglist

[Jason A. Donenfeld]

We call skb_cow_data, which is good anyway to ensure we can actually
modify the skb as such (another error from prior). Now that we have the
number of fragments required, we can safely allocate exactly that amount
of memory.

Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
Cc: Sabrina Dubroca <s...@queasysnail.net>
Cc: secur...@kernel.org
Cc: sta...@vger.kernel.org
---
 drivers/net/macsec.c | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..56dafdee4c9c 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -617,7 +617,8 @@ static void macsec_encrypt_done(struct crypto_async_request 
*base, int err)
 
 static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm,
 unsigned char **iv,
-struct scatterlist **sg)
+struct scatterlist **sg,
+int num_frags)
 {
size_t size, iv_offset, sg_offset;
struct aead_request *req;
@@ -629,7 +630,7 @@ static struct aead_request *macsec_alloc_req(struct 
crypto_aead *tfm,
 
size = ALIGN(size, __alignof__(struct scatterlist));
sg_offset = size;
-   size += sizeof(struct scatterlist) * (MAX_SKB_FRAGS + 1);
+   size += sizeof(struct scatterlist) * num_frags;
 
tmp = kmalloc(size, GFP_ATOMIC);
if (!tmp)
@@ -649,6 +650,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 {
int ret;
struct scatterlist *sg;
+   struct sk_buff *trailer;
unsigned char *iv;
struct ethhdr *eth;
struct macsec_eth_header *hh;
@@ -723,7 +725,14 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
return ERR_PTR(-EINVAL);
}
 
-   req = macsec_alloc_req(tx_sa->key.tfm, , );
+   ret = skb_cow_data(skb, 0, );
+   if (unlikely(ret < 0)) {
+   macsec_txsa_put(tx_sa);
+   kfree_skb(skb);
+   return ERR_PTR(ret);
+   }
+
+   req = macsec_alloc_req(tx_sa->key.tfm, , , ret);
if (!req) {
macsec_txsa_put(tx_sa);
kfree_skb(skb);
@@ -917,6 +926,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
 {
int ret;
struct scatterlist *sg;
+   struct sk_buff *trailer;
unsigned char *iv;
struct aead_request *req;
struct macsec_eth_header *hdr;
@@ -927,7 +937,12 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
if (!skb)
return ERR_PTR(-ENOMEM);
 
-   req = macsec_alloc_req(rx_sa->key.tfm, , );
+   ret = skb_cow_data(skb, 0, );
+   if (unlikely(ret < 0)) {
+   kfree_skb(skb);
+   return ERR_PTR(ret);
+   }
+   req = macsec_alloc_req(rx_sa->key.tfm, , , ret);
if (!req) {
kfree_skb(skb);
return ERR_PTR(-ENOMEM);
@@ -2716,7 +2731,7 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 }
 
 #define MACSEC_FEATURES \
-   (NETIF_F_SG | NETIF_F_HIGHDMA)
+   (NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST)
 static struct lock_class_key macsec_netdev_addr_lock_key;
 
 static int macsec_dev_init(struct net_device *dev)
-- 
2.12.2

[PATCH] macsec: dynamically allocate space for sglist

[Jason A. Donenfeld]

We call skb_cow_data, which is good anyway to ensure we can actually
modify the skb as such (another error from prior). Now that we have the
number of fragments required, we can safely allocate exactly that amount
of memory.

Signed-off-by: Jason A. Donenfeld 
Cc: Sabrina Dubroca 
Cc: secur...@kernel.org
Cc: sta...@vger.kernel.org
---
 drivers/net/macsec.c | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..56dafdee4c9c 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -617,7 +617,8 @@ static void macsec_encrypt_done(struct crypto_async_request 
*base, int err)
 
 static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm,
 unsigned char **iv,
-struct scatterlist **sg)
+struct scatterlist **sg,
+int num_frags)
 {
size_t size, iv_offset, sg_offset;
struct aead_request *req;
@@ -629,7 +630,7 @@ static struct aead_request *macsec_alloc_req(struct 
crypto_aead *tfm,
 
size = ALIGN(size, __alignof__(struct scatterlist));
sg_offset = size;
-   size += sizeof(struct scatterlist) * (MAX_SKB_FRAGS + 1);
+   size += sizeof(struct scatterlist) * num_frags;
 
tmp = kmalloc(size, GFP_ATOMIC);
if (!tmp)
@@ -649,6 +650,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 {
int ret;
struct scatterlist *sg;
+   struct sk_buff *trailer;
unsigned char *iv;
struct ethhdr *eth;
struct macsec_eth_header *hh;
@@ -723,7 +725,14 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
return ERR_PTR(-EINVAL);
}
 
-   req = macsec_alloc_req(tx_sa->key.tfm, , );
+   ret = skb_cow_data(skb, 0, );
+   if (unlikely(ret < 0)) {
+   macsec_txsa_put(tx_sa);
+   kfree_skb(skb);
+   return ERR_PTR(ret);
+   }
+
+   req = macsec_alloc_req(tx_sa->key.tfm, , , ret);
if (!req) {
macsec_txsa_put(tx_sa);
kfree_skb(skb);
@@ -917,6 +926,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
 {
int ret;
struct scatterlist *sg;
+   struct sk_buff *trailer;
unsigned char *iv;
struct aead_request *req;
struct macsec_eth_header *hdr;
@@ -927,7 +937,12 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
if (!skb)
return ERR_PTR(-ENOMEM);
 
-   req = macsec_alloc_req(rx_sa->key.tfm, , );
+   ret = skb_cow_data(skb, 0, );
+   if (unlikely(ret < 0)) {
+   kfree_skb(skb);
+   return ERR_PTR(ret);
+   }
+   req = macsec_alloc_req(rx_sa->key.tfm, , , ret);
if (!req) {
kfree_skb(skb);
return ERR_PTR(-ENOMEM);
@@ -2716,7 +2731,7 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 }
 
 #define MACSEC_FEATURES \
-   (NETIF_F_SG | NETIF_F_HIGHDMA)
+   (NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST)
 static struct lock_class_key macsec_netdev_addr_lock_key;
 
 static int macsec_dev_init(struct net_device *dev)
-- 
2.12.2

Re: [PATCH] macsec: avoid heap overflow in skb_to_sgvec

[Jason A. Donenfeld]

On Tue, Apr 25, 2017 at 5:12 PM, Sabrina Dubroca  wrote:
>> https://patchwork.ozlabs.org/patch/754861/
>
> Yes, that prevents the overflow, but now you're just dropping
> packets.

Right, it's a so-called "defense-in-depth" measure.

> I'll review that later, let's fix the overflow without
> breaking connectivity for now.

Agreed.

Re: [PATCH] macsec: avoid heap overflow in skb_to_sgvec

[Jason A. Donenfeld]

On Tue, Apr 25, 2017 at 5:12 PM, Sabrina Dubroca  wrote:
>> https://patchwork.ozlabs.org/patch/754861/
>
> Yes, that prevents the overflow, but now you're just dropping
> packets.

Right, it's a so-called "defense-in-depth" measure.

> I'll review that later, let's fix the overflow without
> breaking connectivity for now.

Agreed.

Re: [PATCH] macsec: avoid heap overflow in skb_to_sgvec

[Jason A. Donenfeld]

Hi Sabrina,

On Tue, Apr 25, 2017 at 4:53 PM, Sabrina Dubroca  wrote:
> Ugh, good catch :/
>
> AFAICT this patch doesn't really help, because NETIF_F_FRAGLIST
> doesn't get tested in paths that can lead to triggering this.

You're right. This fixes the xmit() path, but not the receive path,
which appears to take skbs directly from the upper device.

> I'll post a patch to allocate a properly-sized sg array.

I just posted this series, which should fix things in a robust way:

https://patchwork.ozlabs.org/patch/754861/

If you want to handle fraglists, it might be a decent idea to allocate
things of the proper size, I guess. It's a good opportunity to call
skb_cow_data, which you need to do anyway when modifying skbs, I
think.

Jason

Re: [PATCH] macsec: avoid heap overflow in skb_to_sgvec

[Jason A. Donenfeld]

Hi Sabrina,

On Tue, Apr 25, 2017 at 4:53 PM, Sabrina Dubroca  wrote:
> Ugh, good catch :/
>
> AFAICT this patch doesn't really help, because NETIF_F_FRAGLIST
> doesn't get tested in paths that can lead to triggering this.

You're right. This fixes the xmit() path, but not the receive path,
which appears to take skbs directly from the upper device.

> I'll post a patch to allocate a properly-sized sg array.

I just posted this series, which should fix things in a robust way:

https://patchwork.ozlabs.org/patch/754861/

If you want to handle fraglists, it might be a decent idea to allocate
things of the proper size, I guess. It's a good opportunity to call
skb_cow_data, which you need to do anyway when modifying skbs, I
think.

Jason

[PATCH v4 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[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 <ja...@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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (elt && sg_is_last([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([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([nsg - 1]);
 
return nsg;
-- 
2.12.2

[PATCH v4 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[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 
---
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(_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = _shinfo(skb)->frags[i];
+   if (elt && sg_is_last([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([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([nsg - 1]);
 
return nsg;
-- 
2.12.2

[PATCH v3 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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

Signed-off-by: Jason A. Donenfeld <ja...@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([elt - 1]))
+   return -EMSGSIZE;
+
WARN_ON(start > offset + len);
 
end = start + skb_frag_size(_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([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([nsg - 1]);
 
return nsg;
-- 
2.12.2

[PATCH v3 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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

Signed-off-by: Jason A. Donenfeld 
---
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([elt - 1]))
+   return -EMSGSIZE;
+
WARN_ON(start > offset + len);
 
end = start + skb_frag_size(_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([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([nsg - 1]);
 
return nsg;
-- 
2.12.2

[PATCH v2 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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

Signed-off-by: Jason A. Donenfeld <ja...@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([elt - 1]))
+   return -EMSGSIZE;
+
WARN_ON(start > offset + len);
 
end = start + skb_frag_size(_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([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([nsg - 1]);
 
return nsg;
-- 
2.12.2

[PATCH v2 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow

[Jason A. Donenfeld]

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

Signed-off-by: Jason A. Donenfeld 
---
 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([elt - 1]))
+   return -EMSGSIZE;
+
WARN_ON(start > offset + len);
 
end = start + skb_frag_size(_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([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([nsg - 1]);
 
return nsg;
-- 
2.12.2

[PATCH 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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

[PATCH 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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(>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(>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 + sglis

[PATCH 4/5] macsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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

[PATCH 2/5] ipsec: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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(>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(>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);
-   s

[PATCH 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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(, 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

[PATCH 3/5] rxrpc: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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(, 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

[PATCH 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld <ja...@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

[PATCH 5/5] virtio_net: check return value of skb_to_sgvec always

[Jason A. Donenfeld]

Signed-off-by: Jason A. Donenfeld 
---
 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