[PATCH 0/2] KVM: fixes for the kernel-hardening tree

[PATCH 0/2] KVM: fixes for the kernel-hardening tree

[Paolo Bonzini]

Two KVM ioctls (KVM_GET/SET_CPUID2) directly access the cpuid_entries
field of struct kvm_vcpu_arch.  Therefore, the new usercopy hardening
work in linux-next, which forbids copies from and to slab objects
unless they are from kmalloc or explicitly whitelisted, breaks KVM
completely.

This series fixes it by adding the two new usercopy arguments
to kvm_init (more precisely to a new function kvm_init_usercopy,
while kvm_init passes zeroes as a default).

There's also another broken ioctl, KVM_XEN_HVM_CONFIG, but it is
obsolete and not a big deal at all.

I'm Ccing all submaintainers in case they have something similar
going on in their kvm_arch and kvm_vcpu_arch structs.  KVM has a
pretty complex userspace API, so thorough with linux-next is highly
recommended.

Many thanks to Thomas Gleixner for reporting this to me.

Paolo

Paolo Bonzini (2):
  KVM: allow setting a usercopy region in struct kvm_vcpu
  KVM: fix KVM_XEN_HVM_CONFIG ioctl

 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/svm.c              |  4 ++--
 arch/x86/kvm/vmx.c              |  4 ++--
 arch/x86/kvm/x86.c              | 17 ++++++++++++++---
 include/linux/kvm_host.h        | 13 +++++++++++--
 virt/kvm/kvm_main.c             | 13 ++++++++-----
 6 files changed, 40 insertions(+), 14 deletions(-)

-- 
2.14.2

[PATCH 1/2] KVM: allow setting a usercopy region in struct kvm_vcpu

[Paolo Bonzini]

On x86, struct kvm_vcpu has a usercopy region corresponding to the CPUID
entries.  The area is read and written by the KVM_GET/SET_CPUID2 ioctls.
Without this patch, KVM is completely broken on x86 with usercopy
hardening enabled.

Define kvm_init in terms of a more generic function that allows setting
a usercopy region.  Because x86 has separate kvm_init callers for Intel and
AMD, another variant called kvm_init_x86 passes the region corresponding
to the cpuid_entries array.

Reported-by: Thomas Gleixner <tglx@linutronix.de>
Cc: kernel-hardening@lists.openwall.com
Cc: Kees Cook <keescook@chromium.org>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: James Hogan <james.hogan@imgtec.com>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	The patch is on top of linux-next.

 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/svm.c              |  4 ++--
 arch/x86/kvm/vmx.c              |  4 ++--
 arch/x86/kvm/x86.c              | 10 ++++++++++
 include/linux/kvm_host.h        | 13 +++++++++++--
 virt/kvm/kvm_main.c             | 13 ++++++++-----
 6 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6b8f937ca398..bb8243d413d0 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1420,6 +1420,9 @@ static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
 
 static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
 
+int kvm_init_x86(struct kvm_x86_ops *kvm_x86_ops, unsigned vcpu_size,
+	         unsigned vcpu_align, struct module *module);
+
 static inline int kvm_cpu_get_apicid(int mps_cpu)
 {
 #ifdef CONFIG_X86_LOCAL_APIC
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ff94552f85d0..457433c3a703 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5594,8 +5594,8 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 
 static int __init svm_init(void)
 {
-	return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
-			__alignof__(struct vcpu_svm), THIS_MODULE);
+	return kvm_init_x86(&svm_x86_ops, sizeof(struct vcpu_svm),
+			    __alignof__(struct vcpu_svm), THIS_MODULE);
 }
 
 static void __exit svm_exit(void)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c460b0b439d3..6e78530df6a8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -12106,8 +12106,8 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
 
 static int __init vmx_init(void)
 {
-	int r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
-                     __alignof__(struct vcpu_vmx), THIS_MODULE);
+	int r = kvm_init_x86(&vmx_x86_ops, sizeof(struct vcpu_vmx),
+			     __alignof__(struct vcpu_vmx), THIS_MODULE);
 	if (r)
 		return r;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5669af09b732..415529a78c37 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8181,6 +8181,16 @@ void kvm_arch_sync_events(struct kvm *kvm)
 	kvm_free_pit(kvm);
 }
 
