On 1/27/22 16:35, Sean Christopherson wrote:
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: { --
Nice, I applied it. Paolo
