Re: [PATCH v2 05/22] KVM: arm64: Make pKVM ownership selftest an HVC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Exposing the pKVM ownership selftest as an HVC allows the untrusted host to trigger an Out-Of-Bounds (OOB) write in hypervisor memory.
- [Medium] Missing NULL check on `init_selftest_vm()` return value leads to an EL2 panic on malicious host input.
--

commit 8fb13365545b2fb6e99ef459c949f368c81f9f88
Author: Vincent Donnefort <vdonnefort@google.com>

KVM: arm64: Make pKVM ownership selftest an HVC

In preparation for testing the pKVM ownership with huge mappings,
refactor the selftest to be triggered via an HVC using dynamically
allocated memory from the host. This avoids wasting memory which would
be even worse for testing ownership transitions for huge stage-2
mappings.

> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -1748,24 +1748,36 @@ static void assert_page_state(void)
[ ... ]
> +int __pkvm_ownership_selftest(u64 pfn, u64 pgtable_pfn, unsigned long pgtable_nr_pages)
>  {
[ ... ]
> +	selftest_vcpu = vcpu = init_selftest_vm(hyp_phys_to_virt(hyp_pfn_to_phys(pgtable_pfn)),
> +						pgtable_nr_pages);
>  	vm = pkvm_hyp_vcpu_to_hyp_vm(vcpu);

[Severity: Medium]
Is there a possibility of a NULL pointer dereference leading to an EL2 panic
here? If the host issues the selftest HVC with pgtable_nr_pages set to 0,
init_selftest_vm() evaluates seeded < min_pages as true and returns NULL.
This NULL return is then unconditionally assigned to vcpu and immediately
dereferenced in pkvm_hyp_vcpu_to_hyp_vm(vcpu).

Should there be a check on the return value of init_selftest_vm() before
using it?

> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> @@ -762,7 +762,7 @@ static struct pkvm_hyp_vcpu selftest_vcpu = {
[ ... ]
> -struct pkvm_hyp_vcpu *init_selftest_vm(void *virt)
> +struct pkvm_hyp_vcpu *init_selftest_vm(void *virt, u64 nr_pages)
>  {
[ ... ]
>  	selftest_vm.kvm.arch.pkvm.handle = __pkvm_reserve_vm();
>  	insert_vm_table_entry(selftest_vm.kvm.arch.pkvm.handle, &selftest_vm);
>  	return &selftest_vcpu;
>  }
>  
>  void teardown_selftest_vm(void)
>  {
> +	struct kvm_hyp_memcache *vcpu_mc = &selftest_vcpu.vcpu.arch.pkvm_memcache;
> +	struct kvm_hyp_memcache mc = { 0 };
> +
> +	while (vcpu_mc->nr_pages)
> +		hyp_put_page(&selftest_vm.pool, pop_hyp_memcache(vcpu_mc, hyp_phys_to_virt));
> +
>  	hyp_spin_lock(&vm_table_lock);
>  	remove_vm_table_entry(selftest_vm.kvm.arch.pkvm.handle);

[Severity: High]
Can this sequence cause an out-of-bounds write in hypervisor memory?

If the host calls the __pkvm_ownership_selftest HVC when the VM table is
full, __pkvm_reserve_vm() fails and returns -ENOMEM. In init_selftest_vm(),
this negative error code is assigned blindly to the handle.

While insert_vm_table_entry() safely rejects the negative handle,
teardown_selftest_vm() unconditionally calls remove_vm_table_entry()
with it. Inside remove_vm_table_entry(), vm_handle_to_idx() converts
the negative handle into a massive 32-bit unsigned index without bounds
checking, resulting in an out-of-bounds write when vm_table[idx] = NULL
executes.

>  	hyp_spin_unlock(&vm_table_lock);
> +
> +	reclaim_pgtable_pages(&selftest_vm, &mc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911135053.146435-1-vdonnefort@google.com?part=5