[PATCH v2] vhost/net: fix clear_user start address in VHOST_GET_FEATURES_ARRAY

[PATCH v2] vhost/net: fix clear_user start address in VHOST_GET_FEATURES_ARRAY

[rom.wang]

From: Yufeng Wang <wangyufeng@kylinos.cn>

The clear_user() call in VHOST_GET_FEATURES_ARRAY incorrectly starts
at argp, which is the beginning of the features array, overwriting the
data just written by copy_to_user(). It should start after the copied
elements at argp + copied * sizeof(u64) to only zero the trailing
unused space.

Use size_mul() for both the offset and length calculations so the
arithmetic stays consistent with the surrounding code and remains
overflow-safe.

Fixes: 333c515d1896 ("vhost-net: allow configuring extended features")
Signed-off-by: Yufeng Wang <wangyufeng@kylinos.cn>

---
Changes in v2:
- Use size_mul() for the offset calculation as well, per review feedback.

Link to v1: https://lore.kernel.org/all/20260526080336.61296-1-r4o5m6e8o@163.com/

Note:
Thank you for your review and suggestions.

I tried to add a switch in tools/virtio/vhost_net_test.c.
The switch is meant to use VHOST_GET_FEATURES_ARRAY and
VHOST_SET_FEATURES_ARRAY instead of the legacy versions.

However, when I ran `make virtio` in the tools directory,
the build failed with an error: missing asm/percpu_types.h.
I fixed that error, but then another error appeared.

Would it be acceptable to postpone the submission of
this test case until I have sorted out all the build
errors?
---
 drivers/vhost/net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 77b59f49bddb..4b963dafa233 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1784,7 +1784,8 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
 			return -EFAULT;
 
 		/* Zero the trailing space provided by user-space, if any */
-		if (clear_user(argp, size_mul(count - copied, sizeof(u64))))
+		if (clear_user(argp + size_mul(copied, sizeof(u64)),
+			       size_mul(count - copied, sizeof(u64))))
 			return -EFAULT;
 		return 0;
 	case VHOST_SET_FEATURES_ARRAY:
-- 
2.34.1

Re: [PATCH v2] vhost/net: fix clear_user start address in VHOST_GET_FEATURES_ARRAY

[Eugenio Perez Martin]

On Fri, Jun 26, 2026 at 9:05 AM rom.wang <r4o5m6e8o@163.com> wrote:
>
> From: Yufeng Wang <wangyufeng@kylinos.cn>
>
> The clear_user() call in VHOST_GET_FEATURES_ARRAY incorrectly starts
> at argp, which is the beginning of the features array, overwriting the
> data just written by copy_to_user(). It should start after the copied
> elements at argp + copied * sizeof(u64) to only zero the trailing
> unused space.
>
> Use size_mul() for both the offset and length calculations so the
> arithmetic stays consistent with the surrounding code and remains
> overflow-safe.
>
> Fixes: 333c515d1896 ("vhost-net: allow configuring extended features")
> Signed-off-by: Yufeng Wang <wangyufeng@kylinos.cn>
>
> ---
> Changes in v2:
> - Use size_mul() for the offset calculation as well, per review feedback.
>
> Link to v1: https://lore.kernel.org/all/20260526080336.61296-1-r4o5m6e8o@163.com/
>
> Note:
> Thank you for your review and suggestions.
>
> I tried to add a switch in tools/virtio/vhost_net_test.c.
> The switch is meant to use VHOST_GET_FEATURES_ARRAY and
> VHOST_SET_FEATURES_ARRAY instead of the legacy versions.
>
> However, when I ran `make virtio` in the tools directory,
> the build failed with an error: missing asm/percpu_types.h.
> I fixed that error, but then another error appeared.
>
> Would it be acceptable to postpone the submission of
> this test case until I have sorted out all the build
> errors?
> ---
>  drivers/vhost/net.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 77b59f49bddb..4b963dafa233 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1784,7 +1784,8 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
>                         return -EFAULT;
>
>                 /* Zero the trailing space provided by user-space, if any */
> -               if (clear_user(argp, size_mul(count - copied, sizeof(u64))))
> +               if (clear_user(argp + size_mul(copied, sizeof(u64)),
> +                              size_mul(count - copied, sizeof(u64))))

