Re: [PATCH 14/22] KVM: arm64: Grant write permission when DBM is set at S1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch incorrectly grants write permissions based on DBM when hardware dirty state management is architecturally disabled due to HA=0.
- [High] Hardware dirty state management (FEAT_HAFDBS) is incorrectly emulated when the Permission Indirection Extension (FEAT_S1PIE) is enabled, leading to spurious permission faults and incorrect descriptor updates.
--

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

KVM: arm64: Grant write permission when DBM is set at S1

The DBM bit effectively behaves as a permission bit when using direct
permissions, especially so in the context of accesses that do not update
the dirty state (like AT).

> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index bfba31b270a95..faffb69c2eef5 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -1077,11 +1077,23 @@ static void compute_s1_direct_permissions(struct kvm_vcpu *vcpu,
>  					  struct s1_walk_info *wi,
>  					  struct s1_walk_result *wr)
>  {
> +	u8 ap = FIELD_GET(PTE_USER | PTE_RDONLY, wr->desc);
>  	bool wxn;
>  
> +	/*
> +	 * Descriptors with the DBM bit set while hardware dirty state are
> +	 * considered writable, even though certain accesses (like AT instructions)
> +	 * don't actually update the dirty state.
> +	 *
> +	 * Assume that walk_s1() made the necessary descriptor updates for the
> +	 * access and just treat DBM as writable here.
> +	 */
> +	if (wi->hd && (wr->desc & PTE_DBM))
> +		ap &= ~BIT(1);

[Severity: High]
Does this incorrectly grant write permissions when hardware management of the
Access Flag is disabled?

According to the ARM Architecture Reference Manual, hardware management of the
dirty state (TCR_ELx.HD = 1) is ignored if hardware management of the Access
Flag is disabled (TCR_ELx.HA = 0).

In setup_s1_walk(), we compute wi->hd directly from TCR.HD without enforcing
this requirement:

arch/arm64/kvm/at.c:setup_s1_walk() {
        ...
        wi->hd  = kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, HAFDBS, DBM);
        wi->hd &= (wi->regime == TR_EL2 ?
                  FIELD_GET(TCR_EL2_HD, tcr) :
                  FIELD_GET(TCR_HD, tcr));
        ...
}

If a guest OS configures TCR_ELx.HA = 0 and TCR_ELx.HD = 1, and creates a
stage-1 descriptor with RDONLY=1 and DBM=1, it should remain strictly
read-only and generate a Permission Fault on a write. By relying on the
ungated wi->hd here, does KVM bypass the Permission Fault and allow a
guest-visible privilege escalation?

[Severity: High]
Is hardware dirty state management incorrectly emulated when the Permission
Indirection Extension (FEAT_S1PIE) is enabled?

When FEAT_S1PIE is enabled, the DBM bit (bit 51) acts as Index[1] in the PI
index. If a write occurs to a "clean" page (DBM=1) and HD=1, the hardware
checks if clearing bit 51 would grant write access.

It looks like compute_s1_indirect_permissions() evaluates permissions using
the current PI index without simulating this dirty bit transition, which could
cause a spurious permission fault instead of updating the dirty state:

arch/arm64/kvm/at.c:compute_s1_indirect_permissions() {
        ...
        idx = pte_pi_index(wr->desc);
        ...
        set_perms(priv, wr, pp);
        ...
}

Additionally, if the dirty state needs to be updated, it appears we
unconditionally clear bit 7 (PTE_RDONLY) instead of bit 51 (PTE_DBM):

arch/arm64/kvm/at.c:handle_desc_update() {
        ...
        if (should_set_dirty_state(wi, ws, wr, access))
                new &= ~PTE_RDONLY;
        ...
}

For PIE, bit 7 is unrelated to the dirty state. Could this corrupt the
guest's PIE descriptor?

>  
>  	/* Non-hierarchical part of AArch64.S1DirectBasePermissions() */
>  	if (wi->regime != TR_EL2) {
> -		switch (FIELD_GET(PTE_USER | PTE_RDONLY, wr->desc)) {
> +		switch (ap) {
>  		case 0b00:
>  			wr->pr = wr->pw = true;
>  			wr->ur = wr->uw = false;

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