[PATCH 0/1] kvm: reject unknown flags in device and dirty-log ioctls

[PATCH 0/1] kvm: reject unknown flags in device and dirty-log ioctls

[Iván Ezequiel Rodriguez]

Three generic KVM paths silently ignored flag bits that uapi or
Documentation/virt/kvm/api.rst treat as reserved or zero-only:

- KVM_CREATE_DEVICE (only KVM_CREATE_DEVICE_TEST is defined)
- KVM_{SET,GET,HAS}_DEVICE_ATTR (kvm_device_attr.flags)
- KVM_ENABLE_CAP for KVM_CAP_DIRTY_LOG_RING{,_ACQ_REL} (cap->flags)

Reject with -EINVAL, matching dma-heap and other strict KVM ioctls.
Adds ioctl_flag_validation_test to tools/testing/selftests/kvm.

Tested: ioctl_flag_validation_test fails on the host kernel without
this patch (KVM_CREATE_DEVICE accepts flags=0x2); requires reloading
the patched kvm module to pass.

Iván Ezequiel Rodriguez (1):
  kvm: reject unknown flags in device and dirty-log ioctls

 tools/testing/selftests/kvm/Makefile.kvm           |   1 +
 .../selftests/kvm/ioctl_flag_validation_test.c     | 104 +++++++++++++++++++++
 virt/kvm/kvm_main.c                                |   9 ++
 3 files changed, 114 insertions(+)

-- 
2.43.0

[PATCH 1/1] kvm: reject unknown flags in device and dirty-log ioctls

[Iván Ezequiel Rodriguez]

KVM_CREATE_DEVICE only defines KVM_CREATE_DEVICE_TEST, but unknown
flag bits were silently ignored. kvm_device_attr.flags is documented
as unused yet was never checked centrally. KVM_ENABLE_CAP for
KVM_CAP_DIRTY_LOG_RING and KVM_CAP_DIRTY_LOG_RING_ACQ_REL requires
cap->flags to be zero per api.rst, but the generic handler did not
enforce it.

Reject unknown or non-zero flags with -EINVAL, consistent with other
KVM ioctls and dma-heap flag validation. Add a selftest covering all
three paths.

Signed-off-by: Iván Ezequiel Rodriguez <ivanrwcm25@gmail.com>
---
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../kvm/ioctl_flag_validation_test.c          | 104 ++++++++++++++++++
 virt/kvm/kvm_main.c                           |   9 ++
 3 files changed, 114 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/ioctl_flag_validation_test.c

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index d28a057fa6c2..38c78838318c 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -59,6 +59,7 @@ TEST_PROGS_x86 += x86/nx_huge_pages_test.sh
 TEST_GEN_PROGS_COMMON = demand_paging_test
 TEST_GEN_PROGS_COMMON += dirty_log_test
 TEST_GEN_PROGS_COMMON += guest_print_test
+TEST_GEN_PROGS_COMMON += ioctl_flag_validation_test
 TEST_GEN_PROGS_COMMON += irqfd_test
 TEST_GEN_PROGS_COMMON += kvm_binary_stats_test
 TEST_GEN_PROGS_COMMON += kvm_create_max_vcpus
