Re: [PATCH v4 23/29] KVM: arm64: Add NVHCR_EL2 context switching

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] TOCTOU vulnerability in NV3 context switching leading to cross-VM information leak
--

--- Patch [23]: [PATCH v4 23/29] KVM: arm64: Add NVHCR_EL2 context switching ---
commit f6089755f833008f36c0718e0727dbb85a4f24ae
Author: Marc Zyngier <[email protected]>

KVM: arm64: Add NVHCR_EL2 context switching

This commit introduces NVHCR_EL2 context switching. For a guest in hyp
context, NVHCR_EL2 reflects HCR_EL2 and is eagerly synced. For an L2 guest,
it is context switched like any EL1 register, conditioned on the guest using
NV3.

> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index a17cbe7582de9..c382848d31947 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -172,6 +172,10 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>  
>  	if (ctxt_has_sctlr2(ctxt))
>  		ctxt_sys_reg(ctxt, SCTLR2_EL1) = read_sysreg_el1(SYS_SCTLR2);
> +
> +	/* Retrieve L2's HCR_EL2, and save it for future use */
> +	if (is_nested_nv3_ctxt(ctxt_to_vcpu(ctxt)))
> +		ctxt_sys_reg(ctxt, NVHCR_EL2) = read_sysreg_s(SYS_NVHCR_EL2);

[Severity: High]
Does this introduce a TOCTOU regression leading to a cross-VM information
leak?

KVM uses the guest-modifiable VNCR state to decide whether to read the
hardware register, allowing a TOCTOU that reads uninitialized or stale
hardware state.

Looking at is_nested_nv3_ctxt(), it evaluates HCRX_EL2.NVTGE directly from
the guest-modifiable VNCR page:

arch/arm64/include/asm/kvm_emulate.h:is_nested_nv3_ctxt() {
    ...
    return (... && (__vcpu_sys_reg(vcpu, HCRX_EL2) & HCRX_EL2_NVTGE));
}

If an L1 guest concurrently modifies its VNCR page from another vCPU to
change NVTGE from 0 to 1 during an L2 guest's execution, KVM might read from
SYS_NVHCR_EL2 here on vcpu_put() or during sysreg trap handling. This could
copy the leaked hardware state into the L2 guest's context, where it can be
read.

[ ... ]

> @@ -285,6 +289,13 @@ static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt,
>  
>  	if (ctxt_has_sctlr2(ctxt))
>  		write_sysreg_el1(ctxt_sys_reg(ctxt, SCTLR2_EL1), SYS_SCTLR2);
> +
> +	/*
> +	 * Publish the L2 view of HCR_EL2 to the HW if L1 is using NV3.
> +	 * Otherwise, the data is already in place in the L1's own VNCR.
> +	 */
> +	if (is_nested_nv3_ctxt(ctxt_to_vcpu(ctxt)))
> +		write_sysreg_s(ctxt_sys_reg(ctxt, NVHCR_EL2), SYS_NVHCR_EL2);

[Severity: High]
Can the same concurrent VNCR page modification cause KVM to skip initializing
the hardware register?

If NVTGE is 0 during vcpu_load(), KVM skips initializing the hardware
register, leaving the previous VM's state in hardware. A malicious L1 guest
can trigger this by configuring an L2 guest, spawning a concurrent vCPU to
modify the VNCR page, and executing a trapped instruction in L2.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=23