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

[netdev-bot+sashiko@kernel.org]

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