| Message ID | 20171026134547.23664-2-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 26 Oct 2017 15:45:46 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On x86, ARM and s390, struct kvm_vcpu_arch has a usercopy region > taht is read and written by the KVM_GET/SET_CPUID2 ioctls (x86) > or KVM_GET/SET_ONE_REG (ARM/s390). Without whitelisting the area, > KVM is completely broken on those architectures with usercopy hardening > enabled. > > For now, allow writing to the entire struct on all architectures. > The KVM tree will not refine this to an architecture-specific > subset of struct kvm_vcpu_arch. > > Cc: kernel-hardening@lists.openwall.com > Cc: Kees Cook <keescook@chromium.org> > Cc: Christian Borntraeger <borntraeger@redhat.com> > Cc: Christoffer Dall <cdall@linaro.org> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > virt/kvm/kvm_main.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 4d81f6ded88e..b4809ccfdfa1 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4005,8 +4005,12 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, > /* A kmem cache lets us meet the alignment requirements of fx_save. */ > if (!vcpu_align) > vcpu_align = __alignof__(struct kvm_vcpu); > - kvm_vcpu_cache = kmem_cache_create("kvm_vcpu", vcpu_size, vcpu_align, > - 0, NULL); > + kvm_vcpu_cache = > + kmem_cache_create_usercopy("kvm_vcpu", > + sizeof(struct kvm_vcpu), vcpu_align, > + 0, offsetof(struct kvm_vcpu, arch), > + sizeof_field(struct kvm_vcpu, arch), > + NULL); > if (!kvm_vcpu_cache) { > r = -ENOMEM; > goto out_free_3; Acked-by: Cornelia Huck <cohuck@redhat.com>
On Thu, Oct 26, 2017 at 3:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On x86, ARM and s390, struct kvm_vcpu_arch has a usercopy region > taht is read and written by the KVM_GET/SET_CPUID2 ioctls (x86) > or KVM_GET/SET_ONE_REG (ARM/s390). Without whitelisting the area, > KVM is completely broken on those architectures with usercopy hardening > enabled. > > For now, allow writing to the entire struct on all architectures. > The KVM tree will not refine this to an architecture-specific > subset of struct kvm_vcpu_arch. > > Cc: kernel-hardening@lists.openwall.com > Cc: Kees Cook <keescook@chromium.org> > Cc: Christian Borntraeger <borntraeger@redhat.com> > Cc: Christoffer Dall <cdall@linaro.org> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > virt/kvm/kvm_main.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 4d81f6ded88e..b4809ccfdfa1 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4005,8 +4005,12 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, > /* A kmem cache lets us meet the alignment requirements of fx_save. */ > if (!vcpu_align) > vcpu_align = __alignof__(struct kvm_vcpu); > - kvm_vcpu_cache = kmem_cache_create("kvm_vcpu", vcpu_size, vcpu_align, > - 0, NULL); > + kvm_vcpu_cache = > + kmem_cache_create_usercopy("kvm_vcpu", > + sizeof(struct kvm_vcpu), vcpu_align, > + 0, offsetof(struct kvm_vcpu, arch), > + sizeof_field(struct kvm_vcpu, arch), > + NULL); > if (!kvm_vcpu_cache) { > r = -ENOMEM; > goto out_free_3; > -- > 2.14.2 Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
On Thu, Oct 26 2017 at 3:45:46 pm BST, Paolo Bonzini <pbonzini@redhat.com> wrote: > On x86, ARM and s390, struct kvm_vcpu_arch has a usercopy region > taht is read and written by the KVM_GET/SET_CPUID2 ioctls (x86) > or KVM_GET/SET_ONE_REG (ARM/s390). Without whitelisting the area, > KVM is completely broken on those architectures with usercopy hardening > enabled. > > For now, allow writing to the entire struct on all architectures. > The KVM tree will not refine this to an architecture-specific > subset of struct kvm_vcpu_arch. > > Cc: kernel-hardening@lists.openwall.com > Cc: Kees Cook <keescook@chromium.org> > Cc: Christian Borntraeger <borntraeger@redhat.com> > Cc: Christoffer Dall <cdall@linaro.org> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Acked-by: Marc Zyngier <marc.zyngier@arm.com> M.
On 10/26/2017 03:45 PM, Paolo Bonzini wrote: > On x86, ARM and s390, struct kvm_vcpu_arch has a usercopy region > taht is read and written by the KVM_GET/SET_CPUID2 ioctls (x86) > or KVM_GET/SET_ONE_REG (ARM/s390). Without whitelisting the area, > KVM is completely broken on those architectures with usercopy hardening > enabled. > > For now, allow writing to the entire struct on all architectures. > The KVM tree will not refine this to an architecture-specific > subset of struct kvm_vcpu_arch. > > Cc: kernel-hardening@lists.openwall.com > Cc: Kees Cook <keescook@chromium.org> > Cc: Christian Borntraeger <borntraeger@redhat.com> You wish? Not yet Paolo :-) > Cc: Christoffer Dall <cdall@linaro.org> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > virt/kvm/kvm_main.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 4d81f6ded88e..b4809ccfdfa1 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4005,8 +4005,12 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, > /* A kmem cache lets us meet the alignment requirements of fx_save. */ > if (!vcpu_align) > vcpu_align = __alignof__(struct kvm_vcpu); > - kvm_vcpu_cache = kmem_cache_create("kvm_vcpu", vcpu_size, vcpu_align, > - 0, NULL); > + kvm_vcpu_cache = > + kmem_cache_create_usercopy("kvm_vcpu", > + sizeof(struct kvm_vcpu), vcpu_align, > + 0, offsetof(struct kvm_vcpu, arch), > + sizeof_field(struct kvm_vcpu, arch), > + NULL); > if (!kvm_vcpu_cache) { > r = -ENOMEM; > goto out_free_3; >
On Thu, Oct 26, 2017 at 03:45:46PM +0200, Paolo Bonzini wrote: > On x86, ARM and s390, struct kvm_vcpu_arch has a usercopy region > taht is read and written by the KVM_GET/SET_CPUID2 ioctls (x86) > or KVM_GET/SET_ONE_REG (ARM/s390). Without whitelisting the area, > KVM is completely broken on those architectures with usercopy hardening > enabled. > > For now, allow writing to the entire struct on all architectures. > The KVM tree will not refine this to an architecture-specific > subset of struct kvm_vcpu_arch. > > Cc: kernel-hardening@lists.openwall.com > Cc: Kees Cook <keescook@chromium.org> > Cc: Christian Borntraeger <borntraeger@redhat.com> > Cc: Christoffer Dall <cdall@linaro.org> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > virt/kvm/kvm_main.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 4d81f6ded88e..b4809ccfdfa1 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4005,8 +4005,12 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, > /* A kmem cache lets us meet the alignment requirements of fx_save. */ > if (!vcpu_align) > vcpu_align = __alignof__(struct kvm_vcpu); > - kvm_vcpu_cache = kmem_cache_create("kvm_vcpu", vcpu_size, vcpu_align, > - 0, NULL); > + kvm_vcpu_cache = > + kmem_cache_create_usercopy("kvm_vcpu", > + sizeof(struct kvm_vcpu), vcpu_align, > + 0, offsetof(struct kvm_vcpu, arch), > + sizeof_field(struct kvm_vcpu, arch), > + NULL); Doesn't it need to be 'vcpu_size' instead of 'sizeof(struct kvm_vcpu)'? Eric
On Mon, Oct 30, 2017 at 4:28 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > On Thu, Oct 26, 2017 at 03:45:46PM +0200, Paolo Bonzini wrote: >> On x86, ARM and s390, struct kvm_vcpu_arch has a usercopy region >> taht is read and written by the KVM_GET/SET_CPUID2 ioctls (x86) >> or KVM_GET/SET_ONE_REG (ARM/s390). Without whitelisting the area, >> KVM is completely broken on those architectures with usercopy hardening >> enabled. >> >> For now, allow writing to the entire struct on all architectures. >> The KVM tree will not refine this to an architecture-specific >> subset of struct kvm_vcpu_arch. >> >> Cc: kernel-hardening@lists.openwall.com >> Cc: Kees Cook <keescook@chromium.org> >> Cc: Christian Borntraeger <borntraeger@redhat.com> >> Cc: Christoffer Dall <cdall@linaro.org> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> virt/kvm/kvm_main.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 4d81f6ded88e..b4809ccfdfa1 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -4005,8 +4005,12 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, >> /* A kmem cache lets us meet the alignment requirements of fx_save. */ >> if (!vcpu_align) >> vcpu_align = __alignof__(struct kvm_vcpu); >> - kvm_vcpu_cache = kmem_cache_create("kvm_vcpu", vcpu_size, vcpu_align, >> - 0, NULL); >> + kvm_vcpu_cache = >> + kmem_cache_create_usercopy("kvm_vcpu", >> + sizeof(struct kvm_vcpu), vcpu_align, >> + 0, offsetof(struct kvm_vcpu, arch), >> + sizeof_field(struct kvm_vcpu, arch), >> + NULL); > > Doesn't it need to be 'vcpu_size' instead of 'sizeof(struct kvm_vcpu)'? Oh, yikes, yes. $ git grep '\bkvm_init(' arch/mips/kvm/mips.c: ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE); arch/powerpc/kvm/book3s.c: r = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE); arch/powerpc/kvm/e500.c: r = kvm_init(NULL, sizeof(struct kvmppc_vcpu_e500), 0, THIS_MODULE); arch/powerpc/kvm/e500mc.c: r = kvm_init(NULL, sizeof(struct kvmppc_vcpu_e500), 0, THIS_MODULE); arch/s390/kvm/kvm-s390.c: return kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE); arch/x86/kvm/svm.c: return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm), arch/x86/kvm/vmx.c: int r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx), include/linux/kvm_host.h:int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, virt/kvm/arm/arm.c: int rc = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE); I'll fix this up. -Kees
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4d81f6ded88e..b4809ccfdfa1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4005,8 +4005,12 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, /* A kmem cache lets us meet the alignment requirements of fx_save. */ if (!vcpu_align) vcpu_align = __alignof__(struct kvm_vcpu); - kvm_vcpu_cache = kmem_cache_create("kvm_vcpu", vcpu_size, vcpu_align, - 0, NULL); + kvm_vcpu_cache = + kmem_cache_create_usercopy("kvm_vcpu", + sizeof(struct kvm_vcpu), vcpu_align, + 0, offsetof(struct kvm_vcpu, arch), + sizeof_field(struct kvm_vcpu, arch), + NULL); if (!kvm_vcpu_cache) { r = -ENOMEM; goto out_free_3;
On x86, ARM and s390, struct kvm_vcpu_arch has a usercopy region taht is read and written by the KVM_GET/SET_CPUID2 ioctls (x86) or KVM_GET/SET_ONE_REG (ARM/s390). Without whitelisting the area, KVM is completely broken on those architectures with usercopy hardening enabled. For now, allow writing to the entire struct on all architectures. The KVM tree will not refine this to an architecture-specific subset of struct kvm_vcpu_arch. Cc: kernel-hardening@lists.openwall.com Cc: Kees Cook <keescook@chromium.org> Cc: Christian Borntraeger <borntraeger@redhat.com> Cc: Christoffer Dall <cdall@linaro.org> Cc: Radim Krčmář <rkrcmar@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- virt/kvm/kvm_main.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)