2017-04-25 14:08:32

by Jason A. Donenfeld

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

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

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

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

+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
WARN_ON(start > offset + len);

end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
@@ -3535,6 +3540,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)

WARN_ON(start > offset + len);

+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
end = start + frag_iter->len;
if ((copy = end - offset) > 0) {
if (copy > len)
--
2.12.2

2017-04-25 14:08:53

by Jason A. Donenfeld

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

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

2017-04-25 14:09:33

by Jason A. Donenfeld

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

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

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 22377c8ff14b..e8f862358518 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -220,7 +220,9 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
@@ -393,7 +395,9 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
skb_push(skb, ihl);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index b1e24446e297..42cb09cc8533 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -360,9 +360,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_extra(skb, esph, extra);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0)) {
+ spin_unlock_bh(&x->lock);
+ goto error;
+ }

allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);

@@ -381,11 +385,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
pfrag->offset = pfrag->offset + allocsize;

sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
- skb_to_sgvec(skb, dsg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, dsg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);

spin_unlock_bh(&x->lock);
+ if (unlikely(err < 0))
+ goto error;

goto skip_cow2;
}
@@ -422,9 +428,11 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_extra(skb, esph, extra);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0))
+ goto error;

skip_cow2:
if ((x->props.flags & XFRM_STATE_ESN))
@@ -658,7 +666,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
esp_input_set_header(skb, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg, 0, skb->len);
+ err = skb_to_sgvec(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out;

skb->ip_summed = CHECKSUM_NONE;

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index dda6035e3b84..755f38271dd5 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -423,7 +423,9 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
@@ -606,7 +608,9 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
ip6h->hop_limit = 0;

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index ff54faa75631..017e2c2d36e1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -339,9 +339,13 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_esn(skb, esph, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0)) {
+ spin_unlock_bh(&x->lock);
+ goto error;
+ }

allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);

@@ -360,12 +364,15 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
pfrag->offset = pfrag->offset + allocsize;

sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
- skb_to_sgvec(skb, dsg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, dsg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);

spin_unlock_bh(&x->lock);

+ if (unlikely(err < 0))
+ goto error;
+
goto skip_cow2;
}
}
@@ -403,9 +410,11 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_esn(skb, esph, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0))
+ goto error;

skip_cow2:
if ((x->props.flags & XFRM_STATE_ESN))
@@ -600,7 +609,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
esp_input_set_header(skb, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg, 0, skb->len);
+ ret = skb_to_sgvec(skb, sg, 0, skb->len);
+ if (unlikely(ret < 0))
+ goto out;

skb->ip_summed = CHECKSUM_NONE;

--
2.12.2

2017-04-25 14:08:44

by Jason A. Donenfeld

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

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

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..dcf46c9c3ece 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -229,7 +229,9 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
len &= ~(call->conn->size_align - 1);

sg_init_table(sg, nsg);
- skb_to_sgvec(skb, sg, 0, len);
+ err = skb_to_sgvec(skb, sg, 0, len);
+ if (unlikely(err < 0))
+ goto out;
skcipher_request_set_crypt(req, sg, sg, len, iv.x);
crypto_skcipher_encrypt(req);

@@ -342,7 +344,8 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
goto nomem;

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

/* start the decryption afresh */
memset(&iv, 0, sizeof(iv));
@@ -429,7 +432,8 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
}

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

/* decrypt from the session key */
token = call->conn->params.key->payload.data[0];
--
2.12.2

2017-04-25 14:09:21

by Jason A. Donenfeld

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

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

2017-04-25 14:16:22

by Jason A. Donenfeld

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

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

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

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

+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
WARN_ON(start > offset + len);

end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
@@ -3535,6 +3540,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)

WARN_ON(start > offset + len);

