[PATCH net v2 1/2] virtio-net: fix len check in receive_big()

[PATCH net v2 1/2] virtio-net: fix len check in receive_big()

[Xiang Mei]

receive_big() bounds the device-announced length by
(big_packets_num_skbfrags + 1) * PAGE_SIZE.  That is still too loose:
add_recvbuf_big() sets sg[1] to start at offset
sizeof(struct padded_vnet_hdr) into the first page, so the chain
actually carries hdr_len + (PAGE_SIZE - sizeof(padded_vnet_hdr)) +
big_packets_num_skbfrags * PAGE_SIZE bytes -- 20 bytes less than the
check allows for the common hdr_len == 12 case.

A malicious virtio backend can announce a len in that gap.  page_to_skb()
then walks one frag past the page chain, storing a NULL page->private
into skb_shinfo()->frags[MAX_SKB_FRAGS], which is both an out-of-bounds
write past the static frag array and a NULL frag handed up the rx path.

Bound len by the size add_recvbuf_big() actually advertised.

Fixes: 0c716703965f ("virtio-net: fix received length check in big packets")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
v2: add 2/2 for robustness

 drivers/net/virtio_net.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f4adcfee7a80..afe73eda1491 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1999,15 +1999,17 @@ static struct sk_buff *receive_big(struct net_device *dev,
 				   struct virtnet_rq_stats *stats)
 {
 	struct page *page = buf;
+	unsigned long max_len;
 	struct sk_buff *skb;
 
 	/* Make sure that len does not exceed the size allocated in
 	 * add_recvbuf_big.
 	 */
-	if (unlikely(len > (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE)) {
+	max_len = vi->hdr_len + (PAGE_SIZE - sizeof(struct padded_vnet_hdr)) +
+		  vi->big_packets_num_skbfrags * PAGE_SIZE;
+	if (unlikely(len > max_len)) {
 		pr_debug("%s: rx error: len %u exceeds allocated size %lu\n",
-			 dev->name, len,
-			 (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE);
+			 dev->name, len, max_len);
 		goto err;
 	}
 
-- 
2.43.0

[PATCH net v2 2/2] virtio-net: harden page_to_skb() big-packet frag loop

[Xiang Mei]

This is a robustness hardening patch. The slow-path frag loop in
page_to_skb() walks the page chain via page->private until the
device-reported len is consumed, implicitly trusting that len fits the
chain. It does not stop when the chain is exhausted (page becomes NULL
at the tail), nor when nr_frags reaches the end of the static
skb_shinfo()->frags[MAX_SKB_FRAGS] array.

Both bounds are needed: the chain length is big_packets_num_skbfrags + 1
pages, which for an MTU-driven configuration can be well below
MAX_SKB_FRAGS, so neither guard implies the other.

Make the loop self-defending so it no longer relies on the caller having
validated len: stop once the chain is exhausted, and never index past
MAX_SKB_FRAGS. No functional change for well-formed input.

Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
v2: robustness patch

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index afe73eda1491..518c22fa1b68 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -906,8 +906,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	}
 
 	BUG_ON(offset >= PAGE_SIZE);
-	while (len) {
+	while (len && page) {
 		unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
+
+		if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
+			break;
 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
 				frag_size, truesize);
 		len -= frag_size;
-- 
2.43.0

Re: [PATCH net v2 1/2] virtio-net: fix len check in receive_big()

[Xuan Zhuo]

On Wed, 10 Jun 2026 16:29:35 -0700, Xiang Mei <xmei5@asu.edu> wrote:
> receive_big() bounds the device-announced length by
> (big_packets_num_skbfrags + 1) * PAGE_SIZE.  That is still too loose:
> add_recvbuf_big() sets sg[1] to start at offset
> sizeof(struct padded_vnet_hdr) into the first page, so the chain
> actually carries hdr_len + (PAGE_SIZE - sizeof(padded_vnet_hdr)) +
> big_packets_num_skbfrags * PAGE_SIZE bytes -- 20 bytes less than the
> check allows for the common hdr_len == 12 case.
>
> A malicious virtio backend can announce a len in that gap.  page_to_skb()
> then walks one frag past the page chain, storing a NULL page->private
> into skb_shinfo()->frags[MAX_SKB_FRAGS], which is both an out-of-bounds
> write past the static frag array and a NULL frag handed up the rx path.
>
> Bound len by the size add_recvbuf_big() actually advertised.
>
> Fixes: 0c716703965f ("virtio-net: fix received length check in big packets")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Signed-off-by: Xiang Mei <xmei5@asu.edu>

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

> ---
> v2: add 2/2 for robustness
>
>  drivers/net/virtio_net.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f4adcfee7a80..afe73eda1491 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1999,15 +1999,17 @@ static struct sk_buff *receive_big(struct net_device *dev,
>  				   struct virtnet_rq_stats *stats)
>  {
>  	struct page *page = buf;
> +	unsigned long max_len;
>  	struct sk_buff *skb;
>
>  	/* Make sure that len does not exceed the size allocated in
>  	 * add_recvbuf_big.
>  	 */
> -	if (unlikely(len > (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE)) {
> +	max_len = vi->hdr_len + (PAGE_SIZE - sizeof(struct padded_vnet_hdr)) +
> +		  vi->big_packets_num_skbfrags * PAGE_SIZE;
> +	if (unlikely(len > max_len)) {
>  		pr_debug("%s: rx error: len %u exceeds allocated size %lu\n",
> -			 dev->name, len,
> -			 (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE);
> +			 dev->name, len, max_len);
>  		goto err;
>  	}
>
> --
> 2.43.0
>

Re: [PATCH net v2 2/2] virtio-net: harden page_to_skb() big-packet frag loop

[Xuan Zhuo]

On Wed, 10 Jun 2026 16:29:36 -0700, Xiang Mei <xmei5@asu.edu> wrote:
> This is a robustness hardening patch. The slow-path frag loop in
> page_to_skb() walks the page chain via page->private until the
> device-reported len is consumed, implicitly trusting that len fits the
> chain. It does not stop when the chain is exhausted (page becomes NULL
> at the tail), nor when nr_frags reaches the end of the static
> skb_shinfo()->frags[MAX_SKB_FRAGS] array.
>
> Both bounds are needed: the chain length is big_packets_num_skbfrags + 1
> pages, which for an MTU-driven configuration can be well below
> MAX_SKB_FRAGS, so neither guard implies the other.
>
> Make the loop self-defending so it no longer relies on the caller having
> validated len: stop once the chain is exhausted, and never index past
> MAX_SKB_FRAGS. No functional change for well-formed input.

At this point, we are assuming that len represents the correct packet length. If
there is a bug in the validation, it can be fixed, just like in your previous
patch. Indeed, not checking nr_frags is also based on the overall design.
However, I do not recommend adding this kind of enhancement. If we follow
this logic, we would end up adding similar code in many other places, which
doesn't make much sense.

Thanks.

>
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
> v2: robustness patch
>
>  drivers/net/virtio_net.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index afe73eda1491..518c22fa1b68 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -906,8 +906,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  	}
>
>  	BUG_ON(offset >= PAGE_SIZE);
> -	while (len) {
> +	while (len && page) {
>  		unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
> +
> +		if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
> +			break;
>  		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
>  				frag_size, truesize);
>  		len -= frag_size;
> --
> 2.43.0
>

Re: [PATCH net v2 2/2] virtio-net: harden page_to_skb() big-packet frag loop

[Xiang Mei]

Thanks for the review. I agree with that as I replied at the end of
v1. If we obsolete 2/2 but keep 1/2, is it okay to just leave it as
is?

Xiang

On Wed, Jun 10, 2026 at 7:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 10 Jun 2026 16:29:36 -0700, Xiang Mei <xmei5@asu.edu> wrote:
> > This is a robustness hardening patch. The slow-path frag loop in
> > page_to_skb() walks the page chain via page->private until the
> > device-reported len is consumed, implicitly trusting that len fits the
> > chain. It does not stop when the chain is exhausted (page becomes NULL
> > at the tail), nor when nr_frags reaches the end of the static
> > skb_shinfo()->frags[MAX_SKB_FRAGS] array.
> >
> > Both bounds are needed: the chain length is big_packets_num_skbfrags + 1
> > pages, which for an MTU-driven configuration can be well below
> > MAX_SKB_FRAGS, so neither guard implies the other.
> >
> > Make the loop self-defending so it no longer relies on the caller having
> > validated len: stop once the chain is exhausted, and never index past
> > MAX_SKB_FRAGS. No functional change for well-formed input.
>
> At this point, we are assuming that len represents the correct packet length. If
> there is a bug in the validation, it can be fixed, just like in your previous
> patch. Indeed, not checking nr_frags is also based on the overall design.
> However, I do not recommend adding this kind of enhancement. If we follow
> this logic, we would end up adding similar code in many other places, which
> doesn't make much sense.
>
> Thanks.
>
> >
> > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > ---
> > v2: robustness patch
> >
> >  drivers/net/virtio_net.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index afe73eda1491..518c22fa1b68 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -906,8 +906,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >       }
> >
> >       BUG_ON(offset >= PAGE_SIZE);
> > -     while (len) {
> > +     while (len && page) {
> >               unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
> > +
> > +             if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
> > +                     break;
> >               skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
> >                               frag_size, truesize);
> >               len -= frag_size;
> > --
> > 2.43.0
> >

Re: [PATCH net v2 2/2] virtio-net: harden page_to_skb() big-packet frag loop

[Xuan Zhuo]

On Wed, 10 Jun 2026 19:24:03 -0700, Xiang Mei <xmei5@asu.edu> wrote:
> Thanks for the review. I agree with that as I replied at the end of
> v1. If we obsolete 2/2 but keep 1/2, is it okay to just leave it as
> is?

You should post a new version.

Thanks.

>
> Xiang
>
> On Wed, Jun 10, 2026 at 7:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Wed, 10 Jun 2026 16:29:36 -0700, Xiang Mei <xmei5@asu.edu> wrote:
> > > This is a robustness hardening patch. The slow-path frag loop in
> > > page_to_skb() walks the page chain via page->private until the
> > > device-reported len is consumed, implicitly trusting that len fits the
> > > chain. It does not stop when the chain is exhausted (page becomes NULL
> > > at the tail), nor when nr_frags reaches the end of the static
> > > skb_shinfo()->frags[MAX_SKB_FRAGS] array.
> > >
> > > Both bounds are needed: the chain length is big_packets_num_skbfrags + 1
> > > pages, which for an MTU-driven configuration can be well below
> > > MAX_SKB_FRAGS, so neither guard implies the other.
> > >
> > > Make the loop self-defending so it no longer relies on the caller having
> > > validated len: stop once the chain is exhausted, and never index past
> > > MAX_SKB_FRAGS. No functional change for well-formed input.
> >
> > At this point, we are assuming that len represents the correct packet length. If
> > there is a bug in the validation, it can be fixed, just like in your previous
> > patch. Indeed, not checking nr_frags is also based on the overall design.
> > However, I do not recommend adding this kind of enhancement. If we follow
> > this logic, we would end up adding similar code in many other places, which
> > doesn't make much sense.
> >
> > Thanks.
> >
> > >
> > > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > > ---
> > > v2: robustness patch
> > >
> > >  drivers/net/virtio_net.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index afe73eda1491..518c22fa1b68 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -906,8 +906,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > >       }
> > >
> > >       BUG_ON(offset >= PAGE_SIZE);
> > > -     while (len) {
> > > +     while (len && page) {
> > >               unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
> > > +
> > > +             if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
> > > +                     break;
> > >               skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
> > >                               frag_size, truesize);
> > >               len -= frag_size;
> > > --
> > > 2.43.0
> > >

Re: [PATCH net v2 2/2] virtio-net: harden page_to_skb() big-packet frag loop

[Xiang Mei]

On Wed, Jun 10, 2026 at 7:41 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 10 Jun 2026 19:24:03 -0700, Xiang Mei <xmei5@asu.edu> wrote:
> > Thanks for the review. I agree with that as I replied at the end of
> > v1. If we obsolete 2/2 but keep 1/2, is it okay to just leave it as
> > is?
>
> You should post a new version.
>
Thanks for the tips! V3 has been sent out.

Xiang
> Thanks.
>
> >
> > Xiang
> >
> > On Wed, Jun 10, 2026 at 7:19 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Wed, 10 Jun 2026 16:29:36 -0700, Xiang Mei <xmei5@asu.edu> wrote:
> > > > This is a robustness hardening patch. The slow-path frag loop in
> > > > page_to_skb() walks the page chain via page->private until the
> > > > device-reported len is consumed, implicitly trusting that len fits the
> > > > chain. It does not stop when the chain is exhausted (page becomes NULL
> > > > at the tail), nor when nr_frags reaches the end of the static
> > > > skb_shinfo()->frags[MAX_SKB_FRAGS] array.
> > > >
> > > > Both bounds are needed: the chain length is big_packets_num_skbfrags + 1
> > > > pages, which for an MTU-driven configuration can be well below
> > > > MAX_SKB_FRAGS, so neither guard implies the other.
> > > >
> > > > Make the loop self-defending so it no longer relies on the caller having
> > > > validated len: stop once the chain is exhausted, and never index past
> > > > MAX_SKB_FRAGS. No functional change for well-formed input.
> > >
> > > At this point, we are assuming that len represents the correct packet length. If
> > > there is a bug in the validation, it can be fixed, just like in your previous
> > > patch. Indeed, not checking nr_frags is also based on the overall design.
> > > However, I do not recommend adding this kind of enhancement. If we follow
> > > this logic, we would end up adding similar code in many other places, which
> > > doesn't make much sense.
> > >
> > > Thanks.
> > >
> > > >
> > > > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > > > ---
> > > > v2: robustness patch
> > > >
> > > >  drivers/net/virtio_net.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index afe73eda1491..518c22fa1b68 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -906,8 +906,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > >       }
> > > >
> > > >       BUG_ON(offset >= PAGE_SIZE);
> > > > -     while (len) {
> > > > +     while (len && page) {
> > > >               unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
> > > > +
> > > > +             if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
> > > > +                     break;
> > > >               skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
> > > >                               frag_size, truesize);
> > > >               len -= frag_size;
> > > > --
> > > > 2.43.0
> > > >

Re: [PATCH net v2 2/2] virtio-net: harden page_to_skb() big-packet frag loop

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 10:18:49AM +0800, Xuan Zhuo wrote:
> On Wed, 10 Jun 2026 16:29:36 -0700, Xiang Mei <xmei5@asu.edu> wrote:
> > This is a robustness hardening patch. The slow-path frag loop in
> > page_to_skb() walks the page chain via page->private until the
> > device-reported len is consumed, implicitly trusting that len fits the
> > chain. It does not stop when the chain is exhausted (page becomes NULL
> > at the tail), nor when nr_frags reaches the end of the static
> > skb_shinfo()->frags[MAX_SKB_FRAGS] array.
> >
> > Both bounds are needed: the chain length is big_packets_num_skbfrags + 1
> > pages, which for an MTU-driven configuration can be well below
> > MAX_SKB_FRAGS, so neither guard implies the other.

i don't get it, and then what?

> >
> > Make the loop self-defending so it no longer relies on the caller having
> > validated len: stop once the chain is exhausted, and never index past
> > MAX_SKB_FRAGS. No functional change for well-formed input.
> 
> At this point, we are assuming that len represents the correct packet length.
> If
> there is a bug in the validation, it can be fixed, just like in your previous
> patch. Indeed, not checking nr_frags is also based on the overall design.
> However, I do not recommend adding this kind of enhancement. If we follow
> this logic, we would end up adding similar code in many other places, which
> doesn't make much sense.
> 
> Thanks.


I will be frank, I'm never sure where the confidential computing guys
draw the line.

Are speculative things of concern, for example?


> >
> > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > ---
> > v2: robustness patch
> >
> >  drivers/net/virtio_net.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index afe73eda1491..518c22fa1b68 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -906,8 +906,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >  	}
> >
> >  	BUG_ON(offset >= PAGE_SIZE);
> > -	while (len) {
> > +	while (len && page) {


don't see why we would check page

> >  		unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
> > +
> > +		if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
> > +			break;

so do we want BUG_ON here maybe?

> >  		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
> >  				frag_size, truesize);
> >  		len -= frag_size;
> > --
> > 2.43.0
> >