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

[sashiko-bot@kernel.org]

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