On Thu, Jun 11, 2026 at 2:48 AM David Laight
<[email protected]> wrote:
>
> On Wed, 10 Jun 2026 15:16:06 -0700
> Xiang Mei <[email protected]> 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 <[email protected]>
> > Signed-off-by: Xiang Mei <[email protected]>
> > ---
> > 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;
>
> That looks like a constant (for the vi).
Thanks, agreed it's a per-device constant, so the expression never
changes after setup.
> Probably worth saving rather than recalculating all the time.
>
I thought about caching it in a vi->big_packets_max_len field computed
once after virtnet_set_big_packets(). My only concern is that the
saving is marginal here: receive_big() is only reached when
mergeable_rx_bufs is off (the "else if (vi->big_packets)" branch). Any
backend that negotiates VIRTIO_NET_F_MRG_RXBUF takes the
receive_mergeable() path for all RX, so receive_big() is never hit
there at all. And the bound has exactly one consumer.
So it's trading a 4-byte field (probably free in existing padding)
against one cache-hit load plus a shift, and two adds, on a path
that's effectively cold on modern setups. I mainly work on security
issues, so I would like to listen to your idea.
Xiang
> -- David
>
> > + 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;
> > }
> >
>
