[PATCH v1 3/3] KVM: arm64: Create each pKVM hyp vcpu after its corresponding host vcpu

[Fuad Tabba]

Hi Will,

On Mon, 17 Feb 2025 at 15:41, Fuad Tabba <tabba at google.com> wrote:
>
> Hi Will,
>
> On Mon, 17 Feb 2025 at 15:30, Will Deacon <will at kernel.org> wrote:
> >
> > On Fri, Feb 14, 2025 at 03:02:58PM +0000, Fuad Tabba wrote:
> > > Instead of creating and initializing _all_ hyp vcpus in pKVM when
> > > the first host vcpu runs for the first time, initialize _each_
> > > hyp vcpu in conjunction with its corresponding host vcpu.
> > >
> > > Some of the host vcpu state (e.g., system registers and traps
> > > values) are not initialized until the first time the host vcpu is
> > > run. Therefore, initializing a hyp vcpu before its corresponding
> > > host vcpu has run for the first time might not view the complete
> > > host state of these vcpus.
> > >
> > > Additionally, this behavior is inline with non-protected modes.
> > >
> > > Signed-off-by: Fuad Tabba <tabba at google.com>
> > > ---
> > >  arch/arm64/include/asm/kvm_host.h      |  2 +
> > >  arch/arm64/include/asm/kvm_pkvm.h      |  1 +
> > >  arch/arm64/kvm/arm.c                   |  4 ++
> > >  arch/arm64/kvm/hyp/include/nvhe/pkvm.h |  6 ---
> > >  arch/arm64/kvm/hyp/nvhe/pkvm.c         | 54 +++++++++++++++-----------
> > >  arch/arm64/kvm/pkvm.c                  | 28 ++++++-------
> > >  6 files changed, 53 insertions(+), 42 deletions(-)
> >
> > [...]
> >
> > >  static int init_pkvm_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu,
> > >                             struct pkvm_hyp_vm *hyp_vm,
> > > -                           struct kvm_vcpu *host_vcpu,
> > > -                           unsigned int vcpu_idx)
> > > +                           struct kvm_vcpu *host_vcpu)
> > >  {
> > >       int ret = 0;
> > >
> > >       if (hyp_pin_shared_mem(host_vcpu, host_vcpu + 1))
> > >               return -EBUSY;
> > >
> > > -     if (host_vcpu->vcpu_idx != vcpu_idx) {
> > > -             ret = -EINVAL;
> > > -             goto done;
> > > -     }
> > > -
> > >       hyp_vcpu->host_vcpu = host_vcpu;
> > >
> > >       hyp_vcpu->vcpu.kvm = &hyp_vm->kvm;
> > >       hyp_vcpu->vcpu.vcpu_id = READ_ONCE(host_vcpu->vcpu_id);
> > > -     hyp_vcpu->vcpu.vcpu_idx = vcpu_idx;
> > > +     hyp_vcpu->vcpu.vcpu_idx = READ_ONCE(host_vcpu->vcpu_idx);
> > >
> > >       hyp_vcpu->vcpu.arch.hw_mmu = &hyp_vm->kvm.arch.mmu;
> > >       hyp_vcpu->vcpu.arch.cflags = READ_ONCE(host_vcpu->arch.cflags);
> > > @@ -687,27 +689,28 @@ int __pkvm_init_vcpu(pkvm_handle_t handle, struct kvm_vcpu *host_vcpu,
> > >               goto unlock;
> > >       }
> > >
> > > -     idx = hyp_vm->nr_vcpus;
> > > +     ret = init_pkvm_hyp_vcpu(hyp_vcpu, hyp_vm, host_vcpu);
> > > +     if (ret)
> > > +             goto unlock;
> > > +
> > > +     idx = hyp_vcpu->vcpu.vcpu_idx;
> > >       if (idx >= hyp_vm->kvm.created_vcpus) {
> > >               ret = -EINVAL;
> > >               goto unlock;
> > >       }
> > >
> > > -     ret = init_pkvm_hyp_vcpu(hyp_vcpu, hyp_vm, host_vcpu, idx);
> > > -     if (ret)
> > > +     if (hyp_vm->vcpus[idx]) {
> > > +             ret = -EINVAL;
> > >               goto unlock;
> > > +     }
> >
> > I'm not sure how much we care at EL2, but it looks like there's a
> > potential spectre gadget here given that 'idx' is now untrusted.
> > Perhaps chuck something like:
> >
> >         idx = array_index_nospec(idx, hyp_vm->kvm.created_vcpus);
> >
> > before indexing into 'hyp_vm->vcpus[]'?
>
> I'll add that when I respin.

pKVM is riddled with these kinds of accesses (e.g.,
get_vm_by_handle(). It might be better if I address this in a separate
series.

Cheers,
/fuad

>
> > Either way:
> >
> > Acked-by: Will Deacon <will at kernel.org>
>
> Thanks,
> /fuad
>
> > Will