| Message ID | 20171026134547.23664-3-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 26, 2017 at 3:45 PM, Paolo Bonzini <pbonzini@redhat.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@lists.openwall.com > Cc: Kees Cook <keescook@chromium.org> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.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 >
On Thu, Oct 26, 2017 at 6:45 AM, Paolo Bonzini <pbonzini@redhat.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@lists.openwall.com > Cc: Kees Cook <keescook@chromium.org> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.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))) > 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 > Hi Paolo, Since this didn't make it via my usercopy tree, do you want to take it via KVM? It is a stand-alone fix, AIUI. Thanks, -Kees
On 30/11/2017 21:40, Kees Cook wrote: > Hi Paolo, > > Since this didn't make it via my usercopy tree, do you want to take it > via KVM? It is a stand-alone fix, AIUI. Yes, will do! Thanks, Paolo
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))) 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; }
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 <keescook@chromium.org> Cc: Radim Krčmář <rkrcmar@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/x86.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)