Re: [PATCH v2 7/9] KVM: arm64: Allocate hyp vectors statically

[Marc Zyngier <maz@kernel.org>]

On 2020-11-09 21:47, Will Deacon wrote:
> The EL2 vectors installed when a guest is running point at one of the
> following configurations for a given CPU:
> 
>   - Straight at __kvm_hyp_vector
>   - A trampoline containing an SMC sequence to mitigate Spectre-v2 and
>     then a direct branch to __kvm_hyp_vector
>   - A dynamically-allocated trampoline which has an indirect branch to
>     __kvm_hyp_vector
>   - A dynamically-allocated trampoline containing an SMC sequence to
>     mitigate Spectre-v2 and then an indirect branch to __kvm_hyp_vector
> 
> The indirect branches mean that VA randomization at EL2 isn't trivially
> bypassable using Spectre-v3a (where the vector base is readable by the
> guest).
> 
> Rather than populate these vectors dynamically, configure everything
> statically and use an enumerated type to identify the vector "slot"
> corresponding to one of the configurations above. This both simplifies
> the code, but also makes it much easier to implement at EL2 later on.

This series terminally breaks VHE (the host locks up on the first guest
run, CPU is nowhere to be seen again).

I have a hunch about what is happening, see below.

[...]

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 044c5fc81f90..ec6dce70c611 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -51,14 +51,6 @@ DECLARE_KVM_HYP_PER_CPU(unsigned long, 
> kvm_hyp_vector);
>  static DEFINE_PER_CPU(unsigned long, kvm_arm_hyp_stack_page);
>  unsigned long kvm_arm_hyp_percpu_base[NR_CPUS];
> 
> -/* Hypervisor VA of the indirect vector trampoline page */
> -static void *__kvm_bp_vect_base;
> -/*
> - * Slot in the hyp vector page for use by the indirect vector 
> trampoline
> - * when mitigation against Spectre-v2 is not required.
> - */
> -static int __kvm_harden_el2_vector_slot;
> -
>  /* The VMID used in the VTTBR */
>  static atomic64_t kvm_vmid_gen = ATOMIC64_INIT(1);
>  static u32 kvm_next_vmid;
> @@ -1303,33 +1295,36 @@ static unsigned long nvhe_percpu_order(void)
>  	return size ? get_order(size) : 0;
>  }
> 
> -static int kvm_map_vectors(void)
> +/* A lookup table holding the hypervisor VA for each vector slot */
> +static void *hyp_spectre_vector_selector[BP_HARDEN_EL2_SLOTS];
> +
> +static void kvm_init_vector_slot(void *base, enum
> arm64_hyp_spectre_vector slot)
>  {
> -	int slot;
> +	hyp_spectre_vector_selector[slot] = base + (slot * SZ_2K);
> +}
> +
> +static int kvm_init_vector_slots(void)
> +{
> +	int err;
> +	void *base;
> +
> +	base = kern_hyp_va(kvm_ksym_ref(__kvm_hyp_vector));
> +	kvm_init_vector_slot(base, HYP_VECTOR_DIRECT);
> +
> +	base = kern_hyp_va(kvm_ksym_ref(__bp_harden_hyp_vecs));
> +	kvm_init_vector_slot(base, HYP_VECTOR_SPECTRE_DIRECT);
> 
> -	/*
> -	 * SV2  = ARM64_SPECTRE_V2
> -	 * HEL2 = ARM64_HARDEN_EL2_VECTORS
> -	 *
> -	 * !SV2 + !HEL2 -> use direct vectors
> -	 *  SV2 + !HEL2 -> use hardened vectors in place
> -	 * !SV2 +  HEL2 -> allocate one vector slot and use exec mapping
> -	 *  SV2 +  HEL2 -> use hardened vectors and use exec mapping
> -	 */
>  	if (!cpus_have_const_cap(ARM64_HARDEN_EL2_VECTORS))
>  		return 0;
> 
> -	/*
> -	 * Always allocate a spare vector slot, as we don't know yet which 
> CPUs
> -	 * have a BP hardening slot that we can reuse.
> -	 */
> -	slot = atomic_inc_return(&arm64_el2_vector_last_slot);
> -	BUG_ON(slot >= BP_HARDEN_EL2_SLOTS);
> -	__kvm_harden_el2_vector_slot = slot;
> +	err = create_hyp_exec_mappings(__pa_symbol(__bp_harden_hyp_vecs),
> +				       __BP_HARDEN_HYP_VECS_SZ, &base);
> +	if (err)
> +		return err;
> 
> -	return create_hyp_exec_mappings(__pa_symbol(__bp_harden_hyp_vecs),
> -					__BP_HARDEN_HYP_VECS_SZ,
> -					&__kvm_bp_vect_base);
> +	kvm_init_vector_slot(base, HYP_VECTOR_INDIRECT);
> +	kvm_init_vector_slot(base, HYP_VECTOR_SPECTRE_INDIRECT);
> +	return 0;
>  }
> 
>  static void cpu_init_hyp_mode(void)
> @@ -1406,24 +1401,9 @@ static void cpu_hyp_reset(void)
>  static void cpu_set_hyp_vector(void)
>  {
>  	struct bp_hardening_data *data = this_cpu_ptr(&bp_hardening_data);
> -	void *vect = kern_hyp_va(kvm_ksym_ref(__kvm_hyp_vector));
> -	int slot = -1;
> -
> -	if (cpus_have_const_cap(ARM64_SPECTRE_V2) && data->fn) {
> -		vect = kern_hyp_va(kvm_ksym_ref(__bp_harden_hyp_vecs));
> -		slot = data->hyp_vectors_slot;
> -	}
> -
> -	if (this_cpu_has_cap(ARM64_HARDEN_EL2_VECTORS) && !has_vhe()) {
> -		vect = __kvm_bp_vect_base;
> -		if (slot == -1)
> -			slot = __kvm_harden_el2_vector_slot;
> -	}
> -
> -	if (slot != -1)
> -		vect += slot * SZ_2K;
> +	void *vector = hyp_spectre_vector_selector[data->slot];
> 
> -	*this_cpu_ptr_hyp_sym(kvm_hyp_vector) = (unsigned long)vect;
> +	*this_cpu_ptr_hyp_sym(kvm_hyp_vector) = (unsigned long)vector;
>  }
> 
>  static void cpu_hyp_reinit(void)
> @@ -1661,9 +1641,9 @@ static int init_hyp_mode(void)
>  		goto out_err;
>  	}
> 
> -	err = kvm_map_vectors();
> +	err = kvm_init_vector_slots();

Here, you have turned the mapping of the vectors into the full
init+map. The mapping makes no sense on VHE, but the initialization
does matter! init_hyp_mode() being only called on non-VHE, the HYP
vectors are never initialized. Too bad.

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm