[0/3] KVM: SEV: only access GHCB fields once

Message

Paolo Bonzini Aug. 4, 2023, 5:33 p.m. UTC

The VMGEXIT handler has a time-of-check/time-of-use vulnerability; due
to a double fetch, the guest can exploit a race condition to invoke
the VMGEXIT handler recursively.  It is extremely difficult to
reliably win the race ~100 consecutive times in order to cause an
overflow, and the impact is usually mitigated by CONFIG_VMAP_STACK,
but it ought to be fixed anyway.

One way to do so could be to snapshot the whole GHCB, but this is
relatively expensive.  Instead, because the VMGEXIT handler already
syncs the GHCB to internal KVM state, this series makes sure that the
GHCB is not read outside sev_es_sync_from_ghcb().

Patch 1 adds caching for fields that currently are not snapshotted
in host memory; patch 2 ensures that the cached fields are always used,
thus fixing the race.  Finally patch 3 removes some local variables
that are prone to incorrect use, to avoid reintroducing the race in
other places.

Please review!

Paolo

Paolo Bonzini (3):
  KVM: SEV: snapshot the GHCB before accessing it
  KVM: SEV: only access GHCB fields once
  KVM: SEV: remove ghcb variable declarations

 arch/x86/kvm/svm/sev.c | 124 ++++++++++++++++++++---------------------
 arch/x86/kvm/svm/svm.h |  26 +++++++++
 2 files changed, 87 insertions(+), 63 deletions(-)

Comments

Peter Gonda Aug. 9, 2023, 2:38 p.m. UTC | #1

On Fri, Aug 4, 2023 at 11:34 AM Paolo Bonzini <[e-mail]> wrote:
>
> The VMGEXIT handler has a time-of-check/time-of-use vulnerability; due
> to a double fetch, the guest can exploit a race condition to invoke
> the VMGEXIT handler recursively.  It is extremely difficult to
> reliably win the race ~100 consecutive times in order to cause an
> overflow, and the impact is usually mitigated by CONFIG_VMAP_STACK,
> but it ought to be fixed anyway.
>
> One way to do so could be to snapshot the whole GHCB, but this is
> relatively expensive.  Instead, because the VMGEXIT handler already
> syncs the GHCB to internal KVM state, this series makes sure that the
> GHCB is not read outside sev_es_sync_from_ghcb().
>
> Patch 1 adds caching for fields that currently are not snapshotted
> in host memory; patch 2 ensures that the cached fields are always used,
> thus fixing the race.  Finally patch 3 removes some local variables
> that are prone to incorrect use, to avoid reintroducing the race in
> other places.
>
> Please review!
>

Tested-by: Peter Gonda <[e-mail]>

I booted an Ubuntu guest and ran our internal GHCB correctness test
with these patches.

Tom Lendacky Aug. 14, 2023, 12:58 p.m. UTC | #2

On 8/4/23 12:33, Paolo Bonzini wrote:
> The VMGEXIT handler has a time-of-check/time-of-use vulnerability; due
> to a double fetch, the guest can exploit a race condition to invoke
> the VMGEXIT handler recursively.  It is extremely difficult to
> reliably win the race ~100 consecutive times in order to cause an
> overflow, and the impact is usually mitigated by CONFIG_VMAP_STACK,
> but it ought to be fixed anyway.
> 
> One way to do so could be to snapshot the whole GHCB, but this is
> relatively expensive.  Instead, because the VMGEXIT handler already
> syncs the GHCB to internal KVM state, this series makes sure that the
> GHCB is not read outside sev_es_sync_from_ghcb().
> 
> Patch 1 adds caching for fields that currently are not snapshotted
> in host memory; patch 2 ensures that the cached fields are always used,
> thus fixing the race.  Finally patch 3 removes some local variables
> that are prone to incorrect use, to avoid reintroducing the race in
> other places.
> 
> Please review!

Sorry, just got back from vacation... I'll review this as soon as I can.

Thanks,
Tom

> 
> Paolo
> 
> Paolo Bonzini (3):
>    KVM: SEV: snapshot the GHCB before accessing it
>    KVM: SEV: only access GHCB fields once
>    KVM: SEV: remove ghcb variable declarations
> 
>   arch/x86/kvm/svm/sev.c | 124 ++++++++++++++++++++---------------------
>   arch/x86/kvm/svm/svm.h |  26 +++++++++
>   2 files changed, 87 insertions(+), 63 deletions(-)
>