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

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.
- [High] Missing Context Synchronization Event (`isb()`) after writing to `SYS_ICC_ICSR_EL1`.
- [High] Missing Context Synchronization Event (`isb()`) after writing to `SYS_SCTLR`.
--

--- 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