On Thu, Dec 12, 2019 at 1:43 AM Vitaly Kuznetsov vkuznets@redhat.com wrote:

Marios Pomonis pomonis@google.com writes:

...

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.

Just to educate myself,

in both cases 'index' is equal to 'msr - HV_X64_MSR_CRASH_P0' where 'msr' is constrained: case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4: ....

and moreover, kvm_hv_{get,set}_msr_common() is only being called for a narrow set of MSRs. How can an atacker overcome these limitations?

This attack scenario relies on speculative execution. Practically, one could train the branch predictors involved to speculatively execute this path even if the adversary-supplied MSR number does not fall into the legitimate range. The adversary-supplied MSR number however is going to be used when -speculatively- computing the index of the array thus allowing an attacker to load normally illegitimate memory values in the L1 cache.

...

Fixes: commit e7d9513b60e8 ("kvm/x86: added hyper-v crash msrs into kvm hyperv context")

Signed-off-by: Nick Finco nifi@google.com Signed-off-by: Marios Pomonis pomonis@google.com Reviewed-by: Andrew Honig ahonig@google.com Cc: stable@vger.kernel.org

---

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;
  
  

• *pdata = hv->hv_crash_param[index];
  

• *pdata = hv->hv_crash_param[array_index_nospec(index, size)];
  return 0;
  

}

@@ -852,11 +853,12 @@ static int kvm_hv_msr_set_crash_data(struct kvm_vcpu *vcpu, u32 index, u64 data) { 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;
  
  

• hv->hv_crash_param[index] = data;
  

• hv->hv_crash_param[array_index_nospec(index, size)] = data;
  return 0;
  

}

-- Vitaly

On Thu, Dec 12, 2019 at 9:31 AM Christian Borntraeger borntraeger@de.ibm.com 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@google.com Signed-off-by: Marios Pomonis pomonis@google.com Reviewed-by: Andrew Honig ahonig@google.com Cc: stable@vger.kernel.org

---

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.