Re: [PATCH v4 09/29] KVM: arm64: Relax CPTR_EL2 handling when FEAT_NV2p1 is present

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Userspace modifications to CPTR_EL2 via KVM_SET_ONE_REG are silently lost on systems with FEAT_NV2p1.
--

--- Patch [9]: [PATCH v4 09/29] KVM: arm64: Relax CPTR_EL2 handling when FEAT_NV2p1 is present ---
commit 0ba038cc87ef366635c1e40d5069280819355ee7
Author: Marc Zyngier <[email protected]>
KVM: arm64: Relax CPTR_EL2 handling when FEAT_NV2p1 is present

This patch updates the handling of CPTR_EL2 accesses when FEAT_NV2P1 is
present, preventing trapping via CPACR_EL1 and directly context switching
CPACR_EL1 in NV contexts instead.

> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> index be685b63e8cf2..6f0f046e4ca4e 100644
> --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> @@ -42,10 +42,12 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
>  		u64 val;
>  
>  		/*
> -		 * We don't save CPTR_EL2, as accesses to CPACR_EL1
> -		 * are always trapped, ensuring that the in-memory
> -		 * copy is always up-to-date. A small blessing...
> +		 * Without FEAT_NV2p1, we don't save CPTR_EL2, as accesses
> +		 * to CPACR_EL1 are always trapped, ensuring that the
> +		 * in-memory copy is always up-to-date. A small blessing...
>  		 */
> +		if (cpus_have_final_cap(ARM64_HAS_NV2P1))
> +			__vcpu_assign_sys_reg(vcpu, CPTR_EL2, read_sysreg_el1(SYS_CPACR));

[Severity: High]
Will this silently overwrite userspace modifications to CPTR_EL2 during
vcpu_put()?

When userspace calls KVM_SET_ONE_REG for CPTR_EL2, the ioctl is wrapped in
vcpu_load() and vcpu_put(). vcpu_load() loads the old memory state into the
physical CPU. 

Then, because CPTR_EL2 lacks a custom .set_user handler,
kvm_sys_reg_set_user() falls back to updating the in-memory array:

    if (r->set_user) {
        ret = (r->set_user)(vcpu, r, val);
    } else {
        __vcpu_assign_sys_reg(vcpu, r->reg, val);
        ret = 0;
    }

This updates the in-memory array, but not the physical hardware register.

When vcpu_put() is called, this newly added code in
__sysreg_save_vel2_state() reads the stale old value from the physical
hardware register and overwrites the newly set in-memory value, silently
destroying the userspace modification.

Could a custom .set_user handler be needed for CPTR_EL2 to ensure the
physical CPU register is updated when modified by userspace?

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