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

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]
Message-ID: <3b8953c9-0611-27da-f955-c79a6fcef9ce@amd.com>
Date:   Fri, 1 Oct 2021 12:06:19 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Sean Christopherson <seanjc@...gle.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Brijesh Singh <brijesh.singh@....com>,
        Ashish Kalra <Ashish.Kalra@....com>,
        linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with
 protected state

On 5/25/21 1:25 AM, Vitaly Kuznetsov wrote:
> Tom Lendacky <thomas.lendacky@....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@...gle.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@....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@...hat.com>
> 
> Thanks!

Paolo,

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

Thanks,
Tom

>