Re: [PATCH v2 22/28] KVM: arm64: Set Access flag on table descriptors at stage-1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: arm64: missing kvm_has_tcr2() check in effective_tcr2()
--

commit bcde4120f508adf06668822f4f97b91e98f978ff
Author: Oliver Upton <oupton@kernel.org>

KVM: arm64: Set Access flag on table descriptors at stage-1

This commit implements FEAT_HAFT stage-1 access flag updates on table
descriptors during software page table walks.

> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 2ce34af3b9f20..8f1be3af64e88 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -414,6 +414,8 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  	wi->hd &= (wi->regime == TR_EL2 ?
>  		  FIELD_GET(TCR_EL2_HD, tcr) :
>  		  FIELD_GET(TCR_HD, tcr));
> +	wi->haft  = kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, HAFDBS, HAFT) && wi->ha &&
> +		    FIELD_GET(TCR2_EL1_HAFT, effective_tcr2(vcpu, wi->regime));

[Severity: Medium]
Can this read a stale physical SYS_TCR2_EL12 register value if userspace
disables FEAT_TCR2 (ID_AA64MMFR3_EL1.TCRX = 0) while leaving
ID_AA64MMFR1_EL1.HAFDBS set to HAFT?

ID_AA64MMFR1_EL1 and ID_AA64MMFR3_EL1 are independently writable from
userspace via KVM_SET_ONE_REG and are not cross-sanitized in
kvm_vm_finalize_sys_regs(). When kvm_has_tcr2(vcpu->kvm) is false,
ctxt_has_tcrx() returns false in arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h:

static inline bool ctxt_has_tcrx(struct kvm_cpu_context *ctxt)
{
	struct kvm_vcpu *vcpu;

	if (!cpus_have_final_cap(ARM64_HAS_TCR2))
		return false;

	vcpu = ctxt_to_vcpu(ctxt);
	return kvm_has_tcr2(kern_hyp_va(vcpu->kvm));
}

As a result, __sysreg_restore_el1_state() and __sysreg_restore_vel2_state()
do not context-switch the physical SYS_TCR2_EL12 register when loading the
vCPU.

However, effective_tcr2() in arch/arm64/kvm/at.c does not check
kvm_has_tcr2(vcpu->kvm):

static u64 effective_tcr2(struct kvm_vcpu *vcpu, enum trans_regime regime)
{
	if (regime == TR_EL10) {
		if (vcpu_has_nv(vcpu) &&
		    !(__vcpu_sys_reg(vcpu, HCRX_EL2) & HCRX_EL2_TCR2En))
			return 0;

		return vcpu_read_sys_reg(vcpu, TCR2_EL1);
	}

	return vcpu_read_sys_reg(vcpu, TCR2_EL2);
}

When setup_s1_walk() runs while SYSREGS_ON_CPU is set (for example, in
TR_EL2/TR_EL20 via handle_at_s1e2() -> __kvm_at_s1e2() -> handle_at_slow() ->
setup_s1_walk() or kvm_s2_handle_perm_fault() -> __kvm_translate_va() ->
setup_s1_walk(), or for a non-NV guest in TR_EL10 via kvm_inject_sea() ->
inject_abt64() -> __kvm_find_s1_desc_level() -> setup_s1_walk()),
vcpu_read_sys_reg() resolves to read_sr_from_cpu() in
arch/arm64/kvm/sys_regs.c:

	case TCR2_EL1:		val = read_sysreg_s(SYS_TCR2_EL12);	break;

This reads whatever SYS_TCR2_EL12 value was left on the physical CPU by a
previous vCPU, which could set wi->haft and cause walk_s1() ->
handle_desc_update() to set PTE_AF on guest stage-1 table descriptors.

Should effective_tcr2() check kvm_has_tcr2(vcpu->kvm) first (similar to
__kvm_at_s1e01_fast() and __effective_sctlr2_bit()), or should setup_s1_walk()
gate wi->haft on kvm_has_tcr2(vcpu->kvm)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023205610.1044532-1-oupton@kernel.org?part=22