Re: [PATCH v2 2/2] KVM: arm64: nv: Treat S1PTW permission faults specially

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] An L1 hypervisor can cause an infinite page fault loop in the L0 hypervisor by creating an L1 S2 PTE with XN=0 (execution allowed) but AP=00 (read/write denied). L0 fails to forward the resulting instruction abort to L1, repeatedly mapping the shadow PTE without read permissions, which the hardware continually rejects as non-executable.
--

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

KVM: arm64: nv: Treat S1PTW permission faults specially

Permission faults reported as instruction aborts with S1PTW set are due
to missing read/write permissions for the table walk, not execute
permissions on the output of translation. This patch adds logic to evaluate
both and forward the fault to the L1 hypervisor if either fails.

Closes: https://lore.kernel.org/kvmarm/20260623190607.7106B1F000E9@smtp.kernel.org/

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 94df26de6990c..4c9123cb2e1c5 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
[ ... ]
> @@ -918,14 +919,27 @@ int kvm_s2_handle_perm_fault(struct kvm_vcpu *vcpu, struct kvm_s2_trans *trans)
>  	if (!kvm_vcpu_trap_is_permission_fault(vcpu))
>  		return 0;
>  
> -	if (kvm_vcpu_trap_is_iabt(vcpu)) {
> +	/*
> +	 * S1PTW permission faults do not provide sufficient syndrome information
> +	 * to determine if the fault was for read or write permissions. Perform a
> +	 * read permission check and an optional write permission check, relying
> +	 * on the fact that:
> +	 *
> +	 *  - The table walker at minimum requires read permission
> +	 *
> +	 *  - The L1 hypervisor also needs to deal with the architecture and
> +	 *    cannot directly infer the failing permission from the fault context
> +	 */
> +	if (kvm_vcpu_abt_iss1tw(vcpu)) {
> +		forward_fault = !trans->readable;
> +		if (write_fault)
> +			forward_fault |= !trans->writable;
> +	} else if (kvm_vcpu_trap_is_iabt(vcpu)) {
>  		if (vcpu_mode_priv(vcpu))
>  			forward_fault = !kvm_s2_trans_exec_el1(vcpu->kvm, trans);
>  		else
>  			forward_fault = !kvm_s2_trans_exec_el0(vcpu->kvm, trans);

[Severity: High]
This is a pre-existing issue, but does this logic miss a read-permission
dependency check for normal instruction aborts?

If an L1 hypervisor maps a page for L2 with Stage-2 PTE AP=00 (no read/write)
and XN=0 (executable), and L2 attempts to fetch an instruction, the hardware
generates a Stage-2 Instruction Abort (Permission Fault) because execution
requires read permission.

Here, kvm_s2_handle_perm_fault() intercepts the IABT and checks only the XN
bits via kvm_s2_trans_exec_el1() and kvm_s2_trans_exec_el0(). Since they
only check the XN bits and ignore trans->readable, forward_fault is set to
false.

L0 then handles the fault by mapping the shadow S2 PTE. The shadow fault
handler will clear the read protection bit, leaving the execute protection
bit intact. The hardware will reject this shadow mapping as non-executable
due to missing read permissions, triggering another IABT upon guest
resumption and causing an infinite page fault loop in the L0 hypervisor.

>  	} else {
> -		bool write_fault = kvm_is_write_fault(vcpu);
> -
>  		forward_fault = ((write_fault && !trans->writable) ||
>  				 (!write_fault && !trans->readable));
>  	}

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