Acked-by: Eugenio Pérez <eperezma@redhat.com>

Thanks!

>                         return -EFAULT;
>                 return 0;
>         case VHOST_SET_FEATURES_ARRAY:
> --
> 2.34.1
>

[PATCH 0/3] tools/virtio: exercise VHOST_*_FEATURES_ARRAY ioctls

[rom.wang]

From: Yufeng Wang <wangyufeng@kylinos.cn>

Hi Eugenio,

This series follows up on your review of the vhost/net GET_FEATURES_ARRAY
fix [1], where you asked for a switch in tools/virtio/vhost_net_test.c to
exercise VHOST_GET_FEATURES_ARRAY and VHOST_SET_FEATURES_ARRAY.  The fix
itself is unchanged from the v2 you Acked [2] and is not included here.

Patch 3 adds the requested "--features-array" switch.  When it is given,
the test programs features via VHOST_SET_FEATURES_ARRAY and then verifies
VHOST_GET_FEATURES_ARRAY by issuing it twice with the same count,
comfortably above VIRTIO_FEATURES_U64S: once into a zero-initialized
buffer and once into a buffer pre-filled with a non-zero sentinel.  The
two reads must match on every entry: the kernel fills the prefix with the
device features and clear_user()s the trailing entries it did not fill.
A clear_user() with the wrong start address clobbers the prefix instead
of the tail; the sentinel buffer then keeps non-zero data in the trailing
entries and disagrees with the clean read.  Using a count comfortably
above VIRTIO_FEATURES_U64S (rather than matching it exactly) keeps the
test correct as the kernel's feature-word count grows, so this is exactly
the path that [1] fixes and the test doubles as a regression test for it.

Patches 1-2 are build prerequisites needed for the harness to compile
against current headers: a device-id compat header (since
mod_devicetable.h was split into include/linux/device-id/) and an
__must_be_array stub (needed by struct_size(), which the new test uses).
With them, `make -C tools/virtio test` builds virtio_test, vringh_test
and vhost_net_test cleanly.

Patch 3 depends on [1] for its GET path to pass.  Tested on x86_64:
  - without [1]: ./vhost_net_test --features-array aborts in
    verify_get_features_array() (the GET clear_user bug);
  - with [1]: both ./vhost_net_test and ./vhost_net_test --features-array
    complete RX/TX with started == completed == 0x100000.

[1] https://lore.kernel.org/all/20260526080336.61296-1-r4o5m6e8o@163.com/
[2] https://lore.kernel.org/all/CAJaqyWfP7j5_PvOP0JiHuAQ8appGnoJDS3OB-qK7riBpZ_d_tQ@mail.gmail.com/

Yufeng Wang (3):
  tools/virtio: add device-id compat header for virtio
  tools/virtio: add __must_be_array stub
  tools/virtio: add --features-array switch

 tools/virtio/linux/compiler.h         |  1 +
 tools/virtio/linux/device-id/virtio.h |  1 +
 tools/virtio/vhost_net_test.c         | 70 +++++++++++++++++++++++++--
 3 files changed, 68 insertions(+), 4 deletions(-)
 create mode 100644 tools/virtio/linux/device-id/virtio.h

-- 
2.34.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH 1/3] tools/virtio: add device-id compat header for virtio

[rom.wang]

From: Yufeng Wang <wangyufeng@kylinos.cn>

mod_devicetable.h was split into per-subsystem headers under
include/linux/device-id/ (commit ad428f5811bd "mod_devicetable.h: Split
into per subsystem headers"), and include/linux/virtio.h now includes
<linux/device-id/virtio.h>.

The tools/virtio compat layer has no counterpart for this new include, so
building vhost_net_test fails:

  ../../include/linux/virtio.h:10:10: fatal error: linux/device-id/virtio.h: No such file or directory
     10 | #include <linux/device-id/virtio.h>
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~

Add tools/virtio/linux/device-id/virtio.h, which includes the real
header, matching how the existing virtio.h compat shim reaches the
kernel's include/linux/virtio.h.

Signed-off-by: Yufeng Wang <wangyufeng@kylinos.cn>
---
 tools/virtio/linux/device-id/virtio.h | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 tools/virtio/linux/device-id/virtio.h

diff --git a/tools/virtio/linux/device-id/virtio.h b/tools/virtio/linux/device-id/virtio.h
new file mode 100644
index 000000000000..18e3338158eb
--- /dev/null
+++ b/tools/virtio/linux/device-id/virtio.h
@@ -0,0 +1 @@
+#include <../../include/linux/device-id/virtio.h>
-- 
2.34.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH 2/3] tools/virtio: add __must_be_array stub