+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
end = start + frag_iter->len;
if ((copy = end - offset) > 0) {
if (copy > len)
@@ -3581,6 +3589,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
{
int nsg = __skb_to_sgvec(skb, sg, offset, len);

+ if (nsg < 0)
+ return nsg;
+
sg_mark_end(&sg[nsg - 1]);

return nsg;
--
2.12.2

2017-04-25 14:21:25

by Jason A. Donenfeld

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

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

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

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

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

+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
WARN_ON(start > offset + len);

end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
@@ -3535,6 +3540,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)

WARN_ON(start > offset + len);

+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
end = start + frag_iter->len;
if ((copy = end - offset) > 0) {
if (copy > len)
@@ -3581,6 +3589,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
{
int nsg = __skb_to_sgvec(skb, sg, offset, len);

+ if (nsg <= 0)
+ return nsg;
+
sg_mark_end(&sg[nsg - 1]);

return nsg;
--
2.12.2

2017-04-25 14:47:44

by David Miller

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

From: "Jason A. Donenfeld" <[email protected]>
Date: Tue, 25 Apr 2017 16:08:05 +0200

> This is a defense-in-depth measure in response to bugs like
> 4d6fa57b4dab0d77f4d8e9d9c73d1e63f6fe8fee.
>
> Signed-off-by: Jason A. Donenfeld <[email protected]>

Please refer to commits in the form:

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

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

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

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

Thank you.

2017-04-25 15:04:30

by Jason A. Donenfeld

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

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 <[email protected]>
---
v4 fixes the commit message and moves the check into the inner-most if.

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

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

if (copy > len)
copy = len;
@@ -3537,6 +3541,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)

end = start + frag_iter->len;
if ((copy = end - offset) > 0) {
+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
if (copy > len)
copy = len;
elt += __skb_to_sgvec(frag_iter, sg+elt, offset - start,
@@ -3581,6 +3588,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
{
int nsg = __skb_to_sgvec(skb, sg, offset, len);

+ if (nsg <= 0)
+ return nsg;
+
sg_mark_end(&sg[nsg - 1]);

return nsg;
--
2.12.2

2017-04-25 15:17:40

by David Miller

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

John this isn't how it works.

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

Thanks.

2017-04-25 15:42:57

by Sergei Shtylyov

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

Hello!

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

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

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

> Signed-off-by: Jason A. Donenfeld <[email protected]>
[...]

MBR, Sergei

2017-04-25 15:53:10

by Jason A. Donenfeld

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

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

2017-04-25 15:52:30

by Jason A. Donenfeld

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

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 <[email protected]>
---
This is a resend of v4 with all the other child commits along with it.

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

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

if (copy > len)
copy = len;
@@ -3537,6 +3541,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)

end = start + frag_iter->len;
if ((copy = end - offset) > 0) {
+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
if (copy > len)
copy = len;
elt += __skb_to_sgvec(frag_iter, sg+elt, offset - start,
@@ -3581,6 +3588,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
{
int nsg = __skb_to_sgvec(skb, sg, offset, len);

+ if (nsg <= 0)
+ return nsg;
+
sg_mark_end(&sg[nsg - 1]);

return nsg;
--
2.12.2

2017-04-25 15:53:00

by Jason A. Donenfeld

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

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

2017-04-25 15:52:45

by Jason A. Donenfeld

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

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

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 22377c8ff14b..e8f862358518 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -220,7 +220,9 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
@@ -393,7 +395,9 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
skb_push(skb, ihl);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index b1e24446e297..42cb09cc8533 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -360,9 +360,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_extra(skb, esph, extra);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0)) {
+ spin_unlock_bh(&x->lock);
+ goto error;
+ }

allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);

@@ -381,11 +385,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
pfrag->offset = pfrag->offset + allocsize;

sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
- skb_to_sgvec(skb, dsg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, dsg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);

spin_unlock_bh(&x->lock);
+ if (unlikely(err < 0))
+ goto error;

goto skip_cow2;
}
@@ -422,9 +428,11 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_extra(skb, esph, extra);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0))
+ goto error;

skip_cow2:
if ((x->props.flags & XFRM_STATE_ESN))
@@ -658,7 +666,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
esp_input_set_header(skb, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg, 0, skb->len);
+ err = skb_to_sgvec(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out;

skb->ip_summed = CHECKSUM_NONE;

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index dda6035e3b84..755f38271dd5 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -423,7 +423,9 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
@@ -606,7 +608,9 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
ip6h->hop_limit = 0;

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index ff54faa75631..017e2c2d36e1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -339,9 +339,13 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_esn(skb, esph, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0)) {
+ spin_unlock_bh(&x->lock);
+ goto error;
+ }

allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);

@@ -360,12 +364,15 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
pfrag->offset = pfrag->offset + allocsize;

sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
- skb_to_sgvec(skb, dsg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, dsg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);

spin_unlock_bh(&x->lock);

+ if (unlikely(err < 0))
+ goto error;
+
goto skip_cow2;
}
}
@@ -403,9 +410,11 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_esn(skb, esph, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0))
+ goto error;

skip_cow2:
if ((x->props.flags & XFRM_STATE_ESN))
@@ -600,7 +609,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
esp_input_set_header(skb, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg, 0, skb->len);
+ ret = skb_to_sgvec(skb, sg, 0, skb->len);
+ if (unlikely(ret < 0))
+ goto out;

skb->ip_summed = CHECKSUM_NONE;

--
2.12.2

2017-04-25 15:54:38

by Jason A. Donenfeld

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

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

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..dcf46c9c3ece 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -229,7 +229,9 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
len &= ~(call->conn->size_align - 1);

