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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 26 Oct 2017 15:48:53 +0200
From:   Kees Cook <keescook@...omium.org>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, KVM <kvm@...r.kernel.org>,
        Christoffer Dall <christoffer.dall@...aro.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Cornelia Huck <cohuck@...hat.com>,
        James Hogan <james.hogan@...tec.com>,
        Paul Mackerras <paulus@...ba.org>,
        kernel-hardening@...ts.openwall.com,
        Radim Krčmář <rkrcmar@...hat.com>
Subject: Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

On Thu, Oct 26, 2017 at 3:45 PM, Paolo Bonzini <pbonzini@...hat.com> wrote:
> This ioctl is obsolete (it was used by Xenner as far as I know) but
> still let's not break it gratuitously...  Its handler is copying
> directly into struct kvm.  Go through a bounce buffer instead, with
> the added benefit that we can actually do something useful with the
> flags argument---the previous code was exiting with -EINVAL but still
> doing the copy.
>
> This technically is a userspace ABI breakage, but since no one should be
> using the ioctl, it's a good occasion to see if someone actually
> complains.
>
> Cc: kernel-hardening@...ts.openwall.com
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Radim Krčmář <rkrcmar@...hat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/kvm/x86.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 272320eb328c..f32fbfb833b3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4187,13 +4187,14 @@ long kvm_arch_vm_ioctl(struct file *filp,
>                 mutex_unlock(&kvm->lock);
>                 break;
>         case KVM_XEN_HVM_CONFIG: {
> +               struct kvm_xen_hvm_config xhc;
>                 r = -EFAULT;
> -               if (copy_from_user(&kvm->arch.xen_hvm_config, argp,
> -                                  sizeof(struct kvm_xen_hvm_config)))
> +               if (copy_from_user(&xhc, argp, sizeof(xhc)))

This is replacing a builtin_const size argument, which would already
be allowed by usercopy hardening (const sizes are implicit whitelists,
since they cannot change size at runtime). However, as you point out,
this API should already have been doing a bounce copy to check the
flags sanity.

(I'll add this to the hardening series, thanks!)

-Kees

>                         goto out;
>                 r = -EINVAL;
> -               if (kvm->arch.xen_hvm_config.flags)
> +               if (xhc.flags)
>                         goto out;
> +               memcpy(&kvm->arch.xen_hvm_config, &xhc, sizeof(xhc));
>                 r = 0;
>                 break;
>         }
> --
> 2.14.2
>



-- 
Kees Cook
Pixel Security

Powered by blists - more mailing lists