On 6/16/26 11:28, Xiang Mei wrote:
receive_big() bounds the device-announced length by
(big_packets_num_skbfrags + 1) * PAGE_SIZE. That is still too loose:
add_recvbuf_big() sets sg[1] to start at offset
sizeof(struct padded_vnet_hdr) into the first page, so the chain
actually carries hdr_len + (PAGE_SIZE - sizeof(padded_vnet_hdr)) +
big_packets_num_skbfrags * PAGE_SIZE bytes -- 20 bytes less than the
check allows for the common hdr_len == 12 case.
A malicious virtio backend can announce a len in that gap. page_to_skb()
then walks one frag past the page chain, storing a NULL page->private
into skb_shinfo()->frags[MAX_SKB_FRAGS], which is both an out-of-bounds
write past the static frag array and a NULL frag handed up the rx path.
Bound len by the size add_recvbuf_big() actually advertised.
Fixes: 0c716703965f ("virtio-net: fix received length check in big packets")
I think we should Cc: [email protected]
Reported-by: Weiming Shi <[email protected]>
Signed-off-by: Xiang Mei <[email protected]>
Reviewed-by: Xuan Zhuo <[email protected]>
---
v4: use easy to understand math to compute the max_len
v3: revoke 2/2 and add Xuan Zhuo's Reviewed-by tag
v2: add additiona check as 2/2
drivers/net/virtio_net.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f4adcfee7a80..8f4562316aaa 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1999,15 +1999,16 @@ static struct sk_buff *receive_big(struct net_device
*dev,
struct virtnet_rq_stats *stats)
{
struct page *page = buf;
+ unsigned long max_len = (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE -
+ sizeof(struct padded_vnet_hdr) + vi->hdr_len;
struct sk_buff *skb;
/* Make sure that len does not exceed the size allocated in
* add_recvbuf_big.
*/
- if (unlikely(len > (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE)) {
+ if (unlikely(len > max_len)) {
pr_debug("%s: rx error: len %u exceeds allocated size %lu\n",
- dev->name, len,
- (vi->big_packets_num_skbfrags + 1) * PAGE_SIZE);
+ dev->name, len, max_len);
goto err;
}
Reviewed-by: Bui Quang Minh <[email protected]>
Thanks,
Quang Minh.
