diff mbox series

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

Message ID 20240223104009.632194-3-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series KVM: SEV: allow customizing VMSA features | expand

Commit Message

Paolo Bonzini Feb. 23, 2024, 10:40 a.m. UTC 
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>
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(-)

Comments

Sean Christopherson Feb. 23, 2024, 2:45 p.m. UTC | #1 
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
Paolo Bonzini Feb. 23, 2024, 3:03 p.m. UTC | #2 
On 2/23/24 15:45, Sean Christopherson wrote:
> 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.

Sounds good.

Paolo
diff mbox series

Patch

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)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d271ba20a0b2..0bcd9ae16097 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1769,6 +1769,7 @@  struct kvm_x86_ops {
 	void (*enable_smi_window)(struct kvm_vcpu *vcpu);
 #endif
 
+	int (*dev_get_attr)(u64 attr, u64 *val);
 	int (*mem_enc_ioctl)(struct kvm *kvm, void __user *argp);
 	int (*mem_enc_register_region)(struct kvm *kvm, struct kvm_enc_region *argp);
 	int (*mem_enc_unregister_region)(struct kvm *kvm, struct kvm_enc_region *argp);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bf10a9073a09..8746530930d5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4804,37 +4804,53 @@  static inline void __user *kvm_get_attr_addr(struct kvm_device_attr *attr)
 	return uaddr;
 }
 
-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);
+		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);
 
-	switch (attr->attr) {
-	case KVM_X86_XCOMP_GUEST_SUPP:
-		if (put_user(kvm_caps.supported_xcr0, uaddr))
-			return -EFAULT;
-		return 0;
-	default:
-		return -ENXIO;
-	}
+	if (put_user(val, uaddr))
+		return -EFAULT;
+
+	return 0;
 }
 
 static int kvm_x86_dev_has_attr(struct kvm_device_attr *attr)
 {
-	if (attr->group)
-		return -ENXIO;
+	u64 val;
 
-	switch (attr->attr) {
-	case KVM_X86_XCOMP_GUEST_SUPP:
-		return 0;
-	default:
-		return -ENXIO;
-	}
+	return __kvm_x86_dev_get_attr(attr, &val);
 }
 
 long kvm_arch_dev_ioctl(struct file *filp,