On Thu, Nov 12, 2020 at 11:07:55AM +0000, Marc Zyngier wrote:
> 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).
Whoops, sorry about that. I'll look into it.
> I have a hunch about what is happening, see below.
>
> [...]
>
> > @@ -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.
Yeah, that certainly smells bad. I'll hack at it now and post a v3 when
it comes back to life.
Cheers,
Will
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
