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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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@xxxxxxxxxx
> + return -EFAULT;
> + return 0;
> + default:
> + return -ENXIO;
> + break;
> + }
> +}
> +
