Further fixes for 48a1df65334b

PATCH Applied Nathan Chancellor 2 participants 16 messages Apr 10, 2018

Subsystems (3)

Network drivers Stable branch virtio net driver

lore

Share & tools

lore

Nathan Chancellor

author

2018-04-10 01:22 UTC #0 lore raw

The backport of 48a1df65334b ("skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow") was missing three supplemental commits:

3f29770723fe ("ipsec: check return value of skb_to_sgvec always") 89a5ea996625 ("rxrpc: check return value of skb_to_sgvec always") e2fcad58fd23 ("virtio_net: check return value of skb_to_sgvec always")

I already sent the first one, this thread contains the other two for 3.18, 4.4, and 4.9. There is an additional patch because these trees do not contain f6b10209b90d ("virtio-net: switch to use build_skb() for small buffer"), which removed another call to skb_to_sgvec, which was before f6b10209b90d. If you feel it should be squashed in f6b10209b90d, please by all means feel free to do so. I was not 100% confident on if the dev_kfree_skb call was necessary in the error path so I hope one of the authors can comment on that.

Thanks!
Nathan

Nathan Chancellor

author

2018-04-10 01:22 UTC #1 lore raw

PATCH 1/3 rxrpc: check return value of skb_to_sgvec always

From: "Jason A. Donenfeld" <[email protected]>

commit 89a5ea99662505d2d61f2a3030a6896c2cb3cdb0 upstream.

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

Acked-by David Howells <[email protected]>

Signed-off-by David S. Miller <[email protected]>

[natechancellor: backport to 3.18]

Signed-off-by Nathan Chancellor <[email protected]>

---
 net/rxrpc/rxkad.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

net/rxrpc/rxkad.c +15 -6

index f226709ebd8f..ca5f3662a485 100644
@@ -209,7 +209,7 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
 	struct sk_buff *trailer;
 	unsigned int len;
 	u16 check;
-	int nsg;
+	int nsg, err;
 
 	sp = rxrpc_skb(skb);

@@ -240,7 +240,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))
+		return err;
 	crypto_blkcipher_encrypt_iv(&desc, sg, sg, len);
 
 	_leave(" = 0");

@@ -336,7 +338,7 @@ static int rxkad_verify_packet_auth(const struct rxrpc_call *call,
 	struct sk_buff *trailer;
 	u32 data_size, buf;
 	u16 check;
-	int nsg;
+	int nsg, ret;
 
 	_enter("");

@@ -348,7 +350,9 @@ static int rxkad_verify_packet_auth(const struct rxrpc_call *call,
 		goto nomem;
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, 0, 8);
+	ret = skb_to_sgvec(skb, sg, 0, 8);
+	if (unlikely(ret < 0))
+		return ret;
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));

@@ -411,7 +415,7 @@ static int rxkad_verify_packet_encrypt(const struct rxrpc_call *call,
 	struct sk_buff *trailer;
 	u32 data_size, buf;
 	u16 check;
-	int nsg;
+	int nsg, ret;
 
 	_enter(",{%d}", skb->len);

@@ -430,7 +434,12 @@ static int rxkad_verify_packet_encrypt(const struct rxrpc_call *call,
 	}
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0)) {
+		if (sg != _sg)
+			kfree(sg);
+		return ret;
+	}
 
 	/* decrypt from the session key */
 	token = call->conn->key->payload.data;

Signature · 1 line

2.17.0

Nathan Chancellor

author

2018-04-10 01:22 UTC #2 lore raw

PATCH 1/3 rxrpc: check return value of skb_to_sgvec always

In reply to Nathan Chancellor

From: "Jason A. Donenfeld" <[email protected]>

commit 89a5ea99662505d2d61f2a3030a6896c2cb3cdb0 upstream.

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

Acked-by David Howells <[email protected]>

Signed-off-by David S. Miller <[email protected]>

[natechancellor: backport to 4.4]

Signed-off-by Nathan Chancellor <[email protected]>

---
 net/rxrpc/rxkad.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

net/rxrpc/rxkad.c +15 -6

index d7a9ab5a9d9c..6c65fb229e50 100644
@@ -209,7 +209,7 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
 	struct sk_buff *trailer;
 	unsigned int len;
 	u16 check;
-	int nsg;
+	int nsg, err;
 
 	sp = rxrpc_skb(skb);

@@ -240,7 +240,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))
+		return err;
 	crypto_blkcipher_encrypt_iv(&desc, sg, sg, len);
 
 	_leave(" = 0");

