On Thu, Jun 26, 2025 at 11:34 PM Bui Quang Minh
<minhquangbu...@gmail.com> wrote:
>
> On 6/26/25 09:34, Jason Wang wrote:
> > On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
> > <minhquangbu...@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 <minhquangbu...@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.
You are right, it might be worth mentioning this somewhere.
>
> >
> >> + 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.
>
> >
> >> +
> >> /* 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
> >>
>
