Re: [PATCH v2 1/2] KVM: arm64: Only consider S1PTW a write fault if HA is set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditionally treating all S1PTW faults as write faults when HA=1 breaks pre-populated guest page tables in read-only memslots.
- [High] `effective_tcr_ha()` incorrectly evaluates the Hardware Access (HA) bit for unprivileged memory accesses (e.g., `LDTR`) at EL2, leading to potential infinite fault loops.
--

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

KVM: arm64: Only consider S1PTW a write fault if HA is set

This commit works around the hardware not always setting the WnR bit for 
stage-1 page table walk (S1PTW) faults. It introduces effective_tcr_ha() 
to assume an S1PTW fault is a write if the Hardware Access (HA) flag is 
enabled at stage-1.

Link: https://lore.kernel.org/kvmarm/20260623213225.A89CF1F000E9@smtp.kernel.org/

> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 5bf3d7e1d92c7..8e208ce2597e5 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h

[ ... ]

> @@ -479,21 +479,13 @@ static __always_inline int kvm_vcpu_sys_get_rt(struct kvm_vcpu *vcpu)
>  
>  static inline bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
>  {
> -	if (kvm_vcpu_abt_iss1tw(vcpu)) {
> -		/*
> -		 * Only a permission fault on a S1PTW should be
> -		 * considered as a write. Otherwise, page tables baked
> -		 * in a read-only memslot will result in an exception
> -		 * being delivered in the guest.
> -		 *
> -		 * The drawback is that we end-up faulting twice if the
> -		 * guest is using any of HW AF/DB: a translation fault
> -		 * to map the page containing the PT (read only at
> -		 * first), then a permission fault to allow the flags
> -		 * to be set.
> -		 */
> -		return kvm_vcpu_trap_is_permission_fault(vcpu);
> -	}
> +	/*
> +	 * The architecture sucks; assume that the S1PTW fetched for write if
> +	 * HA is enabled at stage-1. Note that hardware updates to dirty state
> +	 * and table AF are predicated on HA=1 (DDI0487 M.a D24.2.194; R_SNVTX).
> +	 */
> +	if (kvm_vcpu_abt_iss1tw(vcpu))
> +		return effective_tcr_ha(vcpu);

[Severity: High]
Does unconditionally treating S1PTW faults as writes when HA is enabled break
guests that use pre-populated page tables in read-only memslots?

If a guest populates its page tables with the Access Flag already set, places
them in an RO memslot, and enables HA in its TCR, the hardware only needs read
access during a walk. However, if a translation fault occurs,
kvm_is_write_fault() will unconditionally evaluate to true because of
effective_tcr_ha().

This will cause kvm_handle_guest_abort() to see a write fault on an RO memslot
and inject a spurious Synchronous External Abort (SEA) into the guest, even
though the hardware only needed to read the page table. Could we retain the
conditional check for permission faults to avoid this?

> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 8263c648207b0..91154654210ec 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c

[ ... ]

> +static bool __effective_tcr_ha(struct kvm_vcpu *vcpu, enum trans_regime regime)
> +{
> +	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, HAFDBS, AF))
> +		return false;
> +
> +	switch (regime) {
> +	case TR_EL10:
> +		return vcpu_read_sys_reg(vcpu, TCR_EL1) & TCR_HA;
> +	case TR_EL20:
> +		return vcpu_read_sys_reg(vcpu, TCR_EL2) & TCR_HA;
> +	case TR_EL2:
> +		return vcpu_read_sys_reg(vcpu, TCR_EL2) & TCR_EL2_HA;
> +	default:
> +		BUG();
> +	}
> +}
> +
> +static enum trans_regime vcpu_trans_regime(struct kvm_vcpu *vcpu)
> +{
> +	if (!is_hyp_ctxt(vcpu))
> +		return TR_EL10;
> +
> +	return vcpu_el2_e2h_is_set(vcpu) ? TR_EL20 : TR_EL2;
> +}

[Severity: High]
Does vcpu_trans_regime() correctly handle unprivileged memory accesses 
like LDTR/STTR executed from EL2?

By returning TR_EL2 based strictly on the vCPU's execution context, KVM
disregards that unprivileged instructions at EL2 use the EL1 and 0 translation
regime (TCR_EL1) when HCR_EL2.E2H is 0 or TGE is 0.

Since KVM cannot distinguish an LDTR fault from an LDR fault via the ESR
syndrome, it will read TCR_EL2.HA in __effective_tcr_ha() instead of
TCR_EL1.HA. If TCR_EL1.HA is 1 but TCR_EL2.HA is 0, KVM incorrectly evaluates
HA as disabled.

This causes KVM to treat a legitimate S1PTW permission fault (generated to
update the Access Flag) as a read fault, refusing to grant write permissions.
The hardware will then fault again on retry, creating an infinite stage-2 fault
loop that hangs the guest vCPU.

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