On Thu, Jun 26, 2025 at 11:34 PM Bui Quang Minh
[off-list ref] wrote:

On 6/26/25 09:34, Jason Wang wrote:

quoted

On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
[off-list ref] wrote:

quoted

In xdp_linearize_page, when reading the following buffers from the ring,
we forget to check the received length with the true allocate size. This
can lead to an out-of-bound read. This commit adds that missing check.

Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")

I think we should cc stable.

Okay, I'll do that in next version.

quoted

quoted

Signed-off-by: Bui Quang Minh <redacted>
---
  drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
  1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e53ba600605a..2a130a3e50ac 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -1797,7 +1797,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
   * across multiple buffers (num_buf > 1), and we make sure buffers
   * have enough headroom.
   */
-static struct page *xdp_linearize_page(struct receive_queue *rq,
+static struct page *xdp_linearize_page(struct net_device *dev,
+                                      struct receive_queue *rq,
                                        int *num_buf,
                                        struct page *p,
                                        int offset,

@@ -1818,17 +1819,33 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
         page_off += *len;

         while (--*num_buf) {
-               unsigned int buflen;
+               unsigned int headroom, tailroom, room;
+               unsigned int truesize, buflen;
                 void *buf;
+               void *ctx;
                 int off;

-               buf = virtnet_rq_get_buf(rq, &buflen, NULL);
+               buf = virtnet_rq_get_buf(rq, &buflen, &ctx);
                 if (unlikely(!buf))
                         goto err_buf;

                 p = virt_to_head_page(buf);
                 off = buf - page_address(p);

+               truesize = mergeable_ctx_to_truesize(ctx);

This won't work for receive_small_xdp().

If it is small mode, the num_buf == 1 and we don't get into the while loop.

You are right, it might be worth mentioning this somewhere.

quoted

quoted

+               headroom = mergeable_ctx_to_headroom(ctx);
+               tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
+               room = SKB_DATA_ALIGN(headroom + tailroom);
+
+               if (unlikely(buflen > truesize - room)) {
+                       put_page(p);
+                       pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
+                                dev->name, buflen,
+                                (unsigned long)(truesize - room));
+                       DEV_STATS_INC(dev, rx_length_errors);
+                       goto err_buf;
+               }

I wonder if this issue only affect XDP should we check other places?

In small mode, we check the len with GOOD_PACKET_LEN in receive_small.
In mergeable mode, we have some checks over the place and this is the
only one I see we miss. In xsk, we check inside buf_to_xdp. However, in
the big mode, I feel like there is a bug.

In add_recvbuf_big, 1 first page + vi->big_packets_num_skbfrags pages.
The pages are managed by a linked list. The vi->big_packets_num_skbfrags
is set in virtnet_set_big_packets

     vi->big_packets_num_skbfrags = guest_gso ? MAX_SKB_FRAGS :
DIV_ROUND_UP(mtu, PAGE_SIZE);

So the vi->big_packets_num_skbfrags can be fewer than MAX_SKB_FRAGS.

In receive_big, we call to page_to_skb, there is a check

     if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
         /* error case */
     }

But because the number of allocated buffer is
vi->big_packets_num_skbfrags + 1 and vi->big_packets_num_skbfrags can be
fewer than MAX_SKB_FRAGS, the check seems not enough

     while (len) {
         unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
         skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
                 frag_size, truesize);
         len -= frag_size;
         page = (struct page *)page->private;
         offset = 0;
     }

In the following while loop, we keep running based on len without NULL
check the pages linked list, so it may result into NULL pointer dereference.

What do you think?

This looks like a bug, let's fix it.

Thanks

Thanks,
Quang Minh.

quoted

quoted

+
                 /* guard against a misconfigured or uncooperative backend that
                  * is sending packet larger than the MTU.
                  */

@@ -1917,7 +1934,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
                 headroom = vi->hdr_len + header_offset;
                 buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
-               xdp_page = xdp_linearize_page(rq, &num_buf, page,
+               xdp_page = xdp_linearize_page(dev, rq, &num_buf, page,
                                               offset, header_offset,
                                               &tlen);
                 if (!xdp_page)

@@ -2252,7 +2269,7 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
          */
         if (!xdp_prog->aux->xdp_has_frags) {
                 /* linearize data for XDP */
-               xdp_page = xdp_linearize_page(rq, num_buf,
+               xdp_page = xdp_linearize_page(vi->dev, rq, num_buf,
                                               *page, offset,
                                               XDP_PACKET_HEADROOM,
                                               len);
--

2.43.0