[PATCH 1/2] kvm:x86/svm: enable SVM lock if host supports it

[PATCH 1/2] kvm:x86/svm: enable SVM lock if host supports it

[prasadjoshi.linux]

From: Prasad Joshi <prasadjoshi.linux@gmail.com>

SVM lock features allows software from preventing update to EFER.SVME.
Enable the SVM lock in guest if it is supported on host machine.

Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com>
---
 arch/x86/kvm/svm.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index e1b1ce2..fcdfdea 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -4031,6 +4031,10 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
 		if (boot_cpu_has(X86_FEATURE_NRIPS))
 			entry->edx |= SVM_FEATURE_NRIP;
 
+		/* support SVM Lock if host supports it */
+		if (boot_cpu_has(X86_FEATURE_SVML))
+			entry->edx |= SVM_FEATURE_SVML;
+
 		/* Support NPT for the guest if enabled */
 		if (npt_enabled)
 			entry->edx |= SVM_FEATURE_NPT;
-- 
1.7.10.4

[PATCH 2/2] kvm/svm: emulate SVM_KEY MSR

[prasadjoshi.linux]

From: Prasad Joshi <prasadjoshi.linux@gmail.com>

Write only SVM_KEY can be used to create a password protected mechanism
to clear VM_CR.LOCK.

This key can only be set when VM_CR.LOCK is unset. Once this key is set,
a write to it compares the written value with already set key. The
VM_CR.LOCK is unset only if the key matches. All reads from this MSR
must return 0.

Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com>
---
 arch/x86/include/uapi/asm/msr-index.h |    1 +
 arch/x86/kvm/svm.c                    |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index 892ce40..eda8346 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -170,6 +170,7 @@
 
 #define MSR_AMD64_PATCH_LEVEL		0x0000008b
 #define MSR_AMD64_TSC_RATIO		0xc0000104
+#define MSR_AMD64_SVM_KEY		0xc0010118
 #define MSR_AMD64_NB_CFG		0xc001001f
 #define MSR_AMD64_PATCH_LOADER		0xc0010020
 #define MSR_AMD64_OSVW_ID_LENGTH	0xc0010140
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index fcdfdea..73b2dc2 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -150,6 +150,8 @@ struct vcpu_svm {
 
 	struct nested_state nested;
 
+	u64 svm_key; /* Write only SVM Key */
+
 	bool nmi_singlestep;
 
 	unsigned int3_injected;
@@ -3079,6 +3081,13 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
 	case MSR_VM_CR:
 		*data = svm->nested.vm_cr_msr;
 		break;
+	case MSR_AMD64_SVM_KEY:
+		/*
+		 * SVM Key is password protected mechanism to manage
+		 * VM_CR.LOCK and therefore reading it must always return 0.
+		 */
+		*data = 0;
+		break;
 	case MSR_IA32_UCODE_REV:
 		*data = 0x01000065;
 		break;
@@ -3132,6 +3141,26 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
 	return 0;
 }
 
+static int svm_set_svm_key(struct kvm_vcpu *vcpu, u64 data)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (!boot_cpu_has(X86_FEATURE_SVML))
+		return 1;
+
+	if (!(svm->nested.vm_cr_msr & SVM_VM_CR_SVM_LOCK_MASK) && data) {
+		/* vm_cr.lock not set: set the password key */
+		svm->svm_key = data;
+	} else {
+		/* vm_cr.lock is set */
+		if (data && svm->svm_key == data) {
+			/* password key matched: reset the lock */
+			svm->nested.vm_cr_msr &= ~SVM_VM_CR_SVM_LOCK_MASK;
+		}
+	}
+	return 0;
+}
+
 static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -3191,6 +3220,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		break;
 	case MSR_VM_CR:
 		return svm_set_vm_cr(vcpu, data);
+        case MSR_AMD64_SVM_KEY:
+		return svm_set_svm_key(vcpu, data);
 	case MSR_VM_IGNNE:
 		vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
 		break;
-- 
1.7.10.4

Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR

[Gleb Natapov]