@@ -336,7 +338,7 @@ static int rxkad_verify_packet_auth(const struct rxrpc_call *call,
 	struct sk_buff *trailer;
 	u32 data_size, buf;
 	u16 check;
-	int nsg;
+	int nsg, ret;
 
 	_enter("");

@@ -348,7 +350,9 @@ static int rxkad_verify_packet_auth(const struct rxrpc_call *call,
 		goto nomem;
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, 0, 8);
+	ret = skb_to_sgvec(skb, sg, 0, 8);
+	if (unlikely(ret < 0))
+		return ret;
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));

@@ -411,7 +415,7 @@ static int rxkad_verify_packet_encrypt(const struct rxrpc_call *call,
 	struct sk_buff *trailer;
 	u32 data_size, buf;
 	u16 check;
-	int nsg;
+	int nsg, ret;
 
 	_enter(",{%d}", skb->len);

@@ -430,7 +434,12 @@ static int rxkad_verify_packet_encrypt(const struct rxrpc_call *call,
 	}
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, 0, skb->len);
+	ret = skb_to_sgvec(skb, sg, 0, skb->len);
+	if (unlikely(ret < 0)) {
+		if (sg != _sg)
+			kfree(sg);
+		return ret;
+	}
 
 	/* decrypt from the session key */
 	token = call->conn->key->payload.data[0];

Signature · 1 line

2.17.0

Nathan Chancellor

author

2018-04-10 01:22 UTC #3 lore raw

PATCH 1/3 rxrpc: check return value of skb_to_sgvec always

In reply to Nathan Chancellor

From: "Jason A. Donenfeld" <[email protected]>

commit 89a5ea99662505d2d61f2a3030a6896c2cb3cdb0 upstream.

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

Acked-by David Howells <[email protected]>

Signed-off-by David S. Miller <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

---
 net/rxrpc/rxkad.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

net/rxrpc/rxkad.c +14 -5

index 4374e7b9c7bf..90df95e18ea4 100644
@@ -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);

@@ -325,7 +327,7 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
 	struct sk_buff *trailer;
 	u32 data_size, buf;
 	u16 check;
-	int nsg;
+	int nsg, ret;
 
 	_enter("");

@@ -342,7 +344,9 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
 		goto nomem;
 
 	sg_init_table(sg, nsg);
-	skb_to_sgvec(skb, sg, offset, 8);
+	ret = skb_to_sgvec(skb, sg, offset, 8);
+	if (unlikely(ret < 0))
+		return ret;
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));

@@ -405,7 +409,7 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
 	struct sk_buff *trailer;
 	u32 data_size, buf;
 	u16 check;
-	int nsg;
+	int nsg, ret;
 
 	_enter(",{%d}", skb->len);

@@ -429,7 +433,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];

Signature · 1 line

2.17.0

Nathan Chancellor

author

2018-04-10 01:22 UTC #4 lore raw

PATCH 2/3 virtio_net: check return value of skb_to_sgvec always

In reply to Nathan Chancellor

From: "Jason A. Donenfeld" <[email protected]>

commit e2fcad58fd230f635a74e4e983c6f4ea893642d2 upstream.

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

Reviewed-by Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Signed-off-by David S. Miller <[email protected]>

[natechancellor: backport to 3.18]

Signed-off-by Nathan Chancellor <[email protected]>

---
 drivers/net/virtio_net.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/net/virtio_net.c +7 -2

index ce2a29971230..ede40841e962 100644
@@ -854,7 +854,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	struct skb_vnet_hdr *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;
 	bool can_push;

@@ -906,11 +906,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);
 }

Signature · 1 line

2.17.0

Nathan Chancellor

author

2018-04-10 01:22 UTC #5 lore raw

PATCH 2/3 virtio_net: check return value of skb_to_sgvec always

In reply to Nathan Chancellor

From: "Jason A. Donenfeld" <[email protected]>

commit e2fcad58fd230f635a74e4e983c6f4ea893642d2 upstream.

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

Reviewed-by Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Signed-off-by David S. Miller <[email protected]>

[natechancellor: backport to 4.4]

Signed-off-by Nathan Chancellor <[email protected]>

---
 drivers/net/virtio_net.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/net/virtio_net.c +7 -2

index 8dfc75250583..bb2bae522f67 100644
@@ -858,7 +858,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;

@@ -911,11 +911,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);
 }

Signature · 1 line

2.17.0

Nathan Chancellor

author

2018-04-10 01:22 UTC #6 lore raw

PATCH 2/3 virtio_net: check return value of skb_to_sgvec always

In reply to Nathan Chancellor