[rom.wang]

From: Yufeng Wang <wangyufeng@kylinos.cn>

struct_size() expands to flex_array_size(), which uses __must_be_array()
to assert at build time that its argument is an array.  __must_be_array()
is defined in the kernel's include/linux/compiler.h, but the tools/virtio
compiler.h compat shim does not provide it, so any use of struct_size()
in the test harness fails to build:

  ../include/linux/overflow.h:161:44: error: implicit declaration of function '__must_be_array' [-Wimplicit-function-declaration]
    161 |                     sizeof(*(p)->member) + __must_be_array((p)->member),\
        |                                            ^~~~~~~~~~~~~~~
  vhost_net_test.c:258:32: note: in expansion of macro 'struct_size'

Stub __must_be_array() to 0, consistent with how BUILD_BUG_ON() and
__must_check are already stubbed out in this environment.

Signed-off-by: Yufeng Wang <wangyufeng@kylinos.cn>
---
 tools/virtio/linux/compiler.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h
index 0f25db473f55..3fa035296fab 100644
--- a/tools/virtio/linux/compiler.h
+++ b/tools/virtio/linux/compiler.h
@@ -40,5 +40,6 @@
 })
 
 #define __must_check
+#define __must_be_array(a)	0
 
 #endif
-- 
2.34.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH 3/3] tools/virtio: add --features-array switch

[rom.wang]

From: Yufeng Wang <wangyufeng@kylinos.cn>

Add a --features-array command-line switch that exercises the array-variant
feature ioctls, VHOST_SET_FEATURES_ARRAY and VHOST_GET_FEATURES_ARRAY,
instead of the legacy VHOST_SET_FEATURES.  The legacy path is used when the
switch is omitted, so both code paths stay covered.

SET: features are programmed via VHOST_SET_FEATURES_ARRAY.

GET: VHOST_GET_FEATURES_ARRAY is issued twice with a count above
VIRTIO_FEATURES_U64S, once into a zero-initialized buffer and once into a
sentinel-filled one; the two reads must match.  This verifies the kernel
fills the prefix and clear_user()s the trailing entries, and stays correct
as the kernel's feature-word count grows.

Signed-off-by: Yufeng Wang <wangyufeng@kylinos.cn>
---
 tools/virtio/vhost_net_test.c | 70 +++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 4 deletions(-)

diff --git a/tools/virtio/vhost_net_test.c b/tools/virtio/vhost_net_test.c
index 389d99a6d7c7..77191e60a7a3 100644
--- a/tools/virtio/vhost_net_test.c
+++ b/tools/virtio/vhost_net_test.c
@@ -26,6 +26,12 @@
 #define TEST_PTYPE	ETH_P_LOOPBACK
 #define DESC_NUM	256
 
+/* Number of u64 entries that the kernel uses to store all currently-defined
+ * virtio features. Must stay in sync with the kernel's VIRTIO_FEATURES_U64S. */
+#define FEATURES_U64S	2
+/* Above VIRTIO_FEATURES_U64S so trailing entries hit the clear_user() tail. */
+#define GET_FEATURES_ARRAY_COUNT	8
+
 /* Used by implementation of kmalloc() in tools/virtio/linux/kernel.h */
 void *__kmalloc_fake, *__kfree_ignore_start, *__kfree_ignore_end;
 
@@ -205,8 +211,10 @@ static void vq_info_add(struct vdev_info *dev, int idx, int num, int fd)
 	assert(!r);
 }
 
