2019-12-19 15:26:14

by John Allen

Subject: [PATCH] kvm/svm: PKU not currently supported

Current SVM implementation does not have support for handling PKU. Guests
running on a host with future AMD cpus that support the feature will read
garbage from the PKRU register and will hit segmentation faults on boot as
memory is getting marked as protected that should not be. Ensure that cpuid
from SVM does not advertise the feature.

Signed-off-by: John Allen <[email protected]>
---
arch/x86/kvm/svm.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 122d4ce3b1ab..f911aa1b41c8 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5933,6 +5933,8 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
if (avic)
entry->ecx &= ~bit(X86_FEATURE_X2APIC);
break;
+ case 0x7:
+ entry->ecx &= ~bit(X86_FEATURE_PKU);
case 0x80000001:
if (nested)
entry->ecx |= (1 << 2); /* Set SVM bit */
--
2.24.0

2019-12-19 19:11:51

by Vitaly Kuznetsov

Subject: Re: [PATCH] kvm/svm: PKU not currently supported

John Allen <[email protected]> writes:

> Current SVM implementation does not have support for handling PKU. Guests
> running on a host with future AMD cpus that support the feature will read
> garbage from the PKRU register and will hit segmentation faults on boot as
> memory is getting marked as protected that should not be. Ensure that cpuid
> from SVM does not advertise the feature.
>
> Signed-off-by: John Allen <[email protected]>
> ---
> arch/x86/kvm/svm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 122d4ce3b1ab..f911aa1b41c8 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5933,6 +5933,8 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> if (avic)
> entry->ecx &= ~bit(X86_FEATURE_X2APIC);
> break;
> + case 0x7:
> + entry->ecx &= ~bit(X86_FEATURE_PKU);

Would it make more sense to introduce kvm_x86_ops->pku_supported() (and
return false for SVM and boot_cpu_has(X86_FEATURE_PKU) for vmx) so we
don't set the bit in the first place?

> case 0x80000001:
> if (nested)
> entry->ecx |= (1 << 2); /* Set SVM bit */

--
Vitaly

2019-12-19 19:51:08

by John Allen

Subject: Re: [PATCH] kvm/svm: PKU not currently supported

On Thu, Dec 19, 2019 at 08:09:57PM +0100, Vitaly Kuznetsov wrote:
> John Allen <[email protected]> writes:
>
> > Current SVM implementation does not have support for handling PKU. Guests
> > running on a host with future AMD cpus that support the feature will read
> > garbage from the PKRU register and will hit segmentation faults on boot as
> > memory is getting marked as protected that should not be. Ensure that cpuid
> > from SVM does not advertise the feature.
> >
> > Signed-off-by: John Allen <[email protected]>
> > ---
> > arch/x86/kvm/svm.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 122d4ce3b1ab..f911aa1b41c8 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -5933,6 +5933,8 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> > if (avic)
> > entry->ecx &= ~bit(X86_FEATURE_X2APIC);
> > break;
> > + case 0x7:
> > + entry->ecx &= ~bit(X86_FEATURE_PKU);
>
> Would it make more sense to introduce kvm_x86_ops->pku_supported() (and
> return false for SVM and boot_cpu_has(X86_FEATURE_PKU) for vmx) so we
> don't set the bit in the first place?

Yes, I think you're right. I had initially planned to do it that way so I
already have a patch ready. I'll send it up pronto.

>
> > case 0x80000001:
> > if (nested)
> > entry->ecx |= (1 << 2); /* Set SVM bit */
>
> --
> Vitaly
>