From: "Jason A. Donenfeld" <[email protected]>

commit e2fcad58fd230f635a74e4e983c6f4ea893642d2 upstream.

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

Reviewed-by Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Signed-off-by David S. Miller <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

---
 drivers/net/virtio_net.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/net/virtio_net.c +7 -2

index 1568aedddfc9..0a2f3ac36be2 100644
@@ -831,7 +831,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;

@@ -858,11 +858,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);
 }

Signature · 1 line

2.17.0

Nathan Chancellor

author

2018-04-10 01:22 UTC #7 lore raw

PATCH 3/3 virtio_net: check return value of skb_to_sgvec in one more location

In reply to Nathan Chancellor

Kernels that do not have f6b10209b90d ("virtio-net: switch to use build_skb() for small buffer") will have an extra call to skb_to_sgvec that is not handled by e2fcad58fd23 ("virtio_net: check return value of skb_to_sgvec always"). Since the former does not appear to be stable material, just fix the call up directly.

Cc Jason A. Donenfeld <[email protected]>

Cc Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Cc David S. Miller <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

---

Greg, if you feel this should be squashed into the previous commit, by all means, feel free. I am not entirely sure if the dev_kfree_skb is necessary here, I was hoping to get some review.

---
 drivers/net/virtio_net.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/virtio_net.c +6 -1

index ede40841e962..1da54735681b 100644
@@ -551,7 +551,12 @@ static int add_recvbuf_small(struct receive_queue *rq, gfp_t gfp)
 	hdr = skb_vnet_hdr(skb);
 	sg_init_table(rq->sg, MAX_SKB_FRAGS + 2);
 	sg_set_buf(rq->sg, &hdr->hdr, sizeof hdr->hdr);
-	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+
+	err = skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+	if (unlikely(err < 0)) {
+		dev_kfree_skb(skb);
+		return err;
+	}
 
 	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
 	if (err < 0)

Signature · 1 line

2.17.0

[email protected]

maintainer Applied

2018-04-10 12:37 UTC #8 lore raw

Patch "virtio_net: check return value of skb_to_sgvec in one more location" has been added to the 3.18-stable tree

This is a note to let you know that I've just added the patch titled

virtio_net: check return value of skb_to_sgvec in one more location

to the 3.18-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
virtio_net-check-return-value-of-skb_to_sgvec-in-one-more-location.patch
and it can be found in the queue-3.18 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From [email protected] Tue Apr 10 14:13:23 2018

From: Nathan Chancellor <[email protected]>
Date: Mon, 9 Apr 2018 18:21:44 -0700
Subject: virtio_net: check return value of skb_to_sgvec in one more location
To: Greg Kroah-Hartman <[email protected]>, [email protected]
Cc: Nathan Chancellor <[email protected]>, "Jason A . Donenfeld" <[email protected]>, Sergei Shtylyov <[email protected]>, "Michael S. Tsirkin" <[email protected]>, Jason Wang <[email protected]>, "David S . Miller" <[email protected]>
Message-ID: <[email protected]>

From: Nathan Chancellor <[email protected]>

Cc Jason A. Donenfeld <[email protected]>

Cc Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Cc David S. Miller <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/net/virtio_net.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/virtio_net.c +6 -1