On Sun, Apr 21, 2013 at 02:12:21PM +0530, prasadjoshi.linux@gmail.com wrote:
> From: Prasad Joshi <prasadjoshi.linux@gmail.com>
> 
> Write only SVM_KEY can be used to create a password protected mechanism
> to clear VM_CR.LOCK.
> 
> This key can only be set when VM_CR.LOCK is unset. Once this key is set,
> a write to it compares the written value with already set key. The
> VM_CR.LOCK is unset only if the key matches. All reads from this MSR
> must return 0.
> 
> Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com>
> ---
>  arch/x86/include/uapi/asm/msr-index.h |    1 +
>  arch/x86/kvm/svm.c                    |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
> index 892ce40..eda8346 100644
> --- a/arch/x86/include/uapi/asm/msr-index.h
> +++ b/arch/x86/include/uapi/asm/msr-index.h
> @@ -170,6 +170,7 @@
>  
>  #define MSR_AMD64_PATCH_LEVEL		0x0000008b
>  #define MSR_AMD64_TSC_RATIO		0xc0000104
> +#define MSR_AMD64_SVM_KEY		0xc0010118
>  #define MSR_AMD64_NB_CFG		0xc001001f
>  #define MSR_AMD64_PATCH_LOADER		0xc0010020
>  #define MSR_AMD64_OSVW_ID_LENGTH	0xc0010140
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index fcdfdea..73b2dc2 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -150,6 +150,8 @@ struct vcpu_svm {
>  
>  	struct nested_state nested;
>  
> +	u64 svm_key; /* Write only SVM Key */
> +
>  	bool nmi_singlestep;
>  
>  	unsigned int3_injected;
> @@ -3079,6 +3081,13 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
>  	case MSR_VM_CR:
>  		*data = svm->nested.vm_cr_msr;
>  		break;
> +	case MSR_AMD64_SVM_KEY:
> +		/*
> +		 * SVM Key is password protected mechanism to manage
> +		 * VM_CR.LOCK and therefore reading it must always return 0.
> +		 */
> +		*data = 0;
> +		break;
>  	case MSR_IA32_UCODE_REV:
>  		*data = 0x01000065;
>  		break;
> @@ -3132,6 +3141,26 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
>  	return 0;
>  }
>  
> +static int svm_set_svm_key(struct kvm_vcpu *vcpu, u64 data)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	if (!boot_cpu_has(X86_FEATURE_SVML))
> +		return 1;
> +
Since the feature is emulated why do you care if host cpu supports it?

> +	if (!(svm->nested.vm_cr_msr & SVM_VM_CR_SVM_LOCK_MASK) && data) {
> +		/* vm_cr.lock not set: set the password key */
> +		svm->svm_key = data;
> +	} else {
> +		/* vm_cr.lock is set */
> +		if (data && svm->svm_key == data) {
> +			/* password key matched: reset the lock */
> +			svm->nested.vm_cr_msr &= ~SVM_VM_CR_SVM_LOCK_MASK;
> +		}
> +	}
That's not enough. First of all KVM does not currently checks
SVMDIS before allowing setting of EFER_SVME. Second svm_set_vm_cr() erroneously
ignores writes into VM_CR SVMDIS/LOCK if SVMDIS is set, while it should
check LOCK. Third when write is done by a userspace during reset the
key check should be ignored.


> +	return 0;
> +}
> +
>  static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3191,6 +3220,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  		break;
>  	case MSR_VM_CR:
>  		return svm_set_vm_cr(vcpu, data);
> +        case MSR_AMD64_SVM_KEY:
> +		return svm_set_svm_key(vcpu, data);
>  	case MSR_VM_IGNNE:
>  		vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
>  		break;
> -- 
> 1.7.10.4

--
			Gleb.

Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR

[Prasad Joshi]

