From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932490AbdJZNtp (ORCPT ); Thu, 26 Oct 2017 09:49:45 -0400 Received: from mail-io0-f196.google.com ([209.85.223.196]:46906 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932422AbdJZNsz (ORCPT ); Thu, 26 Oct 2017 09:48:55 -0400 X-Google-Smtp-Source: ABhQp+QK5nQmd7AIbHgc9mw0dYSujtWYUNKu7dwKxlAw57MHhKqIAE2m8ELJx6k4LFV1qy/zBptb7j+zQ+fJaWeqJUA= MIME-Version: 1.0 In-Reply-To: <20171026134547.23664-3-pbonzini@redhat.com> References: <20171026134547.23664-1-pbonzini@redhat.com> <20171026134547.23664-3-pbonzini@redhat.com> From: Kees Cook Date: Thu, 26 Oct 2017 15:48:53 +0200 X-Google-Sender-Auth: GY-8XteXVbr_ZhKnwbuKSPRUyLk Message-ID: Subject: Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl To: Paolo Bonzini Cc: LKML , KVM , Christoffer Dall , Marc Zyngier , Christian Borntraeger , Cornelia Huck , James Hogan , Paul Mackerras , kernel-hardening@lists.openwall.com, =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v9QDnnn4013691 On Thu, Oct 26, 2017 at 3:45 PM, Paolo Bonzini 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@lists.openwall.com > Cc: Kees Cook > Cc: Radim Krčmář > Signed-off-by: Paolo Bonzini > --- > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: <20171026134547.23664-3-pbonzini@redhat.com> References: <20171026134547.23664-1-pbonzini@redhat.com> <20171026134547.23664-3-pbonzini@redhat.com> From: Kees Cook Date: Thu, 26 Oct 2017 15:48:53 +0200 Message-ID: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: [kernel-hardening] Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl To: Paolo Bonzini Cc: LKML , KVM , Christoffer Dall , Marc Zyngier , Christian Borntraeger , Cornelia Huck , James Hogan , Paul Mackerras , kernel-hardening@lists.openwall.com, =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= List-ID: On Thu, Oct 26, 2017 at 3:45 PM, Paolo Bonzini 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@lists.openwall.com > Cc: Kees Cook > Cc: Radim Kr=C4=8Dm=C3=A1=C5=99 > Signed-off-by: Paolo Bonzini > --- > 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 =3D -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 =3D -EINVAL; > - if (kvm->arch.xen_hvm_config.flags) > + if (xhc.flags) > goto out; > + memcpy(&kvm->arch.xen_hvm_config, &xhc, sizeof(xhc)); > r =3D 0; > break; > } > -- > 2.14.2 > --=20 Kees Cook Pixel Security