[PATCH net v3] virtio-net: fix len check in receive_big()

[PATCH net v3] 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>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
v3: revoke 2/2 and add Xuan Zhuo's Reviewed-by tag

 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 v3] virtio-net: fix len check in receive_big()

[Michael S. Tsirkin]

On Wed, Jun 10, 2026 at 07:46:16PM -0700, Xiang Mei 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>

Thanks for the patch! Something small to improve:

> ---
> v3: revoke 2/2 and add Xuan Zhuo's Reviewed-by tag
> 
>  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;

Assignment can happen here?

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

Took me a while to figure out what is going on, but I finally
understand:


Reducing
(vi->big_packets_num_skbfrags + 1) * PAGE_SIZE 

(what we allocated)

by sizeof(struct padded_vnet_hdr) - vi->hdr_len


right?

So clearer as:


	unsigned long max_len = (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE -
	sizeof(struct padded_vnet_hdr) + vi->hdr_len;




> +	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 v3] virtio-net: fix len check in receive_big()

[Xiang Mei]

On Wed, Jun 10, 2026 at 10:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jun 10, 2026 at 07:46:16PM -0700, Xiang Mei 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>
>
> Thanks for the patch! Something small to improve:
>
> > ---
> > v3: revoke 2/2 and add Xuan Zhuo's Reviewed-by tag
> >
> >  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;
>
> Assignment can happen here?
>
> >       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;
>
> Took me a while to figure out what is going on, but I finally
> understand:
>
>
> Reducing
> (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE
>
> (what we allocated)
>
> by sizeof(struct padded_vnet_hdr) - vi->hdr_len
>
>
> right?
>
> So clearer as:
>
>
>         unsigned long max_len = (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE -
>         sizeof(struct padded_vnet_hdr) + vi->hdr_len;
>
Right, that's the same value. Yours reads better!

I'll fold this into the next respin. One thing I'd like to settle
first: David suggested storing this in a vi field computed once at the
probe (it's a per-device constant) and just comparing len against it
on the datapath, instead of re-deriving it in receive_big() each time.
I'll wait for his take on that and send a single v4 that covers both.

Xiang

>
>
>
> > +     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 v3] virtio-net: fix len check in receive_big()

[Michael S. Tsirkin]

On Sat, Jun 13, 2026 at 01:15:02PM -0700, Xiang Mei wrote:
> On Wed, Jun 10, 2026 at 10:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jun 10, 2026 at 07:46:16PM -0700, Xiang Mei 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>
> >
> > Thanks for the patch! Something small to improve:
> >
> > > ---
> > > v3: revoke 2/2 and add Xuan Zhuo's Reviewed-by tag
> > >
> > >  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;
> >
> > Assignment can happen here?
> >
> > >       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;
> >
> > Took me a while to figure out what is going on, but I finally
> > understand:
> >
> >
> > Reducing
> > (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE
> >
> > (what we allocated)
> >
> > by sizeof(struct padded_vnet_hdr) - vi->hdr_len
> >
> >
> > right?
> >
> > So clearer as:
> >
> >
> >         unsigned long max_len = (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE -
> >         sizeof(struct padded_vnet_hdr) + vi->hdr_len;
> >
> Right, that's the same value. Yours reads better!
> 
> I'll fold this into the next respin. One thing I'd like to settle
> first: David suggested storing this in a vi field computed once at the
> probe (it's a per-device constant) and just comparing len against it
> on the datapath, instead of re-deriving it in receive_big() each time.
> I'll wait for his take on that and send a single v4 that covers both.
> 
> Xiang

I don't mind.

> >
> >
> >
> > > +     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
> >