Re: [PATCH net 1/4] virtio-net: ensure the received length does not exceed allocated size

[Bui Quang Minh <minhquangbui99@gmail.com>]

On 6/26/25 09:34, Jason Wang wrote:
> On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
> <minhquangbui99@gmail.com> wrote:
>> 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.

>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   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.

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

Thanks,
Quang Minh.

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