+int kvm_init_x86(struct kvm_x86_ops *kvm_x86_ops, unsigned vcpu_size,
+		 unsigned vcpu_align, struct module *module)
+{
+	return kvm_init_usercopy(kvm_x86_ops, vcpu_size, vcpu_align,
+				 offsetof(struct kvm_vcpu_arch, cpuid_entries),
+				 sizeof_field(struct kvm_vcpu_arch, cpuid_entries),
+				 module);
+}
+EXPORT_SYMBOL_GPL(kvm_init_x86);
+
 int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
 {
 	int i, r;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6882538eda32..21e19658b086 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -561,8 +561,17 @@ static inline void kvm_irqfd_exit(void)
 {
 }
 #endif
-int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
-		  struct module *module);
+
+int kvm_init_usercopy(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
+		      unsigned vcpu_usercopy_start, unsigned vcpu_usercopy_size,
+		      struct module *module);
+
+static inline int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
+			   struct module *module)
+{
+	return kvm_init_usercopy(opaque, vcpu_size, vcpu_align, 0, 0, module);
+}
+
 void kvm_exit(void);
 
 void kvm_get_kvm(struct kvm *kvm);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 261c782a688f..ac889b28bb54 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3959,8 +3959,9 @@ static void kvm_sched_out(struct preempt_notifier *pn,
 	kvm_arch_vcpu_put(vcpu);
 }
 
-int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
-		  struct module *module)
+int kvm_init_usercopy(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
+		      unsigned vcpu_arch_usercopy_start, unsigned vcpu_arch_usercopy_size,
+		      struct module *module)
 {
 	int r;
 	int cpu;
@@ -4006,8 +4007,10 @@ 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,
-					   SLAB_ACCOUNT, NULL);
+	kvm_vcpu_cache = kmem_cache_create_usercopy("kvm_vcpu", vcpu_size, vcpu_align,
+						    SLAB_ACCOUNT,
+						    offsetof(struct kvm_vcpu, arch) + vcpu_arch_usercopy_start,
+						    vcpu_arch_usercopy_size, NULL);
 	if (!kvm_vcpu_cache) {
 		r = -ENOMEM;
 		goto out_free_3;
@@ -4065,7 +4068,7 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 out_fail:
 	return r;
 }