sg_init_table(sg, nsg);
- skb_to_sgvec(skb, sg, 0, len);
+ err = skb_to_sgvec(skb, sg, 0, len);
+ if (unlikely(err < 0))
+ goto out;
skcipher_request_set_crypt(req, sg, sg, len, iv.x);
crypto_skcipher_encrypt(req);

@@ -342,7 +344,8 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
goto nomem;

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

/* start the decryption afresh */
memset(&iv, 0, sizeof(iv));
@@ -429,7 +432,8 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
}

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

/* decrypt from the session key */
token = call->conn->params.key->payload.data[0];
--
2.12.2

2017-04-25 18:47:53

by Jason A. Donenfeld

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

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 <[email protected]>
---
Changes v5->v6:
* Use unlikely() for the rare overflow conditions.
* Also bound recursion, since this is a potential disaster we can avert.

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

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..24fb53f8534e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3489,16 +3489,22 @@ void __init skb_init(void)
* @len: Length of buffer space to be mapped
*
* Fill the specified scatter-gather list with mappings/pointers into a
- * region of the buffer space attached to a socket buffer.
+ * region of the buffer space attached to a socket buffer. Returns either
+ * the number of scatterlist items used, or -EMSGSIZE if the contents
+ * could not fit.
*/
static int
-__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
+__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
+ unsigned int recursion_level)
{
int start = skb_headlen(skb);
int i, copy = start - offset;
struct sk_buff *frag_iter;
int elt = 0;

+ if (unlikely(recursion_level >= 32))
+ return -EMSGSIZE;
+
if (copy > 0) {
if (copy > len)
copy = len;
@@ -3517,6 +3523,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+ if (unlikely(elt && sg_is_last(&sg[elt - 1])))
+ return -EMSGSIZE;

if (copy > len)
copy = len;
@@ -3531,16 +3539,22 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
}

skb_walk_frags(skb, frag_iter) {
- int end;
+ int end, ret;

WARN_ON(start > offset + len);

end = start + frag_iter->len;
if ((copy = end - offset) > 0) {
+ if (unlikely(elt && sg_is_last(&sg[elt - 1])))
+ return -EMSGSIZE;
+
if (copy > len)
copy = len;
- elt += __skb_to_sgvec(frag_iter, sg+elt, offset - start,
- copy);
+ ret = __skb_to_sgvec(frag_iter, sg+elt, offset - start,
+ copy, recursion_level + 1);
+ if (unlikely(ret < 0))
+ return ret;
+ elt += ret;
if ((len -= copy) == 0)
return elt;
offset += copy;
@@ -3573,13 +3587,16 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
int skb_to_sgvec_nomark(struct sk_buff *skb, struct scatterlist *sg,
int offset, int len)
{
- return __skb_to_sgvec(skb, sg, offset, len);
+ return __skb_to_sgvec(skb, sg, offset, len, 0);
}
EXPORT_SYMBOL_GPL(skb_to_sgvec_nomark);

int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
{
- int nsg = __skb_to_sgvec(skb, sg, offset, len);
+ int nsg = __skb_to_sgvec(skb, sg, offset, len, 0);
+
+ if (nsg <= 0)
+ return nsg;

sg_mark_end(&sg[nsg - 1]);

--
2.12.2

2017-04-25 18:48:20

by Jason A. Donenfeld

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

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

2017-04-25 18:48:39

by Jason A. Donenfeld

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

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

2017-04-25 18:48:30

by Jason A. Donenfeld

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

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

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 22377c8ff14b..e8f862358518 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -220,7 +220,9 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
@@ -393,7 +395,9 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
skb_push(skb, ihl);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index b1e24446e297..42cb09cc8533 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -360,9 +360,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_extra(skb, esph, extra);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0)) {
+ spin_unlock_bh(&x->lock);
+ goto error;
+ }

allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);

@@ -381,11 +385,13 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
pfrag->offset = pfrag->offset + allocsize;

sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
- skb_to_sgvec(skb, dsg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, dsg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);

spin_unlock_bh(&x->lock);
+ if (unlikely(err < 0))
+ goto error;

goto skip_cow2;
}
@@ -422,9 +428,11 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_extra(skb, esph, extra);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0))
+ goto error;

