Re: [PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[Tom Lendacky <thomas.lendacky@amd.com>]

On 5/25/21 1:25 AM, Vitaly Kuznetsov wrote:
> Tom Lendacky <thomas.lendacky@amd.com> writes:
> 
>> When processing a hypercall for a guest with protected state, currently
>> SEV-ES guests, the guest CS segment register can't be checked to
>> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is
>> expected that communication between the guest and the hypervisor is
>> performed to shared memory using the GHCB. In order to use the GHCB, the
>> guest must have been in long mode, otherwise writes by the guest to the
>> GHCB would be encrypted and not be able to be comprehended by the
>> hypervisor.
>>
>> Create a new helper function, is_64_bit_hypercall(), that assumes the
>> guest is in 64-bit mode when the guest has protected state, and returns
>> true, otherwise invoking is_64_bit_mode() to determine the mode. Update
>> the hypercall related routines to use is_64_bit_hypercall() instead of
>> is_64_bit_mode().
>>
>> Add a WARN_ON_ONCE() to is_64_bit_mode() to catch occurences of calls to
>> this helper function for a guest running with protected state.
>>
>> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
>> Reported-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>
>> Changes since v1:
>> - Create a new helper routine, is_64_bit_hypercall(), and use it in place
>>    of is_64_bit_mode() in hypercall related areas.
>> - Add a WARN_ON_ONCE() to is_64_bit_mode() to issue a warning if invoked
>>    for a guest with protected state.
>> ---
>>   arch/x86/kvm/hyperv.c |  4 ++--
>>   arch/x86/kvm/x86.c    |  2 +-
>>   arch/x86/kvm/x86.h    | 12 ++++++++++++
>>   arch/x86/kvm/xen.c    |  2 +-
>>   4 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index f98370a39936..1cdf2b213f41 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1818,7 +1818,7 @@ static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
>>   {
>>   	bool longmode;
>>   
>> -	longmode = is_64_bit_mode(vcpu);
>> +	longmode = is_64_bit_hypercall(vcpu);
>>   	if (longmode)
>>   		kvm_rax_write(vcpu, result);
>>   	else {
>> @@ -1895,7 +1895,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>>   	}
>>   
>>   #ifdef CONFIG_X86_64
>> -	if (is_64_bit_mode(vcpu)) {
>> +	if (is_64_bit_hypercall(vcpu)) {
>>   		param = kvm_rcx_read(vcpu);
>>   		ingpa = kvm_rdx_read(vcpu);
>>   		outgpa = kvm_r8_read(vcpu);
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 9b6bca616929..dc72f0a1609a 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8403,7 +8403,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>   
>>   	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>>   
>> -	op_64_bit = is_64_bit_mode(vcpu);
>> +	op_64_bit = is_64_bit_hypercall(vcpu);
>>   	if (!op_64_bit) {
>>   		nr &= 0xFFFFFFFF;
>>   		a0 &= 0xFFFFFFFF;
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index 521f74e5bbf2..3102caf689d2 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -151,12 +151,24 @@ static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
>>   {
>>   	int cs_db, cs_l;
>>   
>> +	WARN_ON_ONCE(vcpu->arch.guest_state_protected);
>> +
>>   	if (!is_long_mode(vcpu))
>>   		return false;
>>   	static_call(kvm_x86_get_cs_db_l_bits)(vcpu, &cs_db, &cs_l);
>>   	return cs_l;
>>   }
>>   
>> +static inline bool is_64_bit_hypercall(struct kvm_vcpu *vcpu)
>> +{
>> +	/*
>> +	 * If running with protected guest state, the CS register is not
>> +	 * accessible. The hypercall register values will have had to been
>> +	 * provided in 64-bit mode, so assume the guest is in 64-bit.
>> +	 */
>> +	return vcpu->arch.guest_state_protected || is_64_bit_mode(vcpu);
>> +}
>> +
>>   static inline bool is_la57_mode(struct kvm_vcpu *vcpu)
>>   {
>>   #ifdef CONFIG_X86_64
>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>> index ae17250e1efe..c58f6369e668 100644
>> --- a/arch/x86/kvm/xen.c
>> +++ b/arch/x86/kvm/xen.c
>> @@ -680,7 +680,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>>   	    kvm_hv_hypercall_enabled(vcpu))
>>   		return kvm_hv_hypercall(vcpu);
>>   
>> -	longmode = is_64_bit_mode(vcpu);
>> +	longmode = is_64_bit_hypercall(vcpu);
>>   	if (!longmode) {
>>   		params[0] = (u32)kvm_rbx_read(vcpu);
>>   		params[1] = (u32)kvm_rcx_read(vcpu);
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> Thanks!

Paolo,

This got lost in my stack of work... any comments?

Thanks,
Tom

>