-static void vdev_info_init(struct vdev_info *dev, unsigned long long features)
+static void vdev_info_init(struct vdev_info *dev, unsigned long long features,
+			   bool features_array)
 {
+	struct vhost_features_array *fa;
 	struct ether_header *eh;
 	int i, r;
 
@@ -248,8 +256,20 @@ static void vdev_info_init(struct vdev_info *dev, unsigned long long features)
 	r = ioctl(dev->control, VHOST_SET_MEM_TABLE, dev->mem);
 	assert(r >= 0);
 
-	r = ioctl(dev->control, VHOST_SET_FEATURES, &features);
-	assert(r >= 0);
+	if (features_array) {
+		fa = calloc(1, struct_size(fa, features, FEATURES_U64S));
+		assert(fa);
+		fa->count = FEATURES_U64S;
+		fa->features[0] = features;
+
+		r = ioctl(dev->control, VHOST_SET_FEATURES_ARRAY, fa);
+		assert(r >= 0);
+
+		free(fa);
+	} else {
+		r = ioctl(dev->control, VHOST_SET_FEATURES, &features);
+		assert(r >= 0);
+	}
 
 	dev->nvqs = 2;
 }
@@ -455,6 +475,10 @@ static const struct option longopts[] = {
 		.val = 'b',
 		.has_arg = required_argument,
 	},
+	{
+		.name = "features-array",
+		.val = 'F',
+	},
 	{
 	}
 };
@@ -467,11 +491,43 @@ static void help(int status)
 		" [--no-virtio-1]"
 		" [--delayed-interrupt]"
 		" [--buf-num]"
+		" [--features-array]"
 		"\n");
 
 	exit(status);
 }
 
