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

[Will Deacon <will@kernel.org>]

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@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[]'?

Either way:

Acked-by: Will Deacon <will@kernel.org>

Will