Re: [PATCH 2/3] KVM: SEV: only access GHCB fields once

[Tom Lendacky]

On 8/4/23 12:33, Paolo Bonzini wrote:
> A KVM guest using SEV-ES or SEV-SNP with multiple vCPUs can trigger
> a double fetch race condition vulnerability and invoke the VMGEXIT
> handler recursively.
>
> sev_handle_vmgexit() maps the GHCB page using kvm_vcpu_map() and then
> fetches the exit code using ghcb_get_sw_exit_code().  Soon after,
> sev_es_validate_vmgexit() fetches the exit code again. Since the GHCB
> page is shared with the guest, the guest is able to quickly swap the
> values with another vCPU and hence bypass the validation. One vmexit code
> that can be rejected by sev_es_validate_vmgexit() is SVM_EXIT_VMGEXIT;
> if sev_handle_vmgexit() observes it in the second fetch, the call
> to svm_invoke_exit_handler() will invoke sev_handle_vmgexit() again
> recursively.
>
> To avoid the race, always fetch the GHCB data from the places where
> sev_es_sync_from_ghcb stores it.
>
> Exploiting recursions on linux kernel has been proven feasible
> in the past, but the impact is mitigated by stack guard pages
> (CONFIG_VMAP_STACK).  Still, if an attacker manages to call the handler
> multiple times, they can theoretically trigger a stack overflow and
> cause a denial-of-service, or potentially guest-to-host escape in kernel
> configurations without stack guard pages.
>
> Note that winning the race reliably in every iteration is very tricky
> due to the very tight window of the fetches; depending on the compiler
> settings, they are often consecutive because of optimization and inlining.
>
> Tested by booting an SEV-ES RHEL9 guest.
>
> Fixes: CVE-2023-4155
> Fixes: 291bd20d5d88 ("KVM: SVM: Add initial support for a VMGEXIT VMEXIT")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Andy Nguyen <theflow@xxxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>

Just one very minor comment below, otherwise

Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>

> ---
>   arch/x86/kvm/svm/sev.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index e898f0b2b0ba..ca4ba5fe9a01 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2445,9 +2445,15 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
>   	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
>   }
>   
> +static u64 kvm_ghcb_get_sw_exit_code(struct vmcb_control_area *control)
> +{

Since ghcb is in the function name it might be nice to have a comment 
indicating that the actual GHCB value was copied to the VMCB fields as 
part of sev_es_sync_from_ghcb() and this is used to avoid reading from the 
GHCB after validation.

Thanks,
Tom

> +	return (((u64)control->exit_code_hi) << 32) | control->exit_code;
> +}
> +
>   static int sev_es_validate_vmgexit(struct vcpu_svm *svm)