Re: [PATCH v3 07/10] KVM: arm64: Allocate hyp vectors statically

[Marc Zyngier <maz@kernel.org>]

On 2020-11-13 11:38, 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.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_asm.h |  5 --
>  arch/arm64/include/asm/spectre.h | 36 +++++++++++++-
>  arch/arm64/kernel/cpu_errata.c   |  2 +
>  arch/arm64/kernel/proton-pack.c  | 63 +++++-------------------
>  arch/arm64/kvm/arm.c             | 82 +++++++++++++-------------------
>  arch/arm64/kvm/hyp/Makefile      |  2 +-
>  arch/arm64/kvm/hyp/hyp-entry.S   | 72 ++++++++++++++++------------
>  arch/arm64/kvm/hyp/smccc_wa.S    | 32 -------------
>  arch/arm64/kvm/va_layout.c       | 11 +----
>  9 files changed, 126 insertions(+), 179 deletions(-)
>  delete mode 100644 arch/arm64/kvm/hyp/smccc_wa.S

I haven't had a chance to test this series yet, but I may have spotted
another small nit, see below:

[...]

> +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;
> +	if (!has_vhe()) {
> +		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 +1403,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;
> -	}
> +	void *vector = hyp_spectre_vector_selector[data->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;
> -
> -	*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 +1643,9 @@ static int init_hyp_mode(void)
>  		goto out_err;
>  	}
> 
> -	err = kvm_map_vectors();
> +	err = kvm_init_vector_slots();
>  	if (err) {
> -		kvm_err("Cannot map vectors\n");
> +		kvm_err("Cannot initialise vector slots\n");
>  		goto out_err;
>  	}
> 
> @@ -1810,6 +1792,10 @@ int kvm_arch_init(void *opaque)
>  			goto out_err;
>  	}
> 
> +	err = kvm_init_vector_slots();
> +	if (err)
> +		goto out_err;

Don't you end-up calling kvm_init_vector_slots() twice on nVHE?
It's probably harmless, but I think we can have a single call here,
and drop the call from init_hyp_mode().

What do you think? If you agree, I can perform the change when queuing
the series.

Thanks,

         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