[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <930c4030-9581-cded-9349-c4486749c7d7@amd.com>
Date: Tue, 15 Aug 2023 10:51:07 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Cc: pgonda@...gle.com, seanjc@...gle.com, theflow@...gle.com,
vkuznets@...hat.com, stable@...r.kernel.org
Subject: Re: [PATCH 2/3] KVM: SEV: only access GHCB fields once
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@...r.kernel.org
> Reported-by: Andy Nguyen <theflow@...gle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
Just one very minor comment below, otherwise
Reviewed-by: Tom Lendacky <thomas.lendacky@....com>
> ---
> 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)
Powered by blists - more mailing lists