linux-kernel - Re: [RFC PATCH 4/4] KVM: vmx: add support for emulating UMIP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Jul 2016 14:36:22 +0200
From:	Radim Krčmář <rkrcmar@...hat.com>
To:	Paolo Bonzini <pbonzini@...hat.com>
Cc:	linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [RFC PATCH 4/4] KVM: vmx: add support for emulating UMIP

2016-07-14 10:09+0200, Paolo Bonzini:
> On 13/07/2016 22:30, Radim Krčmář wrote:
>> 2016-07-12 21:20+0200, Paolo Bonzini:
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> @@ -3967,6 +3968,14 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
>>>  		(to_vmx(vcpu)->rmode.vm86_active ?
>>>  		 KVM_RMODE_VM_CR4_ALWAYS_ON : KVM_PMODE_VM_CR4_ALWAYS_ON);
>>>  
>>> +	if ((cr4 & X86_CR4_UMIP) && !boot_cpu_has(X86_FEATURE_UMIP)) {
>>> +		vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
>>> +			      SECONDARY_EXEC_DESC);
>> 
>> If UMIP support is not exposed in CPUID, we ought to #GP(0), because it
>> is a write to reserved bits.  It could also mean that the vm control is
>> not supported.
> 
> Yes, this is done in kvm_set_cr4:
> 
>         if (cr4 & CR4_RESERVED_BITS)
>                 return 1;
> 	...
>         if (!guest_cpuid_has_umip(vcpu) && (cr4 & X86_CR4_UMIP))
>                 return 1;

Missed that,

>> And we could then return true in vmx_umip_emulated() when
>> boot_cpu_has(X86_FEATURE_UMIP).
>> (Just for self-documentation, because occurrence of X86_FEATURE_UMIP is
>>  most likely a subset of SECONDARY_EXEC_DESC.)
> 
> This is not necessary because this is how KVM computes
> CPUID[EAX=7,EBX=0].ECX:
> 
>         unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0;
> 	...
>         const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | F(UMIP);

and that too, I'm really sorry for the review.

> 	...
> 	// Mask userspace-provided value against supported features
> 	entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
> 	// Mask userspace-provided value against host features
> 	cpuid_mask(&entry->ecx, CPUID_7_ECX);
> 	// Finally add emulated features
> 	entry->ecx |= f_umip;