On Sun, Apr 21, 2013 at 2:49 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Sun, Apr 21, 2013 at 02:12:21PM +0530, prasadjoshi.linux@gmail.com wrote:
>> From: Prasad Joshi <prasadjoshi.linux@gmail.com>
>>
>> Write only SVM_KEY can be used to create a password protected mechanism
>> to clear VM_CR.LOCK.
>>
>> This key can only be set when VM_CR.LOCK is unset. Once this key is set,
>> a write to it compares the written value with already set key. The
>> VM_CR.LOCK is unset only if the key matches. All reads from this MSR
>> must return 0.
>>
>> Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com>
>> ---
>>  arch/x86/include/uapi/asm/msr-index.h |    1 +
>>  arch/x86/kvm/svm.c                    |   31 +++++++++++++++++++++++++++++++
>>  2 files changed, 32 insertions(+)
>>
>> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
>> index 892ce40..eda8346 100644
>> --- a/arch/x86/include/uapi/asm/msr-index.h
>> +++ b/arch/x86/include/uapi/asm/msr-index.h
>> @@ -170,6 +170,7 @@
>>
>>  #define MSR_AMD64_PATCH_LEVEL                0x0000008b
>>  #define MSR_AMD64_TSC_RATIO          0xc0000104
>> +#define MSR_AMD64_SVM_KEY            0xc0010118
>>  #define MSR_AMD64_NB_CFG             0xc001001f
>>  #define MSR_AMD64_PATCH_LOADER               0xc0010020
>>  #define MSR_AMD64_OSVW_ID_LENGTH     0xc0010140
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index fcdfdea..73b2dc2 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -150,6 +150,8 @@ struct vcpu_svm {
>>
>>       struct nested_state nested;
>>
>> +     u64 svm_key; /* Write only SVM Key */
>> +
>>       bool nmi_singlestep;
>>
>>       unsigned int3_injected;
>> @@ -3079,6 +3081,13 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
>>       case MSR_VM_CR:
>>               *data = svm->nested.vm_cr_msr;
>>               break;
>> +     case MSR_AMD64_SVM_KEY:
>> +             /*
>> +              * SVM Key is password protected mechanism to manage
>> +              * VM_CR.LOCK and therefore reading it must always return 0.
>> +              */
>> +             *data = 0;
>> +             break;
>>       case MSR_IA32_UCODE_REV:
>>               *data = 0x01000065;
>>               break;
>> @@ -3132,6 +3141,26 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
>>       return 0;
>>  }
>>
>> +static int svm_set_svm_key(struct kvm_vcpu *vcpu, u64 data)
>> +{
>> +     struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> +     if (!boot_cpu_has(X86_FEATURE_SVML))
>> +             return 1;
>> +
> Since the feature is emulated why do you care if host cpu supports it?

Thanks Gleb. I changed this to checking SVML feature on the guest CPU.

Something like this should work

        if (!(cpuid_edx(SVM_CPUID_FUNC) & SVM_FEATURE_SVML))
                return 1;

>
>> +     if (!(svm->nested.vm_cr_msr & SVM_VM_CR_SVM_LOCK_MASK) && data) {
>> +             /* vm_cr.lock not set: set the password key */
>> +             svm->svm_key = data;
>> +     } else {
>> +             /* vm_cr.lock is set */
>> +             if (data && svm->svm_key == data) {
>> +                     /* password key matched: reset the lock */
>> +                     svm->nested.vm_cr_msr &= ~SVM_VM_CR_SVM_LOCK_MASK;
>> +             }
>> +     }
> That's not enough. First of all KVM does not currently checks
> SVMDIS before allowing setting of EFER_SVME. Second svm_set_vm_cr() erroneously
> ignores writes into VM_CR SVMDIS/LOCK if SVMDIS is set, while it should
> check LOCK.

I was aware of these changes (BUGs), however thought they must go in
another patch.
Any ways, I will include them in the next patch series.

> Third when write is done by a userspace during reset the
> key check should be ignored.

I missed this part. I think, CPU reset must set svm_key to 0 and reset
the VM_CR.LOCK. Both of these can be part of init_vmcb() which is
called during CPU reset. It should ensure that the no key is
registered during CPU reset.

Thanks.

-- Prasad

Re: [PATCH 2/2] kvm/svm: emulate SVM_KEY MSR

[Gleb Natapov]

On Mon, Apr 22, 2013 at 09:24:12AM +0530, Prasad Joshi wrote:
> On Sun, Apr 21, 2013 at 2:49 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Sun, Apr 21, 2013 at 02:12:21PM +0530, prasadjoshi.linux@gmail.com wrote:
> >> From: Prasad Joshi <prasadjoshi.linux@gmail.com>
> >>
> >> Write only SVM_KEY can be used to create a password protected mechanism
> >> to clear VM_CR.LOCK.
> >>
> >> This key can only be set when VM_CR.LOCK is unset. Once this key is set,
> >> a write to it compares the written value with already set key. The
> >> VM_CR.LOCK is unset only if the key matches. All reads from this MSR
> >> must return 0.
> >>
> >> Signed-off-by: Prasad Joshi <prasadjoshi.linux@gmail.com>
> >> ---
> >>  arch/x86/include/uapi/asm/msr-index.h |    1 +
> >>  arch/x86/kvm/svm.c                    |   31 +++++++++++++++++++++++++++++++
> >>  2 files changed, 32 insertions(+)
> >>
> >> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
> >> index 892ce40..eda8346 100644
> >> --- a/arch/x86/include/uapi/asm/msr-index.h
> >> +++ b/arch/x86/include/uapi/asm/msr-index.h
> >> @@ -170,6 +170,7 @@
> >>
> >>  #define MSR_AMD64_PATCH_LEVEL                0x0000008b
> >>  #define MSR_AMD64_TSC_RATIO          0xc0000104
> >> +#define MSR_AMD64_SVM_KEY            0xc0010118
> >>  #define MSR_AMD64_NB_CFG             0xc001001f
> >>  #define MSR_AMD64_PATCH_LOADER               0xc0010020
> >>  #define MSR_AMD64_OSVW_ID_LENGTH     0xc0010140
> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >> index fcdfdea..73b2dc2 100644
> >> --- a/arch/x86/kvm/svm.c
> >> +++ b/arch/x86/kvm/svm.c
> >> @@ -150,6 +150,8 @@ struct vcpu_svm {
> >>
> >>       struct nested_state nested;
> >>
> >> +     u64 svm_key; /* Write only SVM Key */
> >> +
> >>       bool nmi_singlestep;
> >>
> >>       unsigned int3_injected;
> >> @@ -3079,6 +3081,13 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
> >>       case MSR_VM_CR:
> >>               *data = svm->nested.vm_cr_msr;
> >>               break;
> >> +     case MSR_AMD64_SVM_KEY:
> >> +             /*
> >> +              * SVM Key is password protected mechanism to manage
> >> +              * VM_CR.LOCK and therefore reading it must always return 0.
> >> +              */
> >> +             *data = 0;
> >> +             break;
> >>       case MSR_IA32_UCODE_REV:
> >>               *data = 0x01000065;
> >>               break;
> >> @@ -3132,6 +3141,26 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
> >>       return 0;
> >>  }
> >>
> >> +static int svm_set_svm_key(struct kvm_vcpu *vcpu, u64 data)
> >> +{
> >> +     struct vcpu_svm *svm = to_svm(vcpu);
> >> +
> >> +     if (!boot_cpu_has(X86_FEATURE_SVML))
> >> +             return 1;
> >> +
> > Since the feature is emulated why do you care if host cpu supports it?
> 
> Thanks Gleb. I changed this to checking SVML feature on the guest CPU.
> 
> Something like this should work
> 
>         if (!(cpuid_edx(SVM_CPUID_FUNC) & SVM_FEATURE_SVML))
>                 return 1;
> 
This still checks host cpu. You need to use kvm_find_cpuid_entry().

> >
> >> +     if (!(svm->nested.vm_cr_msr & SVM_VM_CR_SVM_LOCK_MASK) && data) {
> >> +             /* vm_cr.lock not set: set the password key */
> >> +             svm->svm_key = data;
> >> +     } else {
> >> +             /* vm_cr.lock is set */
> >> +             if (data && svm->svm_key == data) {
> >> +                     /* password key matched: reset the lock */
> >> +                     svm->nested.vm_cr_msr &= ~SVM_VM_CR_SVM_LOCK_MASK;
> >> +             }
> >> +     }
> > That's not enough. First of all KVM does not currently checks
> > SVMDIS before allowing setting of EFER_SVME. Second svm_set_vm_cr() erroneously
> > ignores writes into VM_CR SVMDIS/LOCK if SVMDIS is set, while it should
> > check LOCK.
> 
> I was aware of these changes (BUGs), however thought they must go in
> another patch.
It should go to another patch, but send them together with the series
that implement LOCK feature. Without the fixes LOCK feature does not
work.

> Any ways, I will include them in the next patch series.
> 
Thanks.

> > Third when write is done by a userspace during reset the
> > key check should be ignored.
> 
> I missed this part. I think, CPU reset must set svm_key to 0 and reset
> the VM_CR.LOCK. Both of these can be part of init_vmcb() which is
> called during CPU reset. It should ensure that the no key is
> registered during CPU reset.
init_vmcb() is called as part of kvm_vcpu_reset(). kvm_vcpu_reset() is
called during vcpu creation and when vcpu receives INIT signal (if
irqchip is in kernel). INIT signal does not cause LOCK to be unset,
otherwise it would have been pointless. Power-up reset is emulated by
userspace configuring vcpu with reset state, for that userspase should
be able to set any value there.

--
			Gleb.