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

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

[Amirmohammad Eftekhar]

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).

Clamp the index with array_index_nospec(), mirroring
get_vmcs12_field_offset().

Signed-off-by: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>
---
 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];
 
 	/*
-- 
2.54.0 (Apple Git-157)

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

[Vitaly Kuznetsov]

Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de> 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@cispa.de>
> ---
>  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

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

[Amirmohammad Eftekhar]

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().

A regular VMREAD instruction is allowed while an enlightened VMCS is active
since 6cbbaab60ff3 ("KVM: nVMX: Allow VMREAD when Enlightened VMCS is in
use"), and it reaches evmcs_field_offset() with a guest-supplied field
encoding. Because the index is not clamped, the guest may be able to
speculatively access past the bound check and read out of bounds of
vmcs_field_to_evmcs_1[] (Spectre v1). The leaked value is only the u16
'offset', so the exposure is limited.

The other callers of evmcs_field_offset() are not under direct guest
control, but the array_index_nospec() clamp does not hurt them.

Clamp the index with array_index_nospec(), mirroring
get_vmcs12_field_offset().

Fixes: 6cbbaab60ff3 ("KVM: nVMX: Allow VMREAD when Enlightened VMCS is in use")
Signed-off-by: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>
---
v2:
 - Reword the changelog per Vitaly Kuznetsov: the reachable path is a
   regular VMREAD (allowed since 6cbbaab60ff3), not an "enlightened
   VMREAD/VMWRITE"; note the limited exposure and the other callers.
 - Add the Fixes: tag.

v1: https://lore.kernel.org/all/20261017093016.32348-3-amirmohammad.eftekhar@cispa.de/

 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];

 	/*
--
2.54.0 (Apple Git-157)