@@ -551,7 +551,12 @@ static int add_recvbuf_small(struct rece
 	hdr = skb_vnet_hdr(skb);
 	sg_init_table(rq->sg, MAX_SKB_FRAGS + 2);
 	sg_set_buf(rq->sg, &hdr->hdr, sizeof hdr->hdr);
-	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+
+	err = skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+	if (unlikely(err < 0)) {
+		dev_kfree_skb(skb);
+		return err;
+	}
 
 	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
 	if (err < 0)

Patches currently in stable-queue which might be from [email protected] are

queue-3.18/ipsec-check-return-value-of-skb_to_sgvec-always.patch queue-3.18/rxrpc-check-return-value-of-skb_to_sgvec-always.patch queue-3.18/virtio_net-check-return-value-of-skb_to_sgvec-always.patch queue-3.18/virtio_net-check-return-value-of-skb_to_sgvec-in-one-more-location.patch

Nathan Chancellor

author

2018-04-10 01:22 UTC #9 lore raw

PATCH 3/3 virtio_net: check return value of skb_to_sgvec in one more location

In reply to Nathan Chancellor

Cc Jason A. Donenfeld <[email protected]>

Cc Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Cc David S. Miller <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

---

Greg, if you feel this should be squashed into the previous commit, by all means, feel free. I am not entirely sure if the dev_kfree_skb is necessary here, I was hoping to get some review.

---
 drivers/net/virtio_net.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/virtio_net.c +6 -1

index bb2bae522f67..d01285250204 100644
@@ -556,7 +556,12 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
 	hdr = skb_vnet_hdr(skb);
 	sg_init_table(rq->sg, 2);
 	sg_set_buf(rq->sg, hdr, vi->hdr_len);
-	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+
+	err = skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+	if (unlikely(err < 0)) {
+		dev_kfree_skb(skb);
+		return err;
+	}
 
 	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
 	if (err < 0)

Signature · 1 line

2.17.0

[email protected]

maintainer Applied

2018-04-10 09:09 UTC #10 lore raw

Patch "virtio_net: check return value of skb_to_sgvec in one more location" has been added to the 4.4-stable tree

This is a note to let you know that I've just added the patch titled

virtio_net: check return value of skb_to_sgvec in one more location

to the 4.4-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
virtio_net-check-return-value-of-skb_to_sgvec-in-one-more-location.patch
and it can be found in the queue-4.4 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From [email protected] Tue Apr 10 10:36:56 2018

From: Nathan Chancellor <[email protected]>
Date: Mon, 9 Apr 2018 18:21:47 -0700
Subject: virtio_net: check return value of skb_to_sgvec in one more location
To: Greg Kroah-Hartman <[email protected]>, [email protected]
Cc: Nathan Chancellor <[email protected]>, "Jason A . Donenfeld" <[email protected]>, Sergei Shtylyov <[email protected]>, "Michael S. Tsirkin" <[email protected]>, Jason Wang <[email protected]>, "David S . Miller" <[email protected]>
Message-ID: <[email protected]>

From: Nathan Chancellor <[email protected]>

Cc Jason A. Donenfeld <[email protected]>

Cc Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Cc David S. Miller <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/net/virtio_net.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/virtio_net.c +6 -1

@@ -556,7 +556,12 @@ static int add_recvbuf_small(struct virt
 	hdr = skb_vnet_hdr(skb);
 	sg_init_table(rq->sg, 2);
 	sg_set_buf(rq->sg, hdr, vi->hdr_len);
-	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+
+	err = skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+	if (unlikely(err < 0)) {
+		dev_kfree_skb(skb);
+		return err;
+	}
 
 	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
 	if (err < 0)

Patches currently in stable-queue which might be from [email protected] are

queue-4.4/ipsec-check-return-value-of-skb_to_sgvec-always.patch queue-4.4/rxrpc-check-return-value-of-skb_to_sgvec-always.patch queue-4.4/virtio_net-check-return-value-of-skb_to_sgvec-always.patch queue-4.4/virtio_net-check-return-value-of-skb_to_sgvec-in-one-more-location.patch

Nathan Chancellor

author

2018-04-10 01:22 UTC #11 lore raw

PATCH 3/3 virtio_net: check return value of skb_to_sgvec in one more location

In reply to Nathan Chancellor

Cc Jason A. Donenfeld <[email protected]>

Cc Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Cc David S. Miller <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

---

Greg, if you feel this should be squashed into the previous commit, by all means, feel free. I am not entirely sure if the dev_kfree_skb is necessary here, I was hoping to get some review.

---
 drivers/net/virtio_net.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/virtio_net.c +6 -1

index 0a2f3ac36be2..472ed6df2221 100644
@@ -529,7 +529,12 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
 	hdr = skb_vnet_hdr(skb);
 	sg_init_table(rq->sg, 2);
 	sg_set_buf(rq->sg, hdr, vi->hdr_len);
-	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+
+	err = skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+	if (unlikely(err < 0)) {
+		dev_kfree_skb(skb);
+		return err;
+	}
 
 	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
 	if (err < 0)

Signature · 1 line

2.17.0

Greg Kroah-Hartman

maintainer

2018-04-10 06:35 UTC #12 lore raw

Re: [PATCH 4.9 3/3] virtio_net: check return value of skb_to_sgvec in one more location

Quoted 12 lines · Nathan Chancellor

Kernels that do not have f6b10209b90d ("virtio-net: switch to use build_skb() for small buffer") will have an extra call to skb_to_sgvec that is not handled by e2fcad58fd23 ("virtio_net: check return value of skb_to_sgvec always"). Since the former does not appear to be stable material, just fix the call up directly.
Cc Jason A. Donenfeld <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc "Michael S. Tsirkin" <[email protected]>
Cc Jason Wang <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>

Quoted 5 lines

---
Greg, if you feel this should be squashed into the previous commit, by all means, feel free. I am not entirely sure if the dev_kfree_skb is necessary here, I was hoping to get some review.

You did the right thing here by "not squashing" this into the previous patch, thanks!

greg k-h

Greg Kroah-Hartman

maintainer

2018-04-10 07:11 UTC #13 lore raw

Re: [PATCH 4.9 3/3] virtio_net: check return value of skb_to_sgvec in one more location

Quoted 15 lines · Greg Kroah-Hartman

Quoted 14 lines · Nathan Chancellor
Kernels that do not have f6b10209b90d ("virtio-net: switch to use build_skb() for small buffer") will have an extra call to skb_to_sgvec that is not handled by e2fcad58fd23 ("virtio_net: check return value of skb_to_sgvec always"). Since the former does not appear to be stable material, just fix the call up directly.
Cc Jason A. Donenfeld <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc "Michael S. Tsirkin" <[email protected]>
Cc Jason Wang <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>
---

Quoted 6 lines

Quoted 3 lines
Greg, if you feel this should be squashed into the previous commit, by all means, feel free. I am not entirely sure if the dev_kfree_skb is necessary here, I was hoping to get some review.
You did the right thing here by "not squashing" this into the previous patch, thanks!

And all 3 of these fixed up the remaining 4.9.y build issues, many thanks for these.

greg k-h

[email protected]

maintainer Applied

2018-04-10 07:12 UTC #14 lore raw

Patch "virtio_net: check return value of skb_to_sgvec in one more location" has been added to the 4.9-stable tree

In reply to Nathan Chancellor

This is a note to let you know that I've just added the patch titled

virtio_net: check return value of skb_to_sgvec in one more location

to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
virtio_net-check-return-value-of-skb_to_sgvec-in-one-more-location.patch
and it can be found in the queue-4.9 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From [email protected] Tue Apr 10 08:34:50 2018

From: Nathan Chancellor <[email protected]>
Date: Mon, 9 Apr 2018 18:21:50 -0700
Subject: virtio_net: check return value of skb_to_sgvec in one more location
To: Greg Kroah-Hartman <[email protected]>, [email protected]
Cc: Nathan Chancellor <[email protected]>, "Jason A . Donenfeld" <[email protected]>, Sergei Shtylyov <[email protected]>, "Michael S. Tsirkin" <[email protected]>, Jason Wang <[email protected]>, "David S . Miller" <[email protected]>
Message-ID: <[email protected]>

From: Nathan Chancellor <[email protected]>

Cc Jason A. Donenfeld <[email protected]>

Cc Sergei Shtylyov <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Cc Jason Wang <[email protected]>

Cc David S. Miller <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/net/virtio_net.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/virtio_net.c +6 -1

@@ -529,7 +529,12 @@ static int add_recvbuf_small(struct virt
 	hdr = skb_vnet_hdr(skb);
 	sg_init_table(rq->sg, 2);
 	sg_set_buf(rq->sg, hdr, vi->hdr_len);
-	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+
+	err = skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+	if (unlikely(err < 0)) {
+		dev_kfree_skb(skb);
+		return err;
+	}
 
 	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
 	if (err < 0)

Patches currently in stable-queue which might be from [email protected] are

queue-4.9/ipsec-check-return-value-of-skb_to_sgvec-always.patch queue-4.9/rxrpc-check-return-value-of-skb_to_sgvec-always.patch queue-4.9/virtio_net-check-return-value-of-skb_to_sgvec-always.patch queue-4.9/virtio_net-check-return-value-of-skb_to_sgvec-in-one-more-location.patch

Greg Kroah-Hartman

maintainer

2018-04-10 12:53 UTC #15 lore raw

In reply to Nathan Chancellor

Quoted 7 lines · Nathan Chancellor

The backport of 48a1df65334b ("skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow") was missing three supplemental commits:
3f29770723fe ("ipsec: check return value of skb_to_sgvec always") 89a5ea996625 ("rxrpc: check return value of skb_to_sgvec always") e2fcad58fd23 ("virtio_net: check return value of skb_to_sgvec always")

Quoted 8 lines

I already sent the first one, this thread contains the other two for 3.18, 4.4, and 4.9. There is an additional patch because these trees do not contain f6b10209b90d ("virtio-net: switch to use build_skb() for small buffer"), which removed another call to skb_to_sgvec, which was before f6b10209b90d. If you feel it should be squashed in f6b10209b90d, please by all means feel free to do so. I was not 100% confident on if the dev_kfree_skb call was necessary in the error path so I hope one of the authors can comment on that.

Thanks for all of these, now queued up.

greg k-h