Re: [PATCH 04/22] KVM: arm64: nv: Only shadow writable-dirty guest descs as writable

[sashiko-bot@kernel.org]

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

New issues:
- [High] Uninitialized or incorrectly evaluated `wr->dirty` field during Stage-1 page table walks causes an infinite VNCR trap loop.

Pre-existing issues:
- [High] Write-Only VNCR mappings cause an infinite trap loop because KVM falls back to `PAGE_KERNEL_NONE` but considers write faults resolved.
--

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

KVM: arm64: nv: Only shadow writable-dirty guest descs as writable
    
KVM will soon add support for hardware dirty state management for nested
guests. In order to emulate dirty state transitions on the guest
desciptor, KVM will need to use read-only hardware mappings and re-walk
the guest page tables upon taking a permission fault.
    
Prepare by limiting shadow stage-2 and shadow VNCR translations to
read-only for writable-clean guest translations.

> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 86b499e7a9a06..7a84495a2e6d4 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
[ ... ]
> @@ -1317,6 +1317,7 @@ static void compute_s1_permissions(struct kvm_vcpu *vcpu,
>  			  (pan3_enabled(vcpu, wi->regime) && wr->ux));
>  	wr->pw &= !pan;
>  	wr->pr &= !pan;
> +	wr->dirty = !(wr->desc & BIT(7));

[Severity: High]
Could the unconditional use of BIT(7) here cause issues when S1PIE is
enabled?

With S1PIE, BIT(7) becomes PIIndex[3], so using it to evaluate the dirty
state might be incorrect.

Also, if the Stage-1 MMU is disabled, __kvm_translate_va() returns early
without initializing wr->dirty:

arch/arm64/kvm/at.c:__kvm_translate_va() {
        if (wr->level == S1_MMU_DISABLED) {
                wr->ur = wr->uw = wr->ux = true;
                wr->pr = wr->pw = wr->px = true;
                return 0;
        }
        ...
}

Since wr->dirty remains false (0-initialized), could this lead to an
infinite VNCR trap loop? If wr->dirty is false, the shadow fixmap mapping
is downgraded to PAGE_KERNEL_RO in kvm_map_l1_vncr().

When a guest writes to the redirected register, kvm_handle_vncr_abort()
sees vt->wr.pw is true, considers the fault handled without injecting a
permission fault, and the hardware immediately retries and traps again.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index b247bc1d83fa7..dcc7d0cc7c952 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
[ ... ]
> @@ -1603,7 +1605,7 @@ static void kvm_map_l1_vncr(struct kvm_vcpu *vcpu)
>  
>  	vt->cpu = smp_processor_id();
>  
> -	if (vt->hpa_writable && vt->wr.pw && vt->wr.pr)
> +	if (vt->hpa_writable && vt->wr.pw && vt->wr.dirty && vt->wr.pr)
>  		prot = PAGE_KERNEL;
>  	else if (vt->wr.pr)
>  		prot = PAGE_KERNEL_RO;

[Severity: High]
This is a pre-existing issue, but how does this code handle Write-Only VNCR
mappings?

If a nested guest maps its VNCR page as Write-Only (which is possible using
POE/S1PIE), vt->wr.pr will be false. The logic here then falls through to
mapping it as PAGE_NONE:

arch/arm64/kvm/nested.c:kvm_map_l1_vncr() {
        ...
        else if (vt->wr.pr)
                prot = PAGE_KERNEL_RO;
        else
                prot = PAGE_NONE;
        ...
}

When the guest writes to the register, kvm_handle_vncr_abort() sees
vt->wr.pw is true, assumes the write fault is permitted, and returns 1
without injecting a fault.

The hardware then retries against PAGE_NONE and immediately traps again,
creating an infinite loop that can lock up the host CPU.

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