-EXPORT_SYMBOL_GPL(kvm_init);
+EXPORT_SYMBOL_GPL(kvm_init_usercopy);
 
 void kvm_exit(void)
 {
-- 
2.14.2

[PATCH 2/2] KVM: fix KVM_XEN_HVM_CONFIG ioctl

[Paolo Bonzini]

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 415529a78c37..c76d7afa30be 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4218,13 +4218,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

Re: [PATCH 1/2] KVM: allow setting a usercopy region in struct kvm_vcpu

[Kees Cook]

On Fri, Oct 20, 2017 at 4:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On x86, struct kvm_vcpu has a usercopy region corresponding to the CPUID
> entries.  The area is read and written by the KVM_GET/SET_CPUID2 ioctls.
> Without this patch, KVM is completely broken on x86 with usercopy
> hardening enabled.
>
> Define kvm_init in terms of a more generic function that allows setting
> a usercopy region.  Because x86 has separate kvm_init callers for Intel and
> AMD, another variant called kvm_init_x86 passes the region corresponding
> to the cpuid_entries array.
>
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: kernel-hardening@lists.openwall.com
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: James Hogan <james.hogan@imgtec.com>
> Cc: Paul Mackerras <paulus@samba.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         The patch is on top of linux-next.
>
>  arch/x86/include/asm/kvm_host.h |  3 +++
>  arch/x86/kvm/svm.c              |  4 ++--
>  arch/x86/kvm/vmx.c              |  4 ++--
>  arch/x86/kvm/x86.c              | 10 ++++++++++
>  include/linux/kvm_host.h        | 13 +++++++++++--
>  virt/kvm/kvm_main.c             | 13 ++++++++-----
>  6 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 6b8f937ca398..bb8243d413d0 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1420,6 +1420,9 @@ static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
>
>  static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
>
> +int kvm_init_x86(struct kvm_x86_ops *kvm_x86_ops, unsigned vcpu_size,
> +                unsigned vcpu_align, struct module *module);
> +
>  static inline int kvm_cpu_get_apicid(int mps_cpu)
>  {
>  #ifdef CONFIG_X86_LOCAL_APIC
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ff94552f85d0..457433c3a703 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5594,8 +5594,8 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
>
>  static int __init svm_init(void)
>  {
> -       return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
> -                       __alignof__(struct vcpu_svm), THIS_MODULE);
> +       return kvm_init_x86(&svm_x86_ops, sizeof(struct vcpu_svm),
> +                           __alignof__(struct vcpu_svm), THIS_MODULE);
>  }
>
>  static void __exit svm_exit(void)
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c460b0b439d3..6e78530df6a8 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -12106,8 +12106,8 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
>
>  static int __init vmx_init(void)
>  {
> -       int r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
> -                     __alignof__(struct vcpu_vmx), THIS_MODULE);
> +       int r = kvm_init_x86(&vmx_x86_ops, sizeof(struct vcpu_vmx),
> +                            __alignof__(struct vcpu_vmx), THIS_MODULE);
>         if (r)
>                 return r;
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5669af09b732..415529a78c37 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8181,6 +8181,16 @@ void kvm_arch_sync_events(struct kvm *kvm)
>         kvm_free_pit(kvm);
>  }
>
> +int kvm_init_x86(struct kvm_x86_ops *kvm_x86_ops, unsigned vcpu_size,
> +                unsigned vcpu_align, struct module *module)
> +{
> +       return kvm_init_usercopy(kvm_x86_ops, vcpu_size, vcpu_align,
> +                                offsetof(struct kvm_vcpu_arch, cpuid_entries),
> +                                sizeof_field(struct kvm_vcpu_arch, cpuid_entries),
> +                                module);
> +}
> +EXPORT_SYMBOL_GPL(kvm_init_x86);
> +
>  int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
>  {
>         int i, r;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 6882538eda32..21e19658b086 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -561,8 +561,17 @@ static inline void kvm_irqfd_exit(void)
>  {
>  }
>  #endif
> -int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
> -                 struct module *module);
> +
> +int kvm_init_usercopy(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
> +                     unsigned vcpu_usercopy_start, unsigned vcpu_usercopy_size,
> +                     struct module *module);
> +
> +static inline int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
> +                          struct module *module)
> +{
> +       return kvm_init_usercopy(opaque, vcpu_size, vcpu_align, 0, 0, module);
> +}
> +
>  void kvm_exit(void);
>
>  void kvm_get_kvm(struct kvm *kvm);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 261c782a688f..ac889b28bb54 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3959,8 +3959,9 @@ static void kvm_sched_out(struct preempt_notifier *pn,
>         kvm_arch_vcpu_put(vcpu);
>  }
>
> -int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
> -                 struct module *module)
> +int kvm_init_usercopy(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
> +                     unsigned vcpu_arch_usercopy_start, unsigned vcpu_arch_usercopy_size,
> +                     struct module *module)
>  {
>         int r;
>         int cpu;
> @@ -4006,8 +4007,10 @@ 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,
> -                                          SLAB_ACCOUNT, NULL);
> +       kvm_vcpu_cache = kmem_cache_create_usercopy("kvm_vcpu", vcpu_size, vcpu_align,
> +                                                   SLAB_ACCOUNT,
> +                                                   offsetof(struct kvm_vcpu, arch) + vcpu_arch_usercopy_start,
> +                                                   vcpu_arch_usercopy_size, NULL);

I adjusted this hunk for the usercopy tree (SLAB_ACCOUNT got added in
the KVM tree, I think).

-Kees

>         if (!kvm_vcpu_cache) {
>                 r = -ENOMEM;
>                 goto out_free_3;
> @@ -4065,7 +4068,7 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
>  out_fail:
>         return r;
>  }
> -EXPORT_SYMBOL_GPL(kvm_init);
> +EXPORT_SYMBOL_GPL(kvm_init_usercopy);
>
>  void kvm_exit(void)
>  {
> --
> 2.14.2
>
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 0/2] KVM: fixes for the kernel-hardening tree

[David Hildenbrand]

