Re: [PATCH v3 10/11] KVM: arm64: nVHE: Check hypercall handlers against the declared ABI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing context synchronization event (ISB) after writing to SYS_SCTLR.
- [High] Double-fetch of untrusted host memory in pkvm_refill_memcache() leading to a TOCTOU vulnerability.
--

--- Patch [10]: [PATCH v3 10/11] KVM: arm64: nVHE: Check hypercall handlers against the declared ABI ---
commit c02cd7851991d5d8c9aac5af1e86684fe8258ca4
Author: Fuad Tabba <fuad.tabba@linux.dev>
KVM: arm64: nVHE: Check hypercall handlers against the declared ABI

This patch replaces manually unmarshalled hypercall arguments using
DECLARE_REG() with generated handlers via DEFINE_KVM_HOST_HCALL() and
DEFINE_KVM_HOST_HCALL0(). This ensures that the parameter list is type-checked
against the signature declared in <asm/kvm_hcall.h> to prevent mismatches
between caller and handler.

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 23cb4313c60a2..675d607727929 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c

[ ... ]

>  static int pkvm_refill_memcache(struct pkvm_hyp_vcpu *hyp_vcpu)
>  {
>  	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
>  
>  	return refill_memcache(&hyp_vcpu->vcpu.arch.pkvm_memcache,
>  			       host_vcpu->arch.pkvm_memcache.nr_pages,
>  			       &host_vcpu->arch.pkvm_memcache);
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but could this double-fetch of untrusted host
memory lead to a TOCTOU race condition?

The host memory is dereferenced here to read nr_pages and then passed as a
pointer to refill_memcache(), which fetches from it again:

arch/arm64/kvm/hyp/nvhe/mm.c:refill_memcache() {
    ...
    struct kvm_hyp_memcache tmp = *host_mc;
    ...
}

If the host concurrently modifies the struct between the fetches, EL2 might
use a min_pages target that differs from the actual struct state. Should the
struct be copied to EL2 private memory first before acting on it?

>  }

[ ... ]

> -static void handle___kvm_enable_ssbs(struct kvm_cpu_context *host_ctxt)
> +DEFINE_KVM_HOST_HCALL0(void, __kvm_enable_ssbs)
>  {
>  	u64 tmp;
>  
>  	tmp = read_sysreg_el2(SYS_SCTLR);
>  	tmp |= SCTLR_ELx_DSSBS;
>  	write_sysreg_el2(tmp, SYS_SCTLR);
>  }
   ^

[Severity: High]
This isn't a bug introduced by this patch, but should an isb() instruction be
issued after the write to SYS_SCTLR?

Updates to control-plane system registers like SYS_SCTLR require a Context
Synchronization Event (CSE) to be visible to subsequent instructions.
Returning without an immediate isb() might leave the processor operating with
an architecturally undefined pipeline state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804112317.1937387-1-fuad.tabba@linux.dev?part=10