skip_cow2:
if ((x->props.flags & XFRM_STATE_ESN))
@@ -658,7 +666,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
esp_input_set_header(skb, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg, 0, skb->len);
+ err = skb_to_sgvec(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out;

skb->ip_summed = CHECKSUM_NONE;

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index dda6035e3b84..755f38271dd5 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -423,7 +423,9 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
@@ -606,7 +608,9 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
ip6h->hop_limit = 0;

sg_init_table(sg, nfrags + sglists);
- skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ err = skb_to_sgvec_nomark(skb, sg, 0, skb->len);
+ if (unlikely(err < 0))
+ goto out_free;

if (x->props.flags & XFRM_STATE_ESN) {
/* Attach seqhi sg right after packet payload */
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index ff54faa75631..017e2c2d36e1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -339,9 +339,13 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_esn(skb, esph, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0)) {
+ spin_unlock_bh(&x->lock);
+ goto error;
+ }

allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);

@@ -360,12 +364,15 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
pfrag->offset = pfrag->offset + allocsize;

sg_init_table(dsg, skb_shinfo(skb)->nr_frags + 1);
- skb_to_sgvec(skb, dsg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, dsg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);

spin_unlock_bh(&x->lock);

+ if (unlikely(err < 0))
+ goto error;
+
goto skip_cow2;
}
}
@@ -403,9 +410,11 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
esph = esp_output_set_esn(skb, esph, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg,
- (unsigned char *)esph - skb->data,
- assoclen + ivlen + clen + alen);
+ err = skb_to_sgvec(skb, sg,
+ (unsigned char *)esph - skb->data,
+ assoclen + ivlen + clen + alen);
+ if (unlikely(err < 0))
+ goto error;

skip_cow2:
if ((x->props.flags & XFRM_STATE_ESN))
@@ -600,7 +609,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
esp_input_set_header(skb, seqhi);

sg_init_table(sg, nfrags);
- skb_to_sgvec(skb, sg, 0, skb->len);
+ ret = skb_to_sgvec(skb, sg, 0, skb->len);
+ if (unlikely(ret < 0))
+ goto out;

skb->ip_summed = CHECKSUM_NONE;

--
2.12.2

2017-04-25 18:48:10

by Jason A. Donenfeld

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

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

diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..dcf46c9c3ece 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -229,7 +229,9 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
len &= ~(call->conn->size_align - 1);

sg_init_table(sg, nsg);
- skb_to_sgvec(skb, sg, 0, len);
+ err = skb_to_sgvec(skb, sg, 0, len);
+ if (unlikely(err < 0))
+ goto out;
skcipher_request_set_crypt(req, sg, sg, len, iv.x);
crypto_skcipher_encrypt(req);

@@ -342,7 +344,8 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
goto nomem;

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

/* start the decryption afresh */
memset(&iv, 0, sizeof(iv));
@@ -429,7 +432,8 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
}

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

/* decrypt from the session key */
token = call->conn->params.key->payload.data[0];
--
2.12.2

2017-04-27 09:22:01

by Jason A. Donenfeld

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

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

2017-04-27 11:30:31

by Sabrina Dubroca

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

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

Hmm, I think this can actually happen:

/* net/ipv4/ip_fragment.c */
static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
struct net_device *dev)
{

...

/* If the first fragment is fragmented itself, we split
* it to two chunks: the first with data and paged part
* and the second, holding only fragments. */
if (skb_has_frag_list(head)) {
struct sk_buff *clone;
int i, plen = 0;

clone = alloc_skb(0, GFP_ATOMIC);
if (!clone)
goto out_nomem;
clone->next = head->next;
head->next = clone;
skb_shinfo(clone)->frag_list = skb_shinfo(head)->frag_list;
skb_frag_list_init(head);
for (i = 0; i < skb_shinfo(head)->nr_frags; i++)
plen += skb_frag_size(&skb_shinfo(head)->frags[i]);
clone->len = clone->data_len = head->data_len - plen;
head->data_len -= clone->len;
head->len -= clone->len;
clone->csum = 0;
clone->ip_summed = head->ip_summed;
add_frag_mem_limit(qp->q.net, clone->truesize);
}

...
}


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

--
Sabrina

2017-04-27 12:04:52

by Jason A. Donenfeld

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

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

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

Jason

2017-04-27 14:54:36

by David Laight

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

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

You only need to recurse if both pointers are set.

David

2017-04-27 15:55:52

by David Miller

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

From: "Jason A. Donenfeld" <[email protected]>
Date: Thu, 27 Apr 2017 11:21:51 +0200

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

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

2017-04-28 11:41:57

by Sabrina Dubroca

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

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

Adding a few more lines of context:

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

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

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

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



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

You might also want to add a cover letter.

--
Sabrina

2017-04-28 13:30:06

by Jason A. Donenfeld

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

Hi Sabrina,

Thanks for the review.

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

2017-04-28 16:18:54

by Sabrina Dubroca

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

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

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

Thanks!

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

--
Sabrina

2017-04-28 22:47:47

by Jason A. Donenfeld

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

Hi Sabrina,

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