+static void verify_get_features_array(struct vdev_info *dev)
+{
+	struct vhost_features_array *clean, *dirty;
+	int r, i;
+
+	clean = calloc(1, struct_size(clean, features, GET_FEATURES_ARRAY_COUNT));
+	assert(clean);
+	clean->count = GET_FEATURES_ARRAY_COUNT;
+	r = ioctl(dev->control, VHOST_GET_FEATURES_ARRAY, clean);
+	assert(r >= 0);
+
+	/* Sentinel-fill so the kernel's clear_user() of the tail is visible. */
+	dirty = calloc(1, struct_size(dirty, features, GET_FEATURES_ARRAY_COUNT));
+	assert(dirty);
+	dirty->count = GET_FEATURES_ARRAY_COUNT;
+	memset(dirty->features, 0xa5, GET_FEATURES_ARRAY_COUNT * sizeof(dirty->features[0]));
+	r = ioctl(dev->control, VHOST_GET_FEATURES_ARRAY, dirty);
+	assert(r >= 0);
+
+	/* Must match; a wrong clear_user() start would leave them unequal. */
+	for (i = 0; i < GET_FEATURES_ARRAY_COUNT; i++)
+		assert(clean->features[i] == dirty->features[i]);
+
+	printf("GET_FEATURES_ARRAY: 0x%llx 0x%llx (tail zeroed) OK\n",
+	       (unsigned long long)clean->features[0],
+	       (unsigned long long)clean->features[1]);
+
+	free(clean);
+	free(dirty);
+}
+
 int main(int argc, char **argv)
 {
 	unsigned long long features = (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
@@ -480,6 +536,7 @@ int main(int argc, char **argv)
 	long nbufs = 0x100000;
 	struct vdev_info dev;
 	bool delayed = false;
+	bool features_array = false;
 	int o, fd;
 
 	for (;;) {
@@ -503,6 +560,9 @@ int main(int argc, char **argv)
 		case 'D':
 			delayed = true;
 			break;
+		case 'F':
+			features_array = true;
+			break;
 		case 'n':
 			nbufs = strtol(optarg, NULL, 10);
 			assert(nbufs > 0);
@@ -520,7 +580,9 @@ int main(int argc, char **argv)
 	fd = tun_alloc(&dev, tun_name);
 	assert(fd >= 0);
 
-	vdev_info_init(&dev, features);
+	vdev_info_init(&dev, features, features_array);
+	if (features_array)
+		verify_get_features_array(&dev);
 	vq_info_add(&dev, 0, DESC_NUM, fd);
 	vq_info_add(&dev, 1, DESC_NUM, fd);
 	vdev_create_socket(&dev, tun_name);
-- 
2.34.1

Re: [PATCH 3/3] tools/virtio: add --features-array switch

[sashiko-bot]

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

Pre-existing issues:
- [High] The kernel implementation of VHOST_GET_FEATURES_ARRAY zeroes out the returned features instead of the tail, causing incorrect ioctl behavior and causing the newly added test to fail.
--

--- Patch [3]: [PATCH 3/3] tools/virtio: add --features-array switch ---
commit 62c148ff31dfbcede0d6da6c8873dc84602618f3
Author: Yufeng Wang <wangyufeng@kylinos.cn>

tools/virtio: add --features-array switch

This patch adds a --features-array switch to vhost_net_test to exercise the
VHOST_SET_FEATURES_ARRAY and VHOST_GET_FEATURES_ARRAY ioctls. The test verifies
that the kernel correctly copies the features and clears the trailing space when
the requested count exceeds the kernel's defined feature entries.

> diff --git a/tools/virtio/vhost_net_test.c b/tools/virtio/vhost_net_test.c
> index 389d99a6d7c76..77191e60a7a35 100644
> --- a/tools/virtio/vhost_net_test.c
> +++ b/tools/virtio/vhost_net_test.c
[ ... ]
> @@ -467,11 +491,43 @@ static void help(int status)
[ ... ]
> +static void verify_get_features_array(struct vdev_info *dev)
> +{
> +	struct vhost_features_array *clean, *dirty;
> +	int r, i;
> +
> +	clean = calloc(1, struct_size(clean, features, GET_FEATURES_ARRAY_COUNT));
> +	assert(clean);
> +	clean->count = GET_FEATURES_ARRAY_COUNT;
> +	r = ioctl(dev->control, VHOST_GET_FEATURES_ARRAY, clean);
> +	assert(r >= 0);
> +
> +	/* Sentinel-fill so the kernel's clear_user() of the tail is visible. */
> +	dirty = calloc(1, struct_size(dirty, features, GET_FEATURES_ARRAY_COUNT));
> +	assert(dirty);
> +	dirty->count = GET_FEATURES_ARRAY_COUNT;
> +	memset(dirty->features, 0xa5, GET_FEATURES_ARRAY_COUNT * sizeof(dirty->features[0]));
> +	r = ioctl(dev->control, VHOST_GET_FEATURES_ARRAY, dirty);
> +	assert(r >= 0);
> +
> +	/* Must match; a wrong clear_user() start would leave them unequal. */
> +	for (i = 0; i < GET_FEATURES_ARRAY_COUNT; i++)
> +		assert(clean->features[i] == dirty->features[i]);

[Severity: High]
This is a pre-existing issue, but will this assert consistently fail against the
current kernel implementation of VHOST_GET_FEATURES_ARRAY?

Looking at vhost_net_ioctl() in drivers/vhost/net.c, when handling the array
features and zeroing the trailing space, it appears to start at argp:

        /* Zero the trailing space provided by user-space, if any */
        if (clear_user(argp, size_mul(count - copied, sizeof(u64))))
                return -EFAULT;

Because argp is not advanced by copied * sizeof(u64), clear_user() zeroes out
the newly copied features at the beginning of the array instead of the tail.

Since the kernel implementation leaves the actual trailing space uninitialized,
will this cause the test introduced here to always crash via the assertion?

> +
> +	printf("GET_FEATURES_ARRAY: 0x%llx 0x%llx (tail zeroed) OK\n",
> +	       (unsigned long long)clean->features[0],
> +	       (unsigned long long)clean->features[1]);
> +
> +	free(clean);
> +	free(dirty);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804080125.79403-1-r4o5m6e8o@163.com?part=3

Re: [PATCH 3/3] tools/virtio: add --features-array switch

[rom.wang]

This is expected, not a bug in the test.

The clear_user() start-address issue in VHOST_GET_FEATURES_ARRAY is the
pre-existing kernel bug that the separately posted fix [1] (Acked-by:
Eugenio) addresses.  Patch 3 here is its regression test, so it
intentionally aborts in verify_get_features_array() against an unfixed
kernel and passes once [1] is applied; the dependency is noted in the
cover letter.

[1] https://lore.kernel.org/all/20260526080336.61296-1-r4o5m6e8o@163.com/

No virus found
		Checked by Hillstone Network AntiVirus