Re: [PATCH 2/3] KVM: x86: add system attribute to retrieve full set of supported xsave states

[Sean Christopherson <seanjc@google.com>]

On Wed, Jan 26, 2022, Paolo Bonzini wrote:
> +static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
> +{
> +	if (attr->group)
> +		return -ENXIO;
> +
> +	switch (attr->attr) {
> +	case KVM_X86_XCOMP_GUEST_SUPP:
> +		if (put_user(supported_xcr0, (u64 __user *)attr->addr))

Deja vu[*].

  arch/x86/kvm/x86.c: In function ‘kvm_x86_dev_get_attr’:
  arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                                              ^
  arch/x86/include/asm/uaccess.h:221:31: note: in definition of macro ‘do_put_user_call’
    221 |         register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX);           \
        |                               ^~~
  arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                     ^~~~~~~~
  arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                                              ^
  arch/x86/include/asm/uaccess.h:223:21: note: in definition of macro ‘do_put_user_call’
    223 |         __ptr_pu = (ptr);                                               \
        |                     ^~~
  arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                     ^~~~~~~~
  arch/x86/kvm/x86.c:4345:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))
        |                                              ^
  arch/x86/include/asm/uaccess.h:230:45: note: in definition of macro ‘do_put_user_call’
    230 |                        [size] "i" (sizeof(*(ptr)))                      \
        |                                             ^~~
  arch/x86/kvm/x86.c:4345:21: note: in expansion of macro ‘put_user’
   4345 |                 if (put_user(supported_xcr0, (u64 __user *)attr->addr))

Given that we're collectively 2 for 2 in mishandling {g,s}et_attr(), what about
a prep pacth like so?  Compile tested only...

From: Sean Christopherson <seanjc@google.com>
Date: Thu, 27 Jan 2022 07:31:53 -0800
Subject: [PATCH] KVM: x86: Add a helper to retrieve userspace address from
 kvm_device_attr

Add a helper to handle converting the u64 userspace address embedded in
struct kvm_device_attr into a userspace pointer, it's all too easy to
forget the intermediate "unsigned long" cast as well as the truncation
check.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8033eca6f3a1..67836f7c71f5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4335,14 +4335,28 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	return r;
 }

+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(-EFAULT);
+	return uaddr;
+}
+
 static int kvm_x86_dev_get_attr(struct kvm_device_attr *attr)
 {
+	u64 __user *uaddr = kvm_get_attr_addr(attr);
+
 	if (attr->group)
 		return -ENXIO;

+	if (IS_ERR(uaddr))
+		return PTR_ERR(uaddr);
+
 	switch (attr->attr) {
 	case KVM_X86_XCOMP_GUEST_SUPP:
-		if (put_user(supported_xcr0, (u64 __user *)attr->addr))
+		if (put_user(supported_xcr0, uaddr))
 			return -EFAULT;
 		return 0;
 	default:
@@ -5070,11 +5084,11 @@ static int kvm_arch_tsc_has_attr(struct kvm_vcpu *vcpu,
 static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
 				 struct kvm_device_attr *attr)
 {
-	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
+	u64 __user *uaddr = kvm_get_attr_addr(attr);
 	int r;

-	if ((u64)(unsigned long)uaddr != attr->addr)
-		return -EFAULT;
+	if (IS_ERR(uaddr))
+		return PTR_ERR(uaddr);

 	switch (attr->attr) {
 	case KVM_VCPU_TSC_OFFSET:
@@ -5093,12 +5107,12 @@ static int kvm_arch_tsc_get_attr(struct kvm_vcpu *vcpu,
 static int kvm_arch_tsc_set_attr(struct kvm_vcpu *vcpu,
 				 struct kvm_device_attr *attr)
 {
-	u64 __user *uaddr = (u64 __user *)(unsigned long)attr->addr;
+	u64 __user *uaddr = kvm_get_attr_addr(attr);
 	struct kvm *kvm = vcpu->kvm;
 	int r;

-	if ((u64)(unsigned long)uaddr != attr->addr)
-		return -EFAULT;
+	if (IS_ERR(uaddr))
+		return PTR_ERR(uaddr);

 	switch (attr->attr) {
 	case KVM_VCPU_TSC_OFFSET: {
--



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


> +			return -EFAULT;
> +		return 0;
> +	default:
> +		return -ENXIO;
> +		break;
> +	}
> +}
> +