Re: [PATCH] KVM: VMX: Add array_index_nospec() to evmcs_field_offset()

[Vitaly Kuznetsov]

Amirmohammad Eftekhar <amirmohammad.eftekhar@xxxxxxxx> writes:

> evmcs_field_offset() bounds-checks the guest-supplied field index against
> nr_evmcs_1_fields and then indexes vmcs_field_to_evmcs_1[] without an
> array_index_nospec() clamp, unlike its vmcs12 counterpart
> get_vmcs12_field_offset().  An enlightened VMREAD/VMWRITE from the guest
> can therefore speculatively read out of bounds of vmcs_field_to_evmcs_1[]
> past the bound check (Spectre v1).

I think your concern is valid but the changelog requires massaging.

"enlightened VMREAD/VMWRITE" is basically the guest doing just memory
read/write and we can't do much about it. These reads/writes don't trap
to KVM. What the guest can actually do, is try executing 'normal'
VMREAD/VMWRITE instructions.

Note, Hyper-V TLFS states that

"Any VMREAD or VMWRITE instructions while an enlightened VMCS is active
is unsupported and can result in unexpected behavior."

and our initial implementation was completely forbidding
both. Unfortunately, it turned out that Windows itself does not comply
and does occasional VMREADs, see 6cbbaab60ff3 ("KVM: nVMX: Allow VMREAD
when Enlightened VMCS is in use"). I think you need to add corresponding
Fixes: tag.

My understanding is that VMWRITEs are still completely forbidden while
eVMCS is active. So I think you need to say something like

"""
Regular VMREAD instruction is allowed when eVMCS is active since
6cbbaab60ff3 ("KVM: nVMX: Allow VMREAD when Enlightened VMCS is in use")
but unlike get_vmcs12_field_offset() evmcs_field_offset() does not clamp
the index and thus the guest may be able to speculatively access
hypervisor's memory.
"""
Note, 'offset' is short and thus the exposure is somewhat limited.

Other pathes using evmcs_field_offset() should not be under the direct
guest control but array_index_nospec() should not hurt.

>
> Clamp the index with array_index_nospec(), mirroring
> get_vmcs12_field_offset().
>
> Signed-off-by: Amirmohammad Eftekhar <amirmohammad.eftekhar@xxxxxxxx>
> ---
>  arch/x86/kvm/vmx/hyperv_evmcs.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/hyperv_evmcs.h b/arch/x86/kvm/vmx/hyperv_evmcs.h
> index bc08fe405..052b62b8e 100644
> --- a/arch/x86/kvm/vmx/hyperv_evmcs.h
> +++ b/arch/x86/kvm/vmx/hyperv_evmcs.h
> @@ -136,6 +136,8 @@ static __always_inline int evmcs_field_offset(unsigned long field,
>  	if (unlikely(index >= nr_evmcs_1_fields))
>  		return -ENOENT;
>  
> +	index = array_index_nospec(index, nr_evmcs_1_fields);
> +
>  	evmcs_field = &vmcs_field_to_evmcs_1[index];
>  
>  	/*

-- 
Vitaly