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

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

[Paolo Bonzini]

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

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

[Paolo Bonzini]

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

[PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Paolo Bonzini]

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

[kernel-hardening] [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Paolo Bonzini]

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

Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Cornelia Huck]

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>

[kernel-hardening] Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Cornelia Huck]

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>

Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Christoffer Dall]

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>

[kernel-hardening] Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Christoffer Dall]

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>

Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Marc Zyngier]

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.

[kernel-hardening] Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Marc Zyngier]

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.

Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Christian Borntraeger]

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;
> 

[kernel-hardening] Re: [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Christian Borntraeger]

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;
> 

Re: [kernel-hardening] [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Eric Biggers]

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

Re: [kernel-hardening] [PATCH 1/2] kvm: whitelist struct kvm_vcpu_arch

[Kees Cook]

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

[PATCH 2/2] kvm: x86: 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 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

[kernel-hardening] [PATCH 2/2] kvm: x86: 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 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

Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

[Kees Cook]

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

[kernel-hardening] Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

[Kees Cook]

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

Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

[Kees Cook]

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

[kernel-hardening] Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

[Kees Cook]

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

Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

[Paolo Bonzini]

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

[kernel-hardening] Re: [PATCH 2/2] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl

[Paolo Bonzini]

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

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

[Paul Mackerras]

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.

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

[Paul Mackerras]

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.