[PATCH 5.4.y] virtio-net: Add validation for used length

[PATCH 5.4.y] virtio-net: Add validation for used length

[Larry Bassel]

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;
-- 
2.46.0

Re: [PATCH 5.4.y] virtio-net: Add validation for used length

[Harshit Mogalapalli]

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;

Re: [PATCH 5.4.y] virtio-net: Add validation for used length

[Greg KH]

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

Re: [PATCH 5.4.y] virtio-net: Add validation for used length

[Harshit Mogalapalli]

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

Re: [PATCH 5.4.y] virtio-net: Add validation for used length

[Greg KH]

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

Re: [PATCH 5.4.y] virtio-net: Add validation for used length

[Harshit Mogalapalli]

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

Re: [PATCH 5.4.y] virtio-net: Add validation for used length

[Sasha Levin]

[ 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 Bassel<larry.bassel@oracle.com>
Commit author: Xie Yongji<xieyongji@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   |