On 21.10.2017 01:25, Paolo Bonzini wrote:
> Two KVM ioctls (KVM_GET/SET_CPUID2) directly access the cpuid_entries
> field of struct kvm_vcpu_arch.  Therefore, the new usercopy hardening
> work in linux-next, which forbids copies from and to slab objects
> unless they are from kmalloc or explicitly whitelisted, breaks KVM
> completely.
> 
> This series fixes it by adding the two new usercopy arguments
> to kvm_init (more precisely to a new function kvm_init_usercopy,
> while kvm_init passes zeroes as a default).
> 
> There's also another broken ioctl, KVM_XEN_HVM_CONFIG, but it is
> obsolete and not a big deal at all.
> 
> I'm Ccing all submaintainers in case they have something similar
> going on in their kvm_arch and kvm_vcpu_arch structs.  KVM has a
> pretty complex userspace API, so thorough with linux-next is highly
> recommended.

I assume on s390x, at least

kvm_arch_vcpu_ioctl_get_one_reg() and
kvm_arch_vcpu_ioctl_set_one_reg()

have to be fixed.

Christian, are you already looking into this?

> 
> Many thanks to Thomas Gleixner for reporting this to me.
> 
> Paolo
> 
> Paolo Bonzini (2):
>   KVM: allow setting a usercopy region in struct kvm_vcpu
>   KVM: fix KVM_XEN_HVM_CONFIG ioctl
> 
>  arch/x86/include/asm/kvm_host.h |  3 +++
>  arch/x86/kvm/svm.c              |  4 ++--
>  arch/x86/kvm/vmx.c              |  4 ++--
>  arch/x86/kvm/x86.c              | 17 ++++++++++++++---
>  include/linux/kvm_host.h        | 13 +++++++++++--
>  virt/kvm/kvm_main.c             | 13 ++++++++-----
>  6 files changed, 40 insertions(+), 14 deletions(-)
> 


-- 

Thanks,

David

Re: [PATCH 0/2] KVM: fixes for the kernel-hardening tree

[Christian Borntraeger]

On 10/23/2017 11:52 AM, David Hildenbrand wrote:
> On 21.10.2017 01:25, Paolo Bonzini wrote:
>> Two KVM ioctls (KVM_GET/SET_CPUID2) directly access the cpuid_entries
>> field of struct kvm_vcpu_arch.  Therefore, the new usercopy hardening
>> work in linux-next, which forbids copies from and to slab objects
>> unless they are from kmalloc or explicitly whitelisted, breaks KVM
>> completely.
>>
>> This series fixes it by adding the two new usercopy arguments
>> to kvm_init (more precisely to a new function kvm_init_usercopy,
>> while kvm_init passes zeroes as a default).
>>
>> There's also another broken ioctl, KVM_XEN_HVM_CONFIG, but it is
>> obsolete and not a big deal at all.
>>
>> I'm Ccing all submaintainers in case they have something similar
>> going on in their kvm_arch and kvm_vcpu_arch structs.  KVM has a
>> pretty complex userspace API, so thorough with linux-next is highly
>> recommended.
> 
> I assume on s390x, at least
> 
> kvm_arch_vcpu_ioctl_get_one_reg() and
> kvm_arch_vcpu_ioctl_set_one_reg()
> 
> have to be fixed.
> 
> Christian, are you already looking into this?

Not yet. I am in preparation for travel.

> 
>>
>> Many thanks to Thomas Gleixner for reporting this to me.
>>
>> Paolo
>>
>> Paolo Bonzini (2):
>>   KVM: allow setting a usercopy region in struct kvm_vcpu
>>   KVM: fix KVM_XEN_HVM_CONFIG ioctl
>>
>>  arch/x86/include/asm/kvm_host.h |  3 +++
>>  arch/x86/kvm/svm.c              |  4 ++--
>>  arch/x86/kvm/vmx.c              |  4 ++--
>>  arch/x86/kvm/x86.c              | 17 ++++++++++++++---
>>  include/linux/kvm_host.h        | 13 +++++++++++--
>>  virt/kvm/kvm_main.c             | 13 ++++++++-----
>>  6 files changed, 40 insertions(+), 14 deletions(-)
>>
> 
> 

Re: [PATCH 0/2] KVM: fixes for the kernel-hardening tree

[Cornelia Huck]

