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

[Gleb Natapov <gleb@redhat.com>]

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.