diff --git a/tools/testing/selftests/kvm/ioctl_flag_validation_test.c b/tools/testing/selftests/kvm/ioctl_flag_validation_test.c
new file mode 100644
index 000000000000..30c22435bb14
--- /dev/null
+++ b/tools/testing/selftests/kvm/ioctl_flag_validation_test.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test that selected KVM ioctls reject unknown flag bits with -EINVAL.
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+
+static void test_create_device_unknown_flags(struct kvm_vm *vm)
+{
+	struct kvm_create_device cd = {
+		.type = KVM_DEV_TYPE_VFIO,
+		.flags = 0x2,
+	};
+	int r;
+
+	TEST_REQUIRE(kvm_check_cap(KVM_CAP_DEVICE_CTRL));
+
+	r = __vm_ioctl(vm, KVM_CREATE_DEVICE, &cd);
+	TEST_ASSERT(r == -1 && errno == EINVAL,
+		    "KVM_CREATE_DEVICE with unknown flags");
+
+	cd.flags = KVM_CREATE_DEVICE_TEST | 0x2;
+	r = __vm_ioctl(vm, KVM_CREATE_DEVICE, &cd);
+	TEST_ASSERT(r == -1 && errno == EINVAL,
+		    "KVM_CREATE_DEVICE TEST with unknown flags");
+}
+
+static void test_device_attr_flags(struct kvm_vm *vm)
+{
+	struct kvm_device_attr attr = {
+		.flags = 1,
+		.group = KVM_DEV_VFIO_FILE,
+		.attr = KVM_DEV_VFIO_FILE_ADD,
+		.addr = 0,
+	};
+	int dev_fd, r;
+
+	TEST_REQUIRE(kvm_check_cap(KVM_CAP_DEVICE_CTRL));
+
+	dev_fd = __kvm_create_device(vm, KVM_DEV_TYPE_VFIO);
+	if (dev_fd < 0) {
+		pr_info("Skipping device_attr test, KVM_DEV_TYPE_VFIO unavailable\n");
+		return;
+	}
+
+	r = __kvm_ioctl(dev_fd, KVM_HAS_DEVICE_ATTR, &attr);
+	TEST_ASSERT(r == -1 && errno == EINVAL,
+		    "KVM_HAS_DEVICE_ATTR with unknown flags");
+
+	close(dev_fd);
+}
+
+static void test_dirty_log_ring_cap_flags(struct kvm_vm *vm)
+{
+	struct kvm_enable_cap cap = {
+		.flags = 1,
+		.args = { 4096 },
+	};
+	int r;
+
+	if (!kvm_has_cap(KVM_CAP_DIRTY_LOG_RING) &&
+	    !kvm_has_cap(KVM_CAP_DIRTY_LOG_RING_ACQ_REL)) {
+		pr_info("Skipping dirty log ring cap flag test, cap unavailable\n");
+		return;
+	}
+
+	if (kvm_has_cap(KVM_CAP_DIRTY_LOG_RING)) {
+		cap.cap = KVM_CAP_DIRTY_LOG_RING;
+		cap.flags = 1;
+		r = __vm_ioctl(vm, KVM_ENABLE_CAP, &cap);
+		TEST_ASSERT(r == -1 && errno == EINVAL,
+			    "KVM_ENABLE_CAP DIRTY_LOG_RING with unknown flags");
+	}
+
+	if (kvm_has_cap(KVM_CAP_DIRTY_LOG_RING_ACQ_REL)) {
+		cap.cap = KVM_CAP_DIRTY_LOG_RING_ACQ_REL;
+		cap.flags = 1;
+		r = __vm_ioctl(vm, KVM_ENABLE_CAP, &cap);
+		TEST_ASSERT(r == -1 && errno == EINVAL,
+			    "KVM_ENABLE_CAP DIRTY_LOG_RING_ACQ_REL with unknown flags");
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vm *vm;
+
+	vm = vm_create_barebones();
+
+	test_create_device_unknown_flags(vm);
+	test_device_attr_flags(vm);
+	test_dirty_log_ring_cap_flags(vm);
+
+	kvm_vm_free(vm);
+	return 0;
+}
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e44c20c04961..a2aecc06ab67 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4719,6 +4719,9 @@ static int kvm_device_ioctl_attr(struct kvm_device *dev,
 	if (copy_from_user(&attr, (void __user *)arg, sizeof(attr)))
 		return -EFAULT;
 
+	if (attr.flags)
+		return -EINVAL;
+
 	return accessor(dev, &attr);
 }
 
@@ -4811,6 +4814,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 	int type;
 	int ret;
 
+	if (cd->flags & ~KVM_CREATE_DEVICE_TEST)
+		return -EINVAL;
+
 	if (cd->type >= ARRAY_SIZE(kvm_device_ops_table))
 		return -ENODEV;
 
@@ -5061,6 +5067,9 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
 		if (!kvm_vm_ioctl_check_extension_generic(kvm, cap->cap))
 			return -EINVAL;
 
+		if (cap->flags)
+			return -EINVAL;
+
 		return kvm_vm_ioctl_enable_dirty_log_ring(kvm, cap->args[0]);
 	case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP: {
 		int r = -EINVAL;
-- 
2.43.0

Re: [PATCH 1/1] kvm: reject unknown flags in device and dirty-log ioctls

[sashiko-bot]

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

New issues:
- [Critical] Adding strict flag validation to previously ignored ioctl parameters breaks backwards compatibility for existing userspace applications.

Pre-existing issues:
- [Medium] The validation for `kvm_device_attr.flags` is incomplete and misses the VM, VCPU, and System ioctl paths.
--

commit c3cf74adec51cd659e1e4d1a49a36240977087b9
Author: Iván Ezequiel Rodriguez <ivanrwcm25@gmail.com>

kvm: reject unknown flags in device and dirty-log ioctls

KVM_CREATE_DEVICE only defines KVM_CREATE_DEVICE_TEST, but unknown
flag bits were silently ignored. kvm_device_attr.flags is documented
as unused yet was never checked centrally. KVM_ENABLE_CAP for
KVM_CAP_DIRTY_LOG_RING and KVM_CAP_DIRTY_LOG_RING_ACQ_REL requires
cap->flags to be zero per api.rst, but the generic handler did not
enforce it.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e44c20c049610..a2aecc06ab67e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4719,6 +4719,9 @@ static int kvm_device_ioctl_attr(struct kvm_device *dev,
>  	if (copy_from_user(&attr, (void __user *)arg, sizeof(attr)))
>  		return -EFAULT;
>  
> +	if (attr.flags)
> +		return -EINVAL;

[Severity: Critical]
Does adding strict flag validation here break backwards compatibility for
existing userspace applications? If userspace previously allocated
struct kvm_device_attr on the stack without explicitly zero-initializing the
flags field, won't it now fail with -EINVAL since the kernel used to
silently ignore it?

[Severity: Medium]
This is a pre-existing issue, but the commit message states
kvm_device_attr.flags is now checked "centrally". However, doesn't
kvm_device_ioctl_attr() only handle device file descriptors?

The same device attribute ioctls (KVM_HAS_DEVICE_ATTR, KVM_GET_DEVICE_ATTR,
KVM_SET_DEVICE_ATTR) can be called on VM or VCPU file descriptors, which
are handled in architecture-specific code like kvm_vcpu_ioctl_device_attr()
in arch/x86/kvm/x86.c or kvm_arch_vcpu_ioctl() in arch/arm64/kvm/arm.c.

Doesn't this bypass the new validation entirely?

> +
>  	return accessor(dev, &attr);
>  }
>  
> @@ -4811,6 +4814,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
>  	int type;
>  	int ret;
>  
> +	if (cd->flags & ~KVM_CREATE_DEVICE_TEST)
> +		return -EINVAL;

[Severity: Critical]
Similar to the issue above, does rejecting unknown flags retroactively break
applications that pass uninitialized stack memory in cd->flags when calling
KVM_CREATE_DEVICE?

> +
>  	if (cd->type >= ARRAY_SIZE(kvm_device_ops_table))
>  		return -ENODEV;
>  
[ ... ]
> @@ -5061,6 +5067,9 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
>  		if (!kvm_vm_ioctl_check_extension_generic(kvm, cap->cap))
>  			return -EINVAL;
>  
> +		if (cap->flags)
> +			return -EINVAL;

[Severity: Critical]
Will this retroactively break backwards compatibility for applications
enabling KVM_CAP_DIRTY_LOG_RING or KVM_CAP_DIRTY_LOG_RING_ACQ_REL if they
haven't explicitly zero-initialized cap->flags?

> +
>  		return kvm_vm_ioctl_enable_dirty_log_ring(kvm, cap->args[0]);
>  	case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP: {
>  		int r = -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701194946.768731-2-ivanrwcm25@gmail.com?part=1