[PATCH net] virtio_net: check RX buffer length before DMA sync

[PATCH net] virtio_net: check RX buffer length before DMA sync

[Vladimir Petrzhikovskii]

Since commit 24fbd3967f3f ("virtio_net: add page_pool support for
buffer allocation"), an RX queue that uses the DMA API
(rq->use_page_pool_dma, for example with VIRTIO_F_ACCESS_PLATFORM)
syncs each received buffer with page_pool_dma_sync_for_cpu() using the
length the device wrote to the used ring, and checks that length
against the posted buffer only after the sync.

With SWIOTLB bouncing (swiotlb=force, or a confidential-computing
guest), the sync copies from the shared bounce buffer into the guest's
private page, so an oversized length also copies the bytes the device
wrote past the posted buffer over the neighbouring page pool fragments.
The later check drops the packet, but the copy has already happened. A
malicious device, such as a host the confidential guest does not
trust, can overwrite guest kernel memory next to an RX buffer with
bytes it chooses, which is enough to take an unprivileged guest
process to root.

Check the length before the sync in the per-fragment loops.
receive_buf() has to sync the header before the length check, because
the mergeable error path reads num_buffers from it, so limit that sync
to the posted length. A device that reports correct lengths sees no
change.

Found by an LLM code review agent auditing users of the DMA API; the
fix and this changelog were also written with an LLM coding assistant
and reviewed by us. Reproduced in QEMU with swiotlb=force and
iommu_platform=on, a test-only device change that reports a used length
64 bytes past the posted buffer and writes a marker there, and a guest
kprobe module that reads the buffer after the sync: without this patch
the marker reaches the guest page for small and mergeable buffers; with
it the packets are dropped and counted in rx_length_errors. The
virtio_net selftest basic_features.sh passes with and without mergeable
buffers.

Fixes: 24fbd3967f3f ("virtio_net: add page_pool support for buffer allocation")
Cc: stable@vger.kernel.org
Co-developed-by: Nikolai Kalinin <nikolai@kalinidze.me>
Signed-off-by: Nikolai Kalinin <nikolai@kalinidze.me>
Signed-off-by: Vladimir Petrzhikovskii <vladimir@dfb.sh>
---
The QEMU change and the guest probe are available on request.

We also have a working exploit for an unmodified v7.3-rc7 guest with
KASLR and IBT enabled: a malicious backend takes an unprivileged
process in the guest to root in the guest's initial user namespace.
We can share it privately with the maintainers on request.

An alternative is one sync helper bounded by the posted length for all
four call sites; we kept the per-site change to keep the net and
stable backport small.
---
 drivers/net/virtio_net.c | 63 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dd2c41ef6fbe..5aee4351dc4b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -792,19 +792,32 @@ static unsigned int mergeable_ctx_to_truesize(void *mrg_ctx)
 	return (unsigned long)mrg_ctx & ((1 << MRG_CTX_HEADER_SHIFT) - 1);
 }
 
