Re: [PATCH] KVM: x86: hyper-v: Bound the bank index in hv_is_vp_in_sparse_set()

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Hyunwoo Kim <imv4bel@gmail.com> writes:

> hv_is_vp_in_sparse_set() uses valid_bit_nr, i.e. vp_id divided by
> HV_VCPUS_PER_SPARSE_BANK, as the test_bit() index into
> valid_bank_mask. valid_bank_mask is a single u64 and a sparse vCPU
> set holds at most HV_MAX_SPARSE_VCPU_BANKS banks, so valid_bit_nr
> must be less than HV_MAX_SPARSE_VCPU_BANKS.
>
> The caller in kvm_hv_send_ipi_to_many() passes kvm_hv_get_vpindex(),
> which is below KVM_MAX_VCPUS and therefore always within that bound.
> The L2 direct flush branch in kvm_hv_flush_tlb(), however, passes
> hv_v->nested.vp_id, copied verbatim from the enlightened VMCS
> without any bounds check, so valid_bit_nr can reach
> HV_MAX_SPARSE_VCPU_BANKS or more and test_bit() then reads beyond
> valid_bank_mask.
>
> Return false before the test_bit() when valid_bit_nr is not below
> HV_MAX_SPARSE_VCPU_BANKS, since such a VP cannot be present in the
> set.
>
> Cc: stable@vger.kernel.org
> Fixes: c58a318f6090 ("KVM: x86: hyper-v: L2 TLB flush")
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>  arch/x86/kvm/hyperv.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 4438ecac9a89..d8782cb7ba02 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1839,6 +1839,10 @@ static bool hv_is_vp_in_sparse_set(u32 vp_id, u64 valid_bank_mask, u64 sparse_ba
>  	int valid_bit_nr = vp_id / HV_VCPUS_PER_SPARSE_BANK;
>  	unsigned long sbank;
>  
> +	/* A bank index beyond the mask can't be set, the VP isn't in the set. */
> +	if (valid_bit_nr >= HV_MAX_SPARSE_VCPU_BANKS)
> +		return false;
> +
>  	if (!test_bit(valid_bit_nr, (unsigned long *)&valid_bank_mask))
>  		return false;

I think the concern is valid, so

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

what I'm not sure about if we should also deliberately crash the VM
which does such a hypercall. This way it would be easier to find buggy
L1s but given that they are most likely Windows, we need to do some
tests to see if this is not actually happening today (e.g. Hyper-V usign
VP_ID or '-1' for something). Let's have this as a future TODO item.

-- 
Vitaly