On Mon, 23 Oct 2017 11:52:51 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 21.10.2017 01:25, Paolo Bonzini wrote:
> > Two KVM ioctls (KVM_GET/SET_CPUID2) directly access the cpuid_entries
> > field of struct kvm_vcpu_arch.  Therefore, the new usercopy hardening
> > work in linux-next, which forbids copies from and to slab objects
> > unless they are from kmalloc or explicitly whitelisted, breaks KVM
> > completely.
> > 
> > This series fixes it by adding the two new usercopy arguments
> > to kvm_init (more precisely to a new function kvm_init_usercopy,
> > while kvm_init passes zeroes as a default).
> > 
> > There's also another broken ioctl, KVM_XEN_HVM_CONFIG, but it is
> > obsolete and not a big deal at all.
> > 
> > I'm Ccing all submaintainers in case they have something similar
> > going on in their kvm_arch and kvm_vcpu_arch structs.  KVM has a
> > pretty complex userspace API, so thorough with linux-next is highly
> > recommended.  
> 
> I assume on s390x, at least
> 
> kvm_arch_vcpu_ioctl_get_one_reg() and
> kvm_arch_vcpu_ioctl_set_one_reg()
> 
> have to be fixed.

At a glance, seems like it.

> 
> Christian, are you already looking into this?

I'm afraid I'm also busy with travel preparation/travel, so I'd be glad
for any takers.

> 
> > 
> > Many thanks to Thomas Gleixner for reporting this to me.
> > 
> > Paolo
> > 
> > Paolo Bonzini (2):
> >   KVM: allow setting a usercopy region in struct kvm_vcpu
> >   KVM: fix KVM_XEN_HVM_CONFIG ioctl
> > 
> >  arch/x86/include/asm/kvm_host.h |  3 +++
> >  arch/x86/kvm/svm.c              |  4 ++--
> >  arch/x86/kvm/vmx.c              |  4 ++--
> >  arch/x86/kvm/x86.c              | 17 ++++++++++++++---
> >  include/linux/kvm_host.h        | 13 +++++++++++--
> >  virt/kvm/kvm_main.c             | 13 ++++++++-----
> >  6 files changed, 40 insertions(+), 14 deletions(-)
> >   
> 
> 

Re: [PATCH 0/2] KVM: fixes for the kernel-hardening tree

[Paolo Bonzini]

On 23/10/2017 14:39, Cornelia Huck wrote:
> On Mon, 23 Oct 2017 11:52:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 21.10.2017 01:25, Paolo Bonzini wrote:
>>> Two KVM ioctls (KVM_GET/SET_CPUID2) directly access the cpuid_entries
>>> field of struct kvm_vcpu_arch.  Therefore, the new usercopy hardening
>>> work in linux-next, which forbids copies from and to slab objects
>>> unless they are from kmalloc or explicitly whitelisted, breaks KVM
>>> completely.
>>>
>>> This series fixes it by adding the two new usercopy arguments
>>> to kvm_init (more precisely to a new function kvm_init_usercopy,
>>> while kvm_init passes zeroes as a default).
>>>
>>> There's also another broken ioctl, KVM_XEN_HVM_CONFIG, but it is
>>> obsolete and not a big deal at all.
>>>
>>> I'm Ccing all submaintainers in case they have something similar
>>> going on in their kvm_arch and kvm_vcpu_arch structs.  KVM has a
>>> pretty complex userspace API, so thorough with linux-next is highly
>>> recommended.  
>>
>> I assume on s390x, at least
>>
>> kvm_arch_vcpu_ioctl_get_one_reg() and
>> kvm_arch_vcpu_ioctl_set_one_reg()
>>
>> have to be fixed.
> 
> At a glance, seems like it.
> 
>>
>> Christian, are you already looking into this?
> 
> I'm afraid I'm also busy with travel preparation/travel, so I'd be glad
> for any takers.

Let's do a generic fix now, so that we don't need to rush the switch to
explicit whitelisting.

Paolo

Re: [PATCH 0/2] KVM: fixes for the kernel-hardening tree

[David Hildenbrand]

