Re: [PATCH v1 2/4] KVM: arm64: Reject the GICv5 CPU interface hypercalls under pKVM

On Thu, 6 Aug 2026 at 11:20, <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Missing pointer validation in always-on legacy hypercalls allows arbitrary hypervisor memory corruption and information leaks by the host.

Under pKVM this patch removes the dereference: the early return is
before cpu_if is used. Without pKVM the host is trusted.
__kvm_adjust_pc() is WIP for the upcoming pKVM series.

> - [High] Missing Context Synchronization Event (`isb()`) after writing to `SYS_ICC_ICSR_EL1`.

ERET into the guest is a context synchronization event.

> - [High] Missing Context Synchronization Event (`isb()`) after writing to `SYS_SCTLR`.

ERET back to the host on hypercall return is a context synchronization event.

/fuad

> --
>
> --- Patch [2]: [PATCH v1 2/4] KVM: arm64: Reject the GICv5 CPU interface hypercalls under pKVM ---
> commit ecfed0b01b70bb8a7e96b451e64a9f311953ed53
> Author: Fuad Tabba <[email protected]>
>
> KVM: arm64: Reject the GICv5 CPU interface hypercalls under pKVM
>
> __vgic_v5_save_apr() and __vgic_v5_restore_vmcr_apr() remain callable
> after pKVM finalises. pKVM never registers a GICv5 vgic, so neither has
> a valid caller in protected mode, and on a GICv3 machine the registers
> they access are UNDEFINED at EL2 and panic the hypervisor.
>
> Reject both when protected mode is enabled.
>
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > index d3df96ed8ba42..a9afd350b1fb3 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > @@ -695,6 +695,9 @@ static void handle___vgic_v5_save_apr(struct kvm_cpu_context *host_ctxt)
> >  {
> >       DECLARE_REG(struct vgic_v5_cpu_if *, cpu_if, host_ctxt, 1);
> >
> > +     if (unlikely(is_protected_kvm_enabled()))
> > +             return;
> > +
> >       __vgic_v5_save_apr(kern_hyp_va(cpu_if));
> >  }
> >
> > @@ -702,6 +705,9 @@ static void handle___vgic_v5_restore_vmcr_apr(struct kvm_cpu_context *host_ctxt)
> >  {
> >       DECLARE_REG(struct vgic_v5_cpu_if *, cpu_if, host_ctxt, 1);
> >
> > +     if (unlikely(is_protected_kvm_enabled()))
> > +             return;
> > +
> >       __vgic_v5_restore_vmcr_apr(kern_hyp_va(cpu_if));
> >  }
>
> [Severity: Critical]
> This is a pre-existing issue, but could the missing pointer validation in
> these always-on legacy hypercalls allow hypervisor memory corruption and
> information leaks by the host?
>
> arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___kvm_adjust_pc() {
>         DECLARE_REG(struct kvm_vcpu *, vcpu, host_ctxt, 1);
>
>         __kvm_adjust_pc(kern_hyp_va(vcpu));
> }
>
> arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___vgic_v3_save_aprs() {
>         DECLARE_REG(struct vgic_v3_cpu_if *, cpu_if, host_ctxt, 1);
>
>         __vgic_v3_save_aprs(kern_hyp_va(cpu_if));
> }
>
> arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___vgic_v3_restore_vmcr_aprs() {
>         DECLARE_REG(struct vgic_v3_cpu_if *, cpu_if, host_ctxt, 1);
>
>         __vgic_v3_restore_vmcr_aprs(kern_hyp_va(cpu_if));
> }
>
> By exploiting the idempotence of kern_hyp_va() for EL2 linear-map pointers,
> could a malicious host pass a pointer to EL2-private structures, tricking
> EL2 into reading or writing private memory instead of host memory?
>
> Should EL2 validate that the pointer points to host memory, or use the
> known pkvm_get_loaded_hyp_vcpu() state instead of trusting the
> host-supplied pointer?
>
> [Severity: High]
> This is a pre-existing issue, but does the write to SYS_ICC_ICSR_EL1
> require a Context Synchronization Event (isb()) before returning?
>
> arch/arm64/kvm/hyp/vgic-v5-sr.c:__vgic_v5_restore_state() {
> ...
>         write_sysreg_s(cpu_if->vgic_icsr, SYS_ICC_ICSR_EL1);
> }
>
> Since writes to most ICC_*_EL1 registers require an immediate isb(), could
> placing a function return between the write and a barrier cause the CPU
> to observe an undefined pipeline state?
>
> Would this lead to unpredictable behavior or spurious faults when
> switching to a guest?
>
> [Severity: High]
> This is a pre-existing issue, but is there a missing Context Synchronization
> Event (isb()) after writing to SYS_SCTLR here?
>
> arch/arm64/kvm/hyp/nvhe/hyp-main.c:handle___kvm_enable_ssbs() {
>         u64 tmp;
>
>         tmp = read_sysreg_el2(SYS_SCTLR);
>         tmp |= SCTLR_ELx_DSSBS;
>         write_sysreg_el2(tmp, SYS_SCTLR);
> }
>
> If every write to a control-plane system register must be followed by an
> isb() as the very next instruction, could missing the isb() before
> returning result in executing subsequent instructions with an
> architecturally undefined pipeline state?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2