On Sun, Apr 21, 2013 at 2:49 PM, Gleb Natapov <gleb@xxxxxxxxxx> wrote:
> On Sun, Apr 21, 2013 at 02:12:21PM +0530, prasadjoshi.linux@xxxxxxxxx wrote:
>> From: Prasad Joshi <prasadjoshi.linux@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
