[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jKc8zFR0q_DScdEvWfXyBR-TjefrZ6vx1VvMghE05ssiw@mail.gmail.com>
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
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
