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

[sashiko-bot@kernel.org]

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

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

KVM: arm64: nv: Only shadow writable-dirty guest descs as writable

This commit restricts shadow stage-2 and shadow VNCR translations to
read-only for writable-clean guest descriptors in preparation for emulating
hardware dirty state management for nested guests.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 15c22a2186095..9b2ea83aeccd2 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c

[ ... ]

> @@ -1587,6 +1589,11 @@ static u64 read_vncr_el2(struct kvm_vcpu *vcpu)
>  	return (u64)sign_extend64(__vcpu_sys_reg(vcpu, VNCR_EL2), 48);
>  }
>  
> +static bool vncr_can_map_writable(struct vncr_tlb *vt)
> +{
> +	return vt->wr.pw && vt->wr.dirty && vt->hpa_writable;
> +}
> +
>  static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
>  {
>  	struct kvm_memory_slot *memslot;

[ ... ]

> @@ -1781,7 +1788,7 @@ int kvm_handle_vncr_abort(struct kvm_vcpu *vcpu)
>  		kvm_inject_nested_sync(vcpu, esr);
>  		break;
>  	case 0:
> -		perm = kvm_is_write_fault(vcpu) ? vt->wr.pw && vt->hpa_writable : vt->wr.pr;
> +		perm = kvm_is_write_fault(vcpu) ? vncr_can_map_writable(vt) : vt->wr.pr;
>  		if (!perm)
>  			handle_vncr_perm(vcpu);
>  		break;

[Severity: Medium]
Does handle_vncr_perm() in arch/arm64/kvm/nested.c also need to check
vt->wr.dirty when deciding whether to promote a write fault to an external
abort?

With FEAT_S1PIE enabled at guest EL2, compute_s1_indirect_permissions()
computes vt->wr.pw = true from PIR_EL2 and the descriptor's permission index
independently of BIT(7), while compute_s1_permissions() sets vt->wr.dirty =
false when BIT(7) is set on a writable-clean stage-1 descriptor.

When a nested guest takes a VNCR write fault on such a writable-clean
descriptor backed by a read-only memslot or HPA (!vt->hpa_writable),
kvm_handle_vncr_abort() sees !vncr_can_map_writable(vt) and calls
handle_vncr_perm():

arch/arm64/kvm/nested.c:handle_vncr_perm() {
	...
	if (kvm_is_write_fault(vcpu) && vt->wr.pw && !vt->hpa_writable)
		fsc = ESR_ELx_FSC_EXTABT;
	/*
	 * Otherwise, inject a permission fault using the guest's translation
	 * level rather than the host's.
	 */
	else
		fsc = ESR_ELx_FSC_PERM_L(vt->wr.level);
	...
}

Because handle_vncr_perm() only checks vt->wr.pw && !vt->hpa_writable
without checking vt->wr.dirty, won't it inject ESR_ELx_FSC_EXTABT instead of
the architectural stage-1 permission fault ESR_ELx_FSC_PERM_L(vt->wr.level)?

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