Re: [PATCH v2 02/13] KVM: x86: Protect kvm_hv_msr_[get|set]_crash_data() from Spectre-v1/L1TF attacks

[Jim Mattson]

On Thu, Dec 12, 2019 at 9:31 AM Christian Borntraeger
<borntraeger@xxxxxxxxxx> wrote:
>
>
>
> On 11.12.19 21:47, Marios Pomonis wrote:
> > This fixes Spectre-v1/L1TF vulnerabilities in kvm_hv_msr_get_crash_data()
> > and kvm_hv_msr_set_crash_data().
> > These functions contain index computations that use the
> > (attacker-controlled) MSR number.
> >
> > Fixes: commit e7d9513b60e8 ("kvm/x86: added hyper-v crash msrs into kvm hyperv context")
> >
> > Signed-off-by: Nick Finco <nifi@xxxxxxxxxx>
> > Signed-off-by: Marios Pomonis <pomonis@xxxxxxxxxx>
> > Reviewed-by: Andrew Honig <ahonig@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >  arch/x86/kvm/hyperv.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 23ff65504d7e..26408434b9bc 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -809,11 +809,12 @@ static int kvm_hv_msr_get_crash_data(struct kvm_vcpu *vcpu,
> >                                    u32 index, u64 *pdata)
> >  {
> >       struct kvm_hv *hv = &vcpu->kvm->arch.hyperv;
> > +     size_t size = ARRAY_SIZE(hv->hv_crash_param);
> >
> > -     if (WARN_ON_ONCE(index >= ARRAY_SIZE(hv->hv_crash_param)))
> > +     if (WARN_ON_ONCE(index >= size))
> >               return -EINVAL;
>
> The fact that we do a WARN_ON_ONCE here, should actually tell that index is not
> user controllable. Otherwise this would indicate the possibility to trigger a
> kernel warning from a malicious user space. So
> a: we do not need this change
> or
> b: we must also fix the WARN_ON_ONCE

That isn't quite true. The issue is *speculative* execution down this path.

The call site does constrain the *actual* value of index:

case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
        return kvm_hv_msr_get_crash_data(...);

However, it is possible to train the branch predictor to go down this
path using valid indices and subsequently pass what would be an
invalid index. The CPU will speculatively follow this path and may
pull interesting data into the cache before it realizes its mistake
and corrects.