Re: [PATCH v3 3/5] KVM: arm64: nv: Re-translate VNCR before injecting abort

[sashiko-bot@kernel.org]

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

New issues:
- [High] Guest Stage-1 permission faults on VNCR are incorrectly promoted to External Aborts when backed by a read-only memslot.
- [High] Removing the `kvm_vncr_tlb_lookup()` check introduces an infinite host CPU livelock for Write-Only guest VNCR mappings.

Pre-existing issues:
- [High] If `kvm_gmem_get_pfn()` returns `-EINVAL`, KVM drops the prepared userspace memory fault exit and injects a bogus nested exception.
--

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

    KVM: arm64: nv: Re-translate VNCR before injecting abort
    
    KVM faults in the VNCR page with FOLL_WRITE whenever the guest aborts
    for a write, similar to how a regular stage-2 mapping is handled. It is
    entirely possible that the guest reads from the VNCR before writing to
    it, in which case the PFN could only be read-only.
    
    Invalidate the VNCR TLB and re-fetch the translation upon taking a VNCR
    abort, allowing the host mapping to be faulted in for write the second
    time around. Interestingly enough, this also satisfies the ordering
    requirements of FEAT_ETS2/3 between descriptor updates and MMU faults.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 53dea9c3f14f8..7fffd86eee944 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
[ ... ]
> @@ -1466,88 +1466,61 @@ static void handle_vncr_perm(struct kvm_vcpu *vcpu)
[ ... ]
> +	ret = kvm_translate_vncr(vcpu, &is_gmem);
> +	switch (ret) {
> +	case -EAGAIN:
[ ... ]
> +	case -EFAULT:
> +	case -EIO:
> +	case -EHWPOISON:
> +		if (is_gmem)
> +			return 0;
> +		fallthrough;
> +	case -EINVAL:
> +	case -ENOENT:
> +	case -EACCES:
> +		/*
> +		 * Translation failed, inject the corresponding
> +		 * exception back to EL2.
> +		 */
> +		esr &= ~ESR_ELx_FSC;
> +		esr |= FIELD_PREP(ESR_ELx_FSC, vt->wr.fst);
>  
> -			kvm_inject_nested_sync(vcpu, esr);
> -			break;
> -		case -EPERM:
> -			/* Hack to deal with POE until we get kernel support */
> +		kvm_inject_nested_sync(vcpu, esr);
> +		break;

[Severity: High]
This is a pre-existing issue, but if kvm_gmem_get_pfn() fails and returns
-EINVAL, -ENOENT, or -EACCES, does this branch bypass the userspace exit?

If those errors are returned, they skip the is_gmem check because they bypass
the fallthrough block above. Instead of returning to userspace, KVM handles
them by injecting a nested exception back to the guest using an uninitialized
or stale value in vt->wr.fst. Does this destroy the memory fault exit state
and inject a bogus exception?

[ ... ]
> -			handle_vncr_perm(vcpu);
> -			break;
> -		case 0:
> -			break;
> -		}
> -	} else {
> -		WARN_ONCE(1, "Unhandled VNCR abort, ESR=%llx\n", esr);
> +	case 0:
> +		perm = kvm_is_write_fault(vcpu) ? vt->wr.pw && vt->hpa_writable : vt->wr.pr;
> +		if (!perm)
>  			handle_vncr_perm(vcpu);
> +		break;
>  	}
>  
>  	return 1;

[Severity: High]
Does removing the -EPERM loop breaker introduce a host CPU livelock for
Write-Only guest VNCR mappings?

If the guest configures a Write-Only mapping, kvm_map_l1_vncr() falls back
to mapping it as PAGE_NONE. When the hardware accesses the fixmap, it triggers
a Data Abort. KVM then traps and re-translates the mapping.

Because vt->wr.pw is true, the perm check evaluates to true, and KVM returns 1
to resume the guest instead of injecting a fault. The hardware will then fault
again indefinitely.

[Severity: High]
Also, does calling kvm_translate_vncr() unconditionally request FOLL_WRITE
on a write fault without verifying if the guest's own Stage-1 mapping
actually permits writes?

In kvm_translate_vncr(), __kvm_faultin_pfn() is called with FOLL_WRITE if it's
a write fault. If the host memslot happens to be read-only, __kvm_faultin_pfn()
fails and returns -EFAULT.

This error bubbles up to the switch statement above, which injects an External
Abort into the guest. Should the guest instead receive a Stage-1 Permission
Fault if its own Stage-1 translation does not permit writes, regardless of the
memslot permissions?

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