On 23.10.2017 16:15, Paolo Bonzini wrote:
> On 23/10/2017 14:39, Cornelia Huck wrote:
>> On Mon, 23 Oct 2017 11:52:51 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 21.10.2017 01:25, Paolo Bonzini wrote:
>>>> Two KVM ioctls (KVM_GET/SET_CPUID2) directly access the cpuid_entries
>>>> field of struct kvm_vcpu_arch.  Therefore, the new usercopy hardening
>>>> work in linux-next, which forbids copies from and to slab objects
>>>> unless they are from kmalloc or explicitly whitelisted, breaks KVM
>>>> completely.
>>>>
>>>> This series fixes it by adding the two new usercopy arguments
>>>> to kvm_init (more precisely to a new function kvm_init_usercopy,
>>>> while kvm_init passes zeroes as a default).
>>>>
>>>> There's also another broken ioctl, KVM_XEN_HVM_CONFIG, but it is
>>>> obsolete and not a big deal at all.
>>>>
>>>> I'm Ccing all submaintainers in case they have something similar
>>>> going on in their kvm_arch and kvm_vcpu_arch structs.  KVM has a
>>>> pretty complex userspace API, so thorough with linux-next is highly
>>>> recommended.  
>>>
>>> I assume on s390x, at least
>>>
>>> kvm_arch_vcpu_ioctl_get_one_reg() and
>>> kvm_arch_vcpu_ioctl_set_one_reg()
>>>
>>> have to be fixed.
>>
>> At a glance, seems like it.
>>
>>>
>>> Christian, are you already looking into this?
>>
>> I'm afraid I'm also busy with travel preparation/travel, so I'd be glad
>> for any takers.
> 
> Let's do a generic fix now, so that we don't need to rush the switch to
> explicit whitelisting.

You mean a arch specific fix (allow writes/reads to arch) or even more
generic?

Otherwise I can you send a patch to fix these two functions.

> 
> Paolo
> 


-- 

Thanks,

David

Re: [PATCH 0/2] KVM: fixes for the kernel-hardening tree

[Christian Borntraeger]

On 10/25/2017 11:45 AM, David Hildenbrand wrote:
> On 23.10.2017 16:15, Paolo Bonzini wrote:
>> On 23/10/2017 14:39, Cornelia Huck wrote:
>>> On Mon, 23 Oct 2017 11:52:51 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> On 21.10.2017 01:25, Paolo Bonzini wrote:
>>>>> Two KVM ioctls (KVM_GET/SET_CPUID2) directly access the cpuid_entries
>>>>> field of struct kvm_vcpu_arch.  Therefore, the new usercopy hardening
>>>>> work in linux-next, which forbids copies from and to slab objects
>>>>> unless they are from kmalloc or explicitly whitelisted, breaks KVM
>>>>> completely.
>>>>>
>>>>> This series fixes it by adding the two new usercopy arguments
>>>>> to kvm_init (more precisely to a new function kvm_init_usercopy,
>>>>> while kvm_init passes zeroes as a default).
>>>>>
>>>>> There's also another broken ioctl, KVM_XEN_HVM_CONFIG, but it is
>>>>> obsolete and not a big deal at all.
>>>>>
>>>>> I'm Ccing all submaintainers in case they have something similar
>>>>> going on in their kvm_arch and kvm_vcpu_arch structs.  KVM has a
>>>>> pretty complex userspace API, so thorough with linux-next is highly
>>>>> recommended.  
>>>>
>>>> I assume on s390x, at least
>>>>
>>>> kvm_arch_vcpu_ioctl_get_one_reg() and
>>>> kvm_arch_vcpu_ioctl_set_one_reg()
>>>>
>>>> have to be fixed.
>>>
>>> At a glance, seems like it.
>>>
>>>>
>>>> Christian, are you already looking into this?
>>>
>>> I'm afraid I'm also busy with travel preparation/travel, so I'd be glad
>>> for any takers.
>>
>> Let's do a generic fix now, so that we don't need to rush the switch to
>> explicit whitelisting.
> 
> You mean a arch specific fix (allow writes/reads to arch) or even more
> generic?

Kees,

I am somewhat worried about these changes. The onereg interface is certainly
broken right now, but newer QEMUs will not use it. So its very likely that
testing will not find the regression. I would assume that usercopy hardinging
will introduce a lot of non-obvious regressions in seldomly used code. Have you
considered some debugging aids to find "now broken" things?
> 
> Otherwise I can you send a patch to fix these two functions.

Having said that, yes if you can send a fixup for 
kvm_arch_vcpu_ioctl_get/set_one_reg that would be great.

Christian