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

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

On 10/1/21 12:06 PM, Tom Lendacky wrote:
> 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

Ping

Thanks,
Tom

> 
>>