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

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

[Tom Lendacky]

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. Given that, assume that the guest is in 64-bit mode when
processing a hypercall from a guest 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>
---
 arch/x86/kvm/x86.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9b6bca616929..e715c69bb882 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 
 	trace_kvm_hypercall(nr, a0, a1, a2, a3);
 
-	op_64_bit = is_64_bit_mode(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.
+	 */
+	op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected;
 	if (!op_64_bit) {
 		nr &= 0xFFFFFFFF;
 		a0 &= 0xFFFFFFFF;
-- 
2.31.0

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

[Tom Lendacky]

On 5/22/21 11:43 AM, Tom Lendacky wrote:
> 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. Given that, assume that the guest is in 64-bit mode when
> processing a hypercall from a guest 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>
> ---
>  arch/x86/kvm/x86.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 

Subject should actually be KVM: x86: ..., not KVM: SVM:

Sorry about that.

Tom

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9b6bca616929..e715c69bb882 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  
>  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>  
> -	op_64_bit = is_64_bit_mode(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.
> +	 */
> +	op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected;
>  	if (!op_64_bit) {
>  		nr &= 0xFFFFFFFF;
>  		a0 &= 0xFFFFFFFF;
> 

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

[Vitaly Kuznetsov]

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. Given that, assume that the guest is in 64-bit mode when
> processing a hypercall from a guest 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>
> ---
>  arch/x86/kvm/x86.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9b6bca616929..e715c69bb882 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  
>  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>  
> -	op_64_bit = is_64_bit_mode(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.
> +	 */
> +	op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected;
>  	if (!op_64_bit) {
>  		nr &= 0xFFFFFFFF;
>  		a0 &= 0xFFFFFFFF;

While this is might be a very theoretical question, what about other
is_64_bit_mode() users? Namely, a very similar to the above check exists
in kvm_hv_hypercall() and kvm_xen_hypercall().

-- 
Vitaly

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

[Tom Lendacky]

On 5/24/21 6:53 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. Given that, assume that the guest is in 64-bit mode when
>> processing a hypercall from a guest 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>
>> ---
>>  arch/x86/kvm/x86.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 9b6bca616929..e715c69bb882 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>  
>>  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>>  
>> -	op_64_bit = is_64_bit_mode(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.
>> +	 */
>> +	op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected;
>>  	if (!op_64_bit) {
>>  		nr &= 0xFFFFFFFF;
>>  		a0 &= 0xFFFFFFFF;
> 
> While this is might be a very theoretical question, what about other
> is_64_bit_mode() users? Namely, a very similar to the above check exists
> in kvm_hv_hypercall() and kvm_xen_hypercall().

Xen doesn't support SEV, so I think this one is ok until they do. Although
I guess we could be preemptive and hit all those call sites. The other
ones are in arch/x86/kvm/hyperv.c.

Thoughts?

Thanks,
Tom

> 

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

[Vitaly Kuznetsov]

Tom Lendacky <thomas.lendacky@amd.com> writes:

> On 5/24/21 6:53 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. Given that, assume that the guest is in 64-bit mode when
>>> processing a hypercall from a guest 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>
>>> ---
>>>  arch/x86/kvm/x86.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 9b6bca616929..e715c69bb882 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>>  
>>>  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>>>  
>>> -	op_64_bit = is_64_bit_mode(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.
>>> +	 */
>>> +	op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected;
>>>  	if (!op_64_bit) {
>>>  		nr &= 0xFFFFFFFF;
>>>  		a0 &= 0xFFFFFFFF;
>> 
>> While this is might be a very theoretical question, what about other
>> is_64_bit_mode() users? Namely, a very similar to the above check exists
>> in kvm_hv_hypercall() and kvm_xen_hypercall().
>
> Xen doesn't support SEV, so I think this one is ok until they do. Although
> I guess we could be preemptive and hit all those call sites. The other
> ones are in arch/x86/kvm/hyperv.c.
>
> Thoughts?

Would it hurt if we just move 'vcpu->arch.guest_state_protected' check
to is_64_bit_mode() itself? It seems to be too easy to miss this
peculiar detail about SEV in review if new is_64_bit_mode() users are to
be added.

-- 
Vitaly

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

[Tom Lendacky]

On 5/24/21 8:49 AM, Vitaly Kuznetsov wrote:
> Tom Lendacky <thomas.lendacky@amd.com> writes:
> 
>> On 5/24/21 6:53 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. Given that, assume that the guest is in 64-bit mode when
>>>> processing a hypercall from a guest 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>
>>>> ---
>>>>  arch/x86/kvm/x86.c | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 9b6bca616929..e715c69bb882 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>>>  
>>>>  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>>>>  
>>>> -	op_64_bit = is_64_bit_mode(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.
>>>> +	 */
>>>> +	op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected;
>>>>  	if (!op_64_bit) {
>>>>  		nr &= 0xFFFFFFFF;
>>>>  		a0 &= 0xFFFFFFFF;
>>>
>>> While this is might be a very theoretical question, what about other
>>> is_64_bit_mode() users? Namely, a very similar to the above check exists
>>> in kvm_hv_hypercall() and kvm_xen_hypercall().
>>
>> Xen doesn't support SEV, so I think this one is ok until they do. Although
>> I guess we could be preemptive and hit all those call sites. The other
>> ones are in arch/x86/kvm/hyperv.c.
>>
>> Thoughts?
> 
> Would it hurt if we just move 'vcpu->arch.guest_state_protected' check
> to is_64_bit_mode() itself? It seems to be too easy to miss this
> peculiar detail about SEV in review if new is_64_bit_mode() users are to
> be added.

I thought about that, but wondered if is_64_bit_mode() was to be used in
other places in the future, if it would be a concern. I think it would be
safe since anyone adding it to a new section of code is likely to look at
what that function is doing first.

I'm ok with this. Paolo, I know you already queued this, but would you
prefer moving the check into is_64_bit_mode()?

Thanks,
Tom

> 

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

[Paolo Bonzini]

On 24/05/21 15:58, Tom Lendacky wrote:
>> Would it hurt if we just move 'vcpu->arch.guest_state_protected' check
>> to is_64_bit_mode() itself? It seems to be too easy to miss this
>> peculiar detail about SEV in review if new is_64_bit_mode() users are to
>> be added.
> I thought about that, but wondered if is_64_bit_mode() was to be used in
> other places in the future, if it would be a concern. I think it would be
> safe since anyone adding it to a new section of code is likely to look at
> what that function is doing first.
> 
> I'm ok with this. Paolo, I know you already queued this, but would you
> prefer moving the check into is_64_bit_mode()?

Let's introduce a new wrapper is_64_bit_hypercall, and add a 
WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode.

Paolo

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

[Tom Lendacky]

On 5/24/21 9:20 AM, Paolo Bonzini wrote:
> On 24/05/21 15:58, Tom Lendacky wrote:
>>> Would it hurt if we just move 'vcpu->arch.guest_state_protected' check
>>> to is_64_bit_mode() itself? It seems to be too easy to miss this
>>> peculiar detail about SEV in review if new is_64_bit_mode() users are to
>>> be added.
>> I thought about that, but wondered if is_64_bit_mode() was to be used in
>> other places in the future, if it would be a concern. I think it would be
>> safe since anyone adding it to a new section of code is likely to look at
>> what that function is doing first.
>>
>> I'm ok with this. Paolo, I know you already queued this, but would you
>> prefer moving the check into is_64_bit_mode()?
> 
> Let's introduce a new wrapper is_64_bit_hypercall, and add a
> WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode.

Will do.

Thanks,
Tom

> 
> Paolo
> 

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

[Sean Christopherson]

On Mon, May 24, 2021, Paolo Bonzini wrote:
> On 24/05/21 15:58, Tom Lendacky wrote:
> > > Would it hurt if we just move 'vcpu->arch.guest_state_protected' check
> > > to is_64_bit_mode() itself? It seems to be too easy to miss this
> > > peculiar detail about SEV in review if new is_64_bit_mode() users are to
> > > be added.
> > I thought about that, but wondered if is_64_bit_mode() was to be used in
> > other places in the future, if it would be a concern. I think it would be
> > safe since anyone adding it to a new section of code is likely to look at
> > what that function is doing first.
> > 
> > I'm ok with this. Paolo, I know you already queued this, but would you
> > prefer moving the check into is_64_bit_mode()?
> 
> Let's introduce a new wrapper is_64_bit_hypercall, and add a
> WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode.

Can we introduce the WARN(s) in a separate patch, and deploy them much more
widely than just is_64_bit_mode()?  I would like to have them lying in wait at
every path that should be unreachable, e.g. get/set segments, get_cpl(), etc...

Side topic, kvm_get_cs_db_l_bits() should be moved to svm.c.  Functionally, it's
fine to have it as a vendor-agnostic helper, but practically speaking it should
never be called directly.

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

[Paolo Bonzini]

On 24/05/21 19:03, Sean Christopherson wrote:
>> Let's introduce a new wrapper is_64_bit_hypercall, and add a
>> WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode.
>
> Can we introduce the WARN(s) in a separate patch, and deploy them much more
> widely than just is_64_bit_mode()?  I would like to have them lying in wait at
> every path that should be unreachable, e.g. get/set segments, get_cpl(), etc...

Each WARN that is added must be audited separately, so this one I'd like 
to have now; it is pretty much the motivation for introducing a new 
function, as the other caller of is_64_bit_mode, kvm_get_linear_rip() is 
already "handling" SEV-ES by always returning 0.

But yes adding more WARNs can only be good.

Paolo

> Side topic, kvm_get_cs_db_l_bits() should be moved to svm.c.  Functionally, it's
> fine to have it as a vendor-agnostic helper, but practically speaking it should
> never be called directly.
> 

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

[Tom Lendacky]

On 5/24/21 12:06 PM, Paolo Bonzini wrote:
> On 24/05/21 19:03, Sean Christopherson wrote:
>>> Let's introduce a new wrapper is_64_bit_hypercall, and add a
>>> WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode.
>>
>> Can we introduce the WARN(s) in a separate patch, and deploy them much more
>> widely than just is_64_bit_mode()?  I would like to have them lying in
>> wait at
>> every path that should be unreachable, e.g. get/set segments, get_cpl(),
>> etc...
> 
> Each WARN that is added must be audited separately, so this one I'd like
> to have now; it is pretty much the motivation for introducing a new
> function, as the other caller of is_64_bit_mode, kvm_get_linear_rip() is
> already "handling" SEV-ES by always returning 0.

The kvm_register_{read,write}() functions also call is_64_bit_mode(). But...

The SVM support uses those for CR and DR intercepts. CR intercepts are not
enabled under SEV-ES. DR intercepts are only set for DR7. DR7 reads don't
actually exit, the intercept is there to trigger the #VC and return a
cached value from the #VC handler. DR7 writes do exit but don't actually
do much since we don't allow guest_debug to be set so kvm_register_read()
is never called.

The x86 support also uses those functions for emulation and SMM, both of
which aren't supported under SEV-ES.

Thanks,
Tom

> 
> But yes adding more WARNs can only be good.
> 
> Paolo
> 
>> Side topic, kvm_get_cs_db_l_bits() should be moved to svm.c. 
>> Functionally, it's
>> fine to have it as a vendor-agnostic helper, but practically speaking it
>> should
>> never be called directly.
>>
> 

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

[Paolo Bonzini]

On 22/05/21 18:43, Tom Lendacky wrote:
> 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. Given that, assume that the guest is in 64-bit mode when
> processing a hypercall from a guest 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>
> ---
>   arch/x86/kvm/x86.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9b6bca616929..e715c69bb882 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   
>   	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>   
> -	op_64_bit = is_64_bit_mode(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.
> +	 */
> +	op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected;
>   	if (!op_64_bit) {
>   		nr &= 0xFFFFFFFF;
>   		a0 &= 0xFFFFFFFF;
> 

Queued, thanks.

Paolo