* [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree
@ 2017-10-26 13:45 Paolo Bonzini
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-10-26 13:45 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Christoffer Dall, Marc Zyngier, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, kernel-hardening,
Kees Cook, Radim Krčmář
Four KVM ioctls (KVM_GET/SET_CPUID2 on x86, KVM_GET/SET_ONE_REG on
ARM and s390) directly access the kvm_vcpu_arch struct. 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 on those architectures.
The kvm_vcpu_arch struct is embedded in the kvm_vcpu struct and the
corresponding slab cache is allocated by architecture-independent code.
It is enough, for simplicity, to whitelist the whole sub-struct and
only touch one place of the KVM code. Later, any further restrictions
can be applied in the KVM tree.
Paolo Bonzini (2):
kvm: whitelist struct kvm_vcpu_arch
kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl
arch/x86/kvm/x86.c | 7 ++++---
virt/kvm/kvm_main.c | 8 ++++++--
2 files changed, 10 insertions(+), 5 deletions(-)
--
2.14.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch
2017-10-26 13:45 [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree Paolo Bonzini
@ 2017-10-26 13:45 ` Paolo Bonzini
2017-10-26 14:13 ` Cornelia Huck
` (4 more replies)
2017-10-26 13:45 ` [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl Paolo Bonzini
2017-10-27 5:25 ` [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree Paul Mackerras
2 siblings, 5 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-10-26 13:45 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Christoffer Dall, Marc Zyngier, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, kernel-hardening,
Kees Cook, Christian Borntraeger, Christoffer Dall,
Radim Krčmář
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl
2017-10-26 13:45 [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree Paolo Bonzini
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
@ 2017-10-26 13:45 ` Paolo Bonzini
2017-10-26 13:48 ` Kees Cook
2017-11-30 20:40 ` Kees Cook
2017-10-27 5:25 ` [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree Paul Mackerras
2 siblings, 2 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-10-26 13:45 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Christoffer Dall, Marc Zyngier, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, kernel-hardening,
Kees Cook, Radim Krčmář
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl
2017-10-26 13:45 ` [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl Paolo Bonzini
@ 2017-10-26 13:48 ` Kees Cook
2017-11-30 20:40 ` Kees Cook
1 sibling, 0 replies; 13+ messages in thread
From: Kees Cook @ 2017-10-26 13:48 UTC (permalink / raw)
To: Paolo Bonzini
Cc: LKML, KVM, Christoffer Dall, Marc Zyngier, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, kernel-hardening,
Radim Krčmář
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
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
@ 2017-10-26 14:13 ` Cornelia Huck
2017-10-26 14:21 ` Christoffer Dall
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Cornelia Huck @ 2017-10-26 14:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, kvm, Christoffer Dall, Marc Zyngier,
Christian Borntraeger, James Hogan, Paul Mackerras,
kernel-hardening, Kees Cook, Christoffer Dall,
Radim Krčmář
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
2017-10-26 14:13 ` Cornelia Huck
@ 2017-10-26 14:21 ` Christoffer Dall
2017-10-26 14:34 ` Marc Zyngier
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Christoffer Dall @ 2017-10-26 14:21 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, kvm, Marc Zyngier, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, kernel-hardening,
Kees Cook, Christian Borntraeger, Radim Krčmář
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
2017-10-26 14:13 ` Cornelia Huck
2017-10-26 14:21 ` Christoffer Dall
@ 2017-10-26 14:34 ` Marc Zyngier
2017-10-26 14:51 ` Christian Borntraeger
2017-10-30 23:28 ` [kernel-hardening] " Eric Biggers
4 siblings, 0 replies; 13+ messages in thread
From: Marc Zyngier @ 2017-10-26 14:34 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, kvm, Christoffer Dall, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, kernel-hardening,
Kees Cook, Christian Borntraeger, Christoffer Dall,
Radim Krčmář
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.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
` (2 preceding siblings ...)
2017-10-26 14:34 ` Marc Zyngier
@ 2017-10-26 14:51 ` Christian Borntraeger
2017-10-30 23:28 ` [kernel-hardening] " Eric Biggers
4 siblings, 0 replies; 13+ messages in thread
From: Christian Borntraeger @ 2017-10-26 14:51 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel, kvm
Cc: Christoffer Dall, Marc Zyngier, Cornelia Huck, James Hogan,
Paul Mackerras, kernel-hardening, Kees Cook,
Christian Borntraeger, Christoffer Dall,
Radim Krč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;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree
2017-10-26 13:45 [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree Paolo Bonzini
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
2017-10-26 13:45 ` [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl Paolo Bonzini
@ 2017-10-27 5:25 ` Paul Mackerras
2 siblings, 0 replies; 13+ messages in thread
From: Paul Mackerras @ 2017-10-27 5:25 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, kvm, Christoffer Dall, Marc Zyngier,
Christian Borntraeger, Cornelia Huck, James Hogan,
kernel-hardening, Kees Cook, Radim Krčmář
On Thu, Oct 26, 2017 at 03:45:45PM +0200, Paolo Bonzini wrote:
> Four KVM ioctls (KVM_GET/SET_CPUID2 on x86, KVM_GET/SET_ONE_REG on
> ARM and s390) directly access the kvm_vcpu_arch struct. 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 on those architectures.
>
> The kvm_vcpu_arch struct is embedded in the kvm_vcpu struct and the
> corresponding slab cache is allocated by architecture-independent code.
> It is enough, for simplicity, to whitelist the whole sub-struct and
> only touch one place of the KVM code. Later, any further restrictions
> can be applied in the KVM tree.
I checked arch/powerpc/kvm, and all the copy_to/from_user calls are
accessing the stack or memory allocated with kzalloc or kvzalloc, so
if I understand correctly, we should be OK there.
Paul.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kernel-hardening] [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
` (3 preceding siblings ...)
2017-10-26 14:51 ` Christian Borntraeger
@ 2017-10-30 23:28 ` Eric Biggers
2017-10-30 23:51 ` Kees Cook
4 siblings, 1 reply; 13+ messages in thread
From: Eric Biggers @ 2017-10-30 23:28 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, kvm, Christoffer Dall, Marc Zyngier,
Christian Borntraeger, Cornelia Huck, James Hogan,
Paul Mackerras, kernel-hardening, Kees Cook,
Christian Borntraeger, Christoffer Dall,
Radim Krčmář
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [kernel-hardening] [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch
2017-10-30 23:28 ` [kernel-hardening] " Eric Biggers
@ 2017-10-30 23:51 ` Kees Cook
0 siblings, 0 replies; 13+ messages in thread
From: Kees Cook @ 2017-10-30 23:51 UTC (permalink / raw)
To: Eric Biggers
Cc: Paolo Bonzini, LKML, KVM, Christoffer Dall, Marc Zyngier,
Christian Borntraeger, Cornelia Huck, James Hogan,
Paul Mackerras, kernel-hardening, Christian Borntraeger,
Christoffer Dall, Radim Krčmář
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
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl
2017-10-26 13:45 ` [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl Paolo Bonzini
2017-10-26 13:48 ` Kees Cook
@ 2017-11-30 20:40 ` Kees Cook
2017-12-01 8:29 ` Paolo Bonzini
1 sibling, 1 reply; 13+ messages in thread
From: Kees Cook @ 2017-11-30 20:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: LKML, KVM, Christoffer Dall, Marc Zyngier, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, kernel-hardening,
Radim Krčmář
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
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl
2017-11-30 20:40 ` Kees Cook
@ 2017-12-01 8:29 ` Paolo Bonzini
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-12-01 8:29 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, KVM, Christoffer Dall, Marc Zyngier, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, kernel-hardening,
Radim Krčmář
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-12-01 8:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-26 13:45 [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree Paolo Bonzini
2017-10-26 13:45 ` [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch Paolo Bonzini
2017-10-26 14:13 ` Cornelia Huck
2017-10-26 14:21 ` Christoffer Dall
2017-10-26 14:34 ` Marc Zyngier
2017-10-26 14:51 ` Christian Borntraeger
2017-10-30 23:28 ` [kernel-hardening] " Eric Biggers
2017-10-30 23:51 ` Kees Cook
2017-10-26 13:45 ` [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl Paolo Bonzini
2017-10-26 13:48 ` Kees Cook
2017-11-30 20:40 ` Kees Cook
2017-12-01 8:29 ` Paolo Bonzini
2017-10-27 5:25 ` [PATCH v2 0/2] KVM: fixes for the kernel-hardening tree Paul Mackerras
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).