Re: [PATCH v2 02/11] KVM: introduce new vendor op for KVM_GET_DEVICE_ATTR

[Sean Christopherson <seanjc@google.com>]

KVM: x86:

On Fri, Feb 23, 2024, Paolo Bonzini wrote:
> Allow vendor modules to provide their own attributes on /dev/kvm.
> To avoid proliferation of vendor ops, implement KVM_HAS_DEVICE_ATTR
> and KVM_GET_DEVICE_ATTR in terms of the same function.  You're not
> supposed to use KVM_GET_DEVICE_ATTR to do complicated computations,
> especially on /dev/kvm.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20240209183743.22030-3-pbonzini@redhat.com>

Another double-stamp that needs to be dropped.

> Reviewed-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
>  arch/x86/include/asm/kvm_host.h    |  1 +
>  arch/x86/kvm/x86.c                 | 52 +++++++++++++++++++-----------
>  3 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 378ed944b849..ac8b7614e79d 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -122,6 +122,7 @@ KVM_X86_OP(enter_smm)
>  KVM_X86_OP(leave_smm)
>  KVM_X86_OP(enable_smi_window)
>  #endif
> +KVM_X86_OP_OPTIONAL(dev_get_attr)
>  KVM_X86_OP_OPTIONAL(mem_enc_ioctl)
>  KVM_X86_OP_OPTIONAL(mem_enc_register_region)
>  KVM_X86_OP_OPTIONAL(mem_enc_unregister_region)

...

> -static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
> +static int __kvm_x86_dev_get_attr(struct kvm_device_attr *attr, u64 *val)
>  {
> -	u64 __user *uaddr = kvm_get_attr_addr(attr);
> +	int r;
>  
>  	if (attr->group)
>  		return -ENXIO;
>  
> +	switch (attr->attr) {
> +	case KVM_X86_XCOMP_GUEST_SUPP:
> +		r = 0;
> +		*val = kvm_caps.supported_xcr0;
> +		break;
> +	default:
> +		r = -ENXIO;
> +		if (kvm_x86_ops.dev_get_attr)
> +			r = kvm_x86_ops.dev_get_attr(attr->attr, val);

If you're going to add an entry in kvm-x86-ops.h, might as well use static_call().,

> +		break;
> +	}
> +
> +	return r;
> +}
> +
> +static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
> +{
> +	u64 __user *uaddr;
> +	int r;
> +	u64 val;
> +
> +	r = __kvm_x86_dev_get_attr(attr, &val);
> +	if (r < 0)
> +		return r;
> +
> +	uaddr = kvm_get_attr_addr(attr);
>  	if (IS_ERR(uaddr))
>  		return PTR_ERR(uaddr);

Way off topic, do we actually need the sanity check that userspace didn't provide
garbage in bits 63:32 when running on a 32-bit kernel?  Aside from the fact that
no one uses 32-bit KVM, if userspace provides a pointer that happens to resolve
to an address in the task's address space, so be it, userspace gets to keep its
pieces.  There's no danger to the kernel because KVM correctly uses the checked
versions of {get,put}_user().

The paranoid check came in with the TSC attribute

  static inline void __user *kvm_get_attr_addr(struct kvm_device_attr *attr)
  {
	void __user *uaddr = (void __user*)(unsigned long)attr->addr;

	if ((u64)(unsigned long)uaddr != attr->addr)
		return ERR_PTR_USR(-EFAULT);
	return uaddr;
  }

I was responsible for the paranoia, though Oliver gets blamed because the fixup
got squashed[1].  And the only reason I posted the paranoid code was because the
very original code did:

  arch/x86/kvm/x86.c: In function ‘kvm_arch_tsc_get_attr’:
  arch/x86/kvm/x86.c:4947:22: error: cast to pointer from integer of different size
   4947 |  u64 __user *uaddr = (u64 __user *)attr->addr;
        |                      ^
  arch/x86/kvm/x86.c: In function ‘kvm_arch_tsc_set_attr’:
  arch/x86/kvm/x86.c:4967:22: error: cast to pointer from integer of different size
   4967 |  u64 __user *uaddr = (u64 __user *)attr->addr;we ended up 

And as I recently found out[2], u64_to_user_ptr() exists for this exact reason.

I vote to convert to u64_to_user_ptr() as a prep patch, which would make this all
quite a bit more readable.

[1] https://lore.kernel.org/all/20211007231647.3553604-1-seanjc@google.com
[2] https://lore.kernel.org/all/20240222100412.560961-1-arnd@kernel.org