| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <abfd32df-b882-a1eb-b2a0-389fd6a68fac@redhat.com>
Date: Fri, 28 Jan 2022 13:33:55 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
yang.zhong@...el.com
Subject: Re: [PATCH 2/3] KVM: x86: add system attribute to retrieve full set
of supported xsave states
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@...gle.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@...gle.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: {
> --
>
Nice, I applied it.
Paolo
Powered by blists - more mailing lists