-static int check_mergeable_len(struct net_device *dev, void *mrg_ctx,
-			       unsigned int len)
+/* Length of the mergeable buffer that add_recvbuf_mergeable() posted. */
+static unsigned int mergeable_ctx_to_buflen(void *mrg_ctx)
 {
-	unsigned int headroom, tailroom, room, truesize;
+	unsigned int headroom, tailroom, room;
 
-	truesize = mergeable_ctx_to_truesize(mrg_ctx);
 	headroom = mergeable_ctx_to_headroom(mrg_ctx);
 	tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
 	room = SKB_DATA_ALIGN(headroom + tailroom);
 
-	if (len > truesize - room) {
-		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
-			 dev->name, len, (unsigned long)(truesize - room));
+	return mergeable_ctx_to_truesize(mrg_ctx) - room;
+}
+
+/* Length of the small buffer that add_recvbuf_small() posts. */
+static unsigned int small_buflen(const struct virtnet_info *vi)
+{
+	return vi->hdr_len + GOOD_PACKET_LEN;
+}
+
+static int check_mergeable_len(struct net_device *dev, void *mrg_ctx,
+			       unsigned int len)
+{
+	unsigned int buflen = mergeable_ctx_to_buflen(mrg_ctx);
+
+	if (len > buflen) {
+		pr_debug("%s: rx error: len %u exceeds buffer length %u\n",
+			 dev->name, len, buflen);
 		DEV_STATS_INC(dev, rx_length_errors);
 		return -1;
 	}
@@ -1797,15 +1810,15 @@ static struct page *xdp_linearize_page(struct net_device *dev,
 		p = virt_to_head_page(buf);
 		off = buf - page_address(p);
 
-		if (rq->use_page_pool_dma)
-			page_pool_dma_sync_for_cpu(rq->page_pool, p,
-						   off, buflen);
-
 		if (check_mergeable_len(dev, ctx, buflen)) {
 			page_pool_put_page(rq->page_pool, p, -1, true);
 			goto err_buf;
 		}
 
+		if (rq->use_page_pool_dma)
+			page_pool_dma_sync_for_cpu(rq->page_pool, p,
+						   off, buflen);
+
 		/* guard against a misconfigured or uncooperative backend that
 		 * is sending packet larger than the MTU.
 		 */
@@ -2165,15 +2178,15 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
 		page = virt_to_head_page(buf);
 		offset = buf - page_address(page);
 
-		if (rq->use_page_pool_dma)
-			page_pool_dma_sync_for_cpu(rq->page_pool, page,
-						   offset, len);
-
 		if (check_mergeable_len(dev, ctx, len)) {
 			page_pool_put_page(rq->page_pool, page, -1, true);
 			goto err;
 		}
 
+		if (rq->use_page_pool_dma)
+			page_pool_dma_sync_for_cpu(rq->page_pool, page,
+						   offset, len);
+
 		truesize = mergeable_ctx_to_truesize(ctx);
 		xdp_frags_truesz += truesize;
 
@@ -2446,15 +2459,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 		u64_stats_add(&stats->bytes, len);
 		page = virt_to_head_page(buf);
 
+		if (check_mergeable_len(dev, ctx, len))
+			goto err_skb;
+
 		if (rq->use_page_pool_dma) {
 			offset = buf - page_address(page);
 			page_pool_dma_sync_for_cpu(rq->page_pool, page,
 						   offset, len);
 		}
 
-		if (check_mergeable_len(dev, ctx, len))
-			goto err_skb;
-
 		truesize = mergeable_ctx_to_truesize(ctx);
 		curr_skb  = virtnet_skb_append_frag(rq, head_skb, curr_skb, page,
 						    buf, len, truesize);
@@ -2574,12 +2587,22 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 
 	/* Sync the memory before touching anything through buf,
 	 * unless virtio core did it already.
+	 *
+	 * len comes from the device and is checked against the buffer
+	 * only later, so never sync past the buffer posted to the device.
+	 * The sync still covers the header, which receive_buf() and the
+	 * mergeable drop path read. Big buffers never come from the page
+	 * pool, so a buffer here is either mergeable or small.
 	 */
 	if (rq->use_page_pool_dma) {
 		struct page *page = virt_to_head_page(buf);
 		int offset = buf - page_address(page);
+		unsigned int buflen = vi->mergeable_rx_bufs ?
+				      mergeable_ctx_to_buflen(ctx) :
+				      small_buflen(vi);
 
-		page_pool_dma_sync_for_cpu(rq->page_pool, page, offset, len);
+		page_pool_dma_sync_for_cpu(rq->page_pool, page, offset,
+					   min(len, buflen));
 	}
 
 	/* About the flags below:
@@ -2655,7 +2678,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
 	buf += VIRTNET_RX_PAD + xdp_headroom;
 
 	ctx = mergeable_len_to_ctx(alloc_len, xdp_headroom);
-	err = virtnet_rq_submit(rq, buf, vi->hdr_len + GOOD_PACKET_LEN, ctx, gfp);
+	err = virtnet_rq_submit(rq, buf, small_buflen(vi), ctx, gfp);
 
 	if (err < 0)
 		page_pool_put_page(rq->page_pool, virt_to_head_page(buf), -1, false);

---
base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0
change-id: 20261108-b4-virtionet-rxlen-afbf3d99e38b

Best regards,
--  
Vladimir Petrzhikovskii <vladimir@dfb.sh>

Re: [PATCH net] virtio_net: check RX buffer length before DMA sync

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The Fixes: tag and the changelog blame commit 24fbd3967f3f
  ("virtio_net: add page_pool support for buffer allocation") for…

--

virtio_net: check RX buffer length before DMA sync

This patch moves check_mergeable_len() ahead of
page_pool_dma_sync_for_cpu() in the mergeable fragment loops. It also
limits the head buffer sync in receive_buf() to the length of the buffer
that was posted. The goal is to stop a malicious device under SWIOTLB from
using an oversized used length to bounce its own bytes over neighbouring
page pool fragments.

> Since commit 24fbd3967f3f ("virtio_net: add page_pool support for
> buffer allocation"), an RX queue that uses the DMA API
> (rq->use_page_pool_dma, for example with VIRTIO_F_ACCESS_PLATFORM)
> syncs each received buffer with page_pool_dma_sync_for_cpu() using the
> length the device wrote to the used ring, and checks that length
> against the posted buffer only after the sync.

[ ... ]

> Fixes: 24fbd3967f3f ("virtio_net: add page_pool support for buffer allocation")
> Cc: stable@vger.kernel.org

[Severity: Medium]
Is 24fbd3967f3f the right commit for the Fixes: tag? The sync before the
length check seems to predate page_pool. That commit appears to have only
moved it into the per-site page_pool_dma_sync_for_cpu() calls.

In the parent of 24fbd3967f3f, every merge or small mode RX buffer goes
through virtnet_rq_get_buf(). It syncs using the raw used length:

drivers/net/virtio_net.c:virtnet_rq_get_buf() {
    ...
	buf = virtqueue_get_buf_ctx(rq->vq, len, ctx);
	if (buf)
		virtnet_rq_unmap(rq, buf, *len);
    ...
}

drivers/net/virtio_net.c:virtnet_rq_unmap() {
    ...
	if (dma->need_sync && len) {
		offset = buf - (head + sizeof(*dma));

		virtqueue_map_sync_single_range_for_cpu(rq->vq, dma->addr,
							offset, len,
							DMA_FROM_DEVICE);
	}
    ...
}

This covers the head buffer through virtnet_receive_packets() ->
receive_buf(). It also covers the fragments in receive_mergeable(),
virtnet_build_xdp_buff_mrg(), xdp_linearize_page() and
mergeable_buf_free().

In every case the sync runs before check_mergeable_len() or the
GOOD_PACKET_LEN check in receive_small().

In virtnet_rq_alloc() the mapping covers the whole alloc_frag:

	dma->len = alloc_frag->size - sizeof(*dma);

So under SWIOTLB, wouldn't an oversized used length copy bytes chosen by
the device over the neighbouring RX buffers in the same frag? That looks
like the same primitive the changelog describes.

git blame points at 295525e29a5b ("virtio_net: merge dma operations when
filling mergeable buffers"). The path then became unconditional for merge
and small mode in fb22437c1ba3 ("virtio_net: rx remove premapped failover
code").

Kernels with fb22437c1ba3 but not 24fbd3967f3f seem to be just as
exposed. With only the 24fbd3967f3f tag, stable tooling would not pick
those trees up. This patch also cannot apply there, because the page_pool
code does not exist in them.

That also affects the note after the changelog:

> An alternative is one sync helper bounded by the posted length for all
> four call sites; we kept the per-site change to keep the net and
> stable backport small.

Should the Fixes: tag point at 295525e29a5b and/or fb22437c1ba3? It would
also help to say in the changelog that those trees need a separate fix,
for example limiting the sync in virtnet_rq_unmap() to the posted length.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-b4-virtionet-rxlen-v1-1-0078c5ab7e14%40dfb.sh