linux-kernel - Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM

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

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