From: Xie Yongji xieyongji@bytedance.com
commit ad993a95c508 ("virtio-net: Add validation for used length")
This adds validation for used length (might come from an untrusted device) to avoid data corruption or loss.
Signed-off-by: Xie Yongji xieyongji@bytedance.com Acked-by: Jason Wang jasowang@redhat.com Link: https://lore.kernel.org/r/20210531135852.113-1-xieyongji@bytedance.com Signed-off-by: Jakub Kicinski kuba@kernel.org (cherry picked from commit ad993a95c508417acdeb15244109e009e50d8758) [Larry: backport to 5.4.y. Minor conflict resolved due to missing commit 9ce6146ec7b50 virtio_net: Add XDP frame size in two code paths] Signed-off-by: Larry Bassel larry.bassel@oracle.com --- drivers/net/virtio_net.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 182b67270044..215c546bf50a 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -717,6 +717,12 @@ static struct sk_buff *receive_small(struct net_device *dev, len -= vi->hdr_len; stats->bytes += len;
+ if (unlikely(len > GOOD_PACKET_LEN)) { + pr_debug("%s: rx error: len %u exceeds max size %d\n", + dev->name, len, GOOD_PACKET_LEN); + dev->stats.rx_length_errors++; + goto err_len; + } rcu_read_lock(); xdp_prog = rcu_dereference(rq->xdp_prog); if (xdp_prog) { @@ -819,6 +825,7 @@ static struct sk_buff *receive_small(struct net_device *dev, err_xdp: rcu_read_unlock(); stats->xdp_drops++; +err_len: stats->drops++; put_page(page); xdp_xmit: @@ -871,6 +878,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, head_skb = NULL; stats->bytes += len - vi->hdr_len;
+ truesize = mergeable_ctx_to_truesize(ctx); + if (unlikely(len > truesize)) { + pr_debug("%s: rx error: len %u exceeds truesize %lu\n", + dev->name, len, (unsigned long)ctx); + dev->stats.rx_length_errors++; + goto err_skb; + } rcu_read_lock(); xdp_prog = rcu_dereference(rq->xdp_prog); if (xdp_prog) { @@ -990,14 +1004,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, } rcu_read_unlock();
- truesize = mergeable_ctx_to_truesize(ctx); - if (unlikely(len > truesize)) { - pr_debug("%s: rx error: len %u exceeds truesize %lu\n", - dev->name, len, (unsigned long)ctx); - dev->stats.rx_length_errors++; - goto err_skb; - } - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog, metasize); curr_skb = head_skb;
Hi Larry,
On 26/03/25 05:14, Larry Bassel wrote:
From: Xie Yongji xieyongji@bytedance.com
commit ad993a95c508 ("virtio-net: Add validation for used length")
I understand checkpatch.pl warned you, but for stable patches this should still be [ Upstream commit ad993a95c508417acdeb15244109e009e50d8758 ]
Stable maintainers, do you think it is good idea to tweak checkpatch.pl to detect these are backports(with help of Upstream commit, commit .. upstream, or cherrypicked from lines ?) and it shouldn't warn about long SHA ?
Thanks, Harshit
This adds validation for used length (might come from an untrusted device) to avoid data corruption or loss.
Signed-off-by: Xie Yongji xieyongji@bytedance.com Acked-by: Jason Wang jasowang@redhat.com Link: https://lore.kernel.org/r/20210531135852.113-1-xieyongji@bytedance.com Signed-off-by: Jakub Kicinski kuba@kernel.org (cherry picked from commit ad993a95c508417acdeb15244109e009e50d8758) [Larry: backport to 5.4.y. Minor conflict resolved due to missing commit 9ce6146ec7b50 virtio_net: Add XDP frame size in two code paths] Signed-off-by: Larry Bassel larry.bassel@oracle.com
drivers/net/virtio_net.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 182b67270044..215c546bf50a 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -717,6 +717,12 @@ static struct sk_buff *receive_small(struct net_device *dev, len -= vi->hdr_len; stats->bytes += len;
- if (unlikely(len > GOOD_PACKET_LEN)) {
pr_debug("%s: rx error: len %u exceeds max size %d\n",dev->name, len, GOOD_PACKET_LEN);dev->stats.rx_length_errors++;goto err_len;- } rcu_read_lock(); xdp_prog = rcu_dereference(rq->xdp_prog); if (xdp_prog) {
@@ -819,6 +825,7 @@ static struct sk_buff *receive_small(struct net_device *dev, err_xdp: rcu_read_unlock(); stats->xdp_drops++; +err_len: stats->drops++; put_page(page); xdp_xmit: @@ -871,6 +878,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, head_skb = NULL; stats->bytes += len - vi->hdr_len;
- truesize = mergeable_ctx_to_truesize(ctx);
- if (unlikely(len > truesize)) {
pr_debug("%s: rx error: len %u exceeds truesize %lu\n",dev->name, len, (unsigned long)ctx);dev->stats.rx_length_errors++;goto err_skb;- } rcu_read_lock(); xdp_prog = rcu_dereference(rq->xdp_prog); if (xdp_prog) {
@@ -990,14 +1004,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, } rcu_read_unlock();
- truesize = mergeable_ctx_to_truesize(ctx);
- if (unlikely(len > truesize)) {
pr_debug("%s: rx error: len %u exceeds truesize %lu\n",dev->name, len, (unsigned long)ctx);dev->stats.rx_length_errors++;goto err_skb;- }
- head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog, metasize); curr_skb = head_skb;
On Wed, Mar 26, 2025 at 06:32:19AM +0530, Harshit Mogalapalli wrote:
Hi Larry,
On 26/03/25 05:14, Larry Bassel wrote:
From: Xie Yongji xieyongji@bytedance.com
commit ad993a95c508 ("virtio-net: Add validation for used length")
I understand checkpatch.pl warned you, but for stable patches this should still be [ Upstream commit ad993a95c508417acdeb15244109e009e50d8758 ]
Stable maintainers, do you think it is good idea to tweak checkpatch.pl to detect these are backports(with help of Upstream commit, commit .. upstream, or cherrypicked from lines ?) and it shouldn't warn about long SHA ?
Nope! Why would you ever run checkpatch on a patch that is already upstream?
confused,
greg k-h
Hi Greg,
On 26/03/25 06:47, Greg KH wrote:
On Wed, Mar 26, 2025 at 06:32:19AM +0530, Harshit Mogalapalli wrote:
Hi Larry,
On 26/03/25 05:14, Larry Bassel wrote:
From: Xie Yongji xieyongji@bytedance.com
commit ad993a95c508 ("virtio-net: Add validation for used length")
I understand checkpatch.pl warned you, but for stable patches this should still be [ Upstream commit ad993a95c508417acdeb15244109e009e50d8758 ]
Stable maintainers, do you think it is good idea to tweak checkpatch.pl to detect these are backports(with help of Upstream commit, commit .. upstream, or cherrypicked from lines ?) and it shouldn't warn about long SHA ?
Nope! Why would you ever run checkpatch on a patch that is already upstream?
Ah right, not in this case but it might help when the backport is a bit different from the upstream patch(i.e after conflict resolution if the line in code exceeds 80 chars) -- checkpatch.pl might help us do it in the right way ? (only in a case where there are changes between current upstream code and the stable branch where we are backporting to)
Thanks, Harshit
confused,
greg k-h
On Wed, Mar 26, 2025 at 07:02:38AM +0530, Harshit Mogalapalli wrote:
Hi Greg,
On 26/03/25 06:47, Greg KH wrote:
On Wed, Mar 26, 2025 at 06:32:19AM +0530, Harshit Mogalapalli wrote:
Hi Larry,
On 26/03/25 05:14, Larry Bassel wrote:
From: Xie Yongji xieyongji@bytedance.com
commit ad993a95c508 ("virtio-net: Add validation for used length")
I understand checkpatch.pl warned you, but for stable patches this should still be [ Upstream commit ad993a95c508417acdeb15244109e009e50d8758 ]
Stable maintainers, do you think it is good idea to tweak checkpatch.pl to detect these are backports(with help of Upstream commit, commit .. upstream, or cherrypicked from lines ?) and it shouldn't warn about long SHA ?
Nope! Why would you ever run checkpatch on a patch that is already upstream?
Ah right, not in this case but it might help when the backport is a bit different from the upstream patch(i.e after conflict resolution if the line in code exceeds 80 chars) -- checkpatch.pl might help us do it in the right way ? (only in a case where there are changes between current upstream code and the stable branch where we are backporting to)
Then run checkpatch like normal to catch that if you feel you need it, you know to ignore foolish warnings from checkpatch, that's just normal.
People doing backports better be experienced kernel developers as this is NOT a task for newbies for obvious reasons. Which is maybe why no one has ever brought this up in the past 15+ years we have had stable kernels? :)
thanks,
greg k-h
Hi Greg,
On 26/03/25 07:07, Greg KH wrote:
On Wed, Mar 26, 2025 at 07:02:38AM +0530, Harshit Mogalapalli wrote:
Hi Greg,
On 26/03/25 06:47, Greg KH wrote:
On Wed, Mar 26, 2025 at 06:32:19AM +0530, Harshit Mogalapalli wrote:
Hi Larry,
On 26/03/25 05:14, Larry Bassel wrote:
From: Xie Yongji xieyongji@bytedance.com
commit ad993a95c508 ("virtio-net: Add validation for used length")
I understand checkpatch.pl warned you, but for stable patches this should still be [ Upstream commit ad993a95c508417acdeb15244109e009e50d8758 ]
Stable maintainers, do you think it is good idea to tweak checkpatch.pl to detect these are backports(with help of Upstream commit, commit .. upstream, or cherrypicked from lines ?) and it shouldn't warn about long SHA ?
Nope! Why would you ever run checkpatch on a patch that is already upstream?
Ah right, not in this case but it might help when the backport is a bit different from the upstream patch(i.e after conflict resolution if the line in code exceeds 80 chars) -- checkpatch.pl might help us do it in the right way ? (only in a case where there are changes between current upstream code and the stable branch where we are backporting to)
Then run checkpatch like normal to catch that if you feel you need it, you know to ignore foolish warnings from checkpatch, that's just normal.
Agree, that's the best approach!
Thank you.
Regards, Harshit
People doing backports better be experienced kernel developers as this is NOT a task for newbies for obvious reasons. Which is maybe why no one has ever brought this up in the past 15+ years we have had stable kernels? :)
thanks,
greg k-h
[ Sasha's backport helper bot ]
Hi,
Summary of potential issues: ⚠️ Found matching upstream commit but patch is missing proper reference to it
Found matching upstream commit: ad993a95c508417acdeb15244109e009e50d8758
WARNING: Author mismatch between patch and found commit: Backport author: Larry Bassellarry.bassel@oracle.com Commit author: Xie Yongjixieyongji@bytedance.com
Status in newer kernel trees: 6.13.y | Present (exact SHA1) 6.12.y | Present (exact SHA1) 6.6.y | Present (exact SHA1) 6.1.y | Present (exact SHA1) 5.15.y | Present (exact SHA1) 5.10.y | Present (different SHA1: c92298d228f6)
Note: The patch differs from the upstream commit: --- 1: ad993a95c5084 ! 1: 34cd3a563c1ba virtio-net: Add validation for used length @@ Metadata ## Commit message ## virtio-net: Add validation for used length
+ commit ad993a95c508 ("virtio-net: Add validation for used length") + This adds validation for used length (might come from an untrusted device) to avoid data corruption or loss. @@ Commit message Acked-by: Jason Wang jasowang@redhat.com Link: https://lore.kernel.org/r/20210531135852.113-1-xieyongji@bytedance.com Signed-off-by: Jakub Kicinski kuba@kernel.org + (cherry picked from commit ad993a95c508417acdeb15244109e009e50d8758) + [Larry: backport to 5.4.y. Minor conflict resolved due to missing commit 9ce6146ec7b50 + virtio_net: Add XDP frame size in two code paths] + Signed-off-by: Larry Bassel larry.bassel@oracle.com
## drivers/net/virtio_net.c ## @@ drivers/net/virtio_net.c: static struct sk_buff *receive_small(struct net_device *dev, @@ drivers/net/virtio_net.c: static struct sk_buff *receive_mergeable(struct net_de head_skb = NULL; stats->bytes += len - vi->hdr_len;
++ truesize = mergeable_ctx_to_truesize(ctx); + if (unlikely(len > truesize)) { + pr_debug("%s: rx error: len %u exceeds truesize %lu\n", + dev->name, len, (unsigned long)ctx); @@ drivers/net/virtio_net.c: static struct sk_buff *receive_mergeable(struct net_de } rcu_read_unlock();
+- truesize = mergeable_ctx_to_truesize(ctx); - if (unlikely(len > truesize)) { - pr_debug("%s: rx error: len %u exceeds truesize %lu\n", - dev->name, len, (unsigned long)ctx); @@ drivers/net/virtio_net.c: static struct sk_buff *receive_mergeable(struct net_de - } - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog, - metasize, !!headroom); + metasize); curr_skb = head_skb; ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-5.4.y | Success | Success |
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Harshit Mogalapalli -
Larry Bassel -
Sasha Levin