Re: [PATCH v2 01/13] KVM: arm64: HDBSS bits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Mapping writeable pages with both DBM and S2AP_W set causes `kvm_pgtable_stage2_wrprotect` to silently bypass software write-protection.
- [High] `has_vhe_hdbss` incorrectly uses `is_kernel_in_hyp_mode()` which evaluates to true during early boot for pKVM (nVHE), erroneously enabling VHE-specific features.
- [Low] Typo in the parameter name for `has_vhe_hdbss`.
--

commit 49089779295fae526ba79b7c7e78d31fbe6a9636
Author: Leonardo Bras <leo.bras@arm.com>

KVM: arm64: HDBSS bits

The commit introduces HDBSS bits from a future patchset to fulfill
dependencies and allow building.

> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 9a22df0c5120f..aa327eebaf1ca 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c

[ ... ]

> @@ -2131,6 +2131,11 @@ static bool hvhe_possible(const struct arm64_cpu_capabilities *entry,
>  	return arm64_test_sw_feature_override(ARM64_SW_FEATURE_OVERRIDE_HVHE);
>  }
>  
> +static bool has_vhe_hdbss(const struct arm64_cpu_capabilities *entry, int cope)
                                                                             ^^^^
[Severity: Low]
This isn't a bug, but there is a typo in the parameter name (cope instead
of scope).

> +{
> +	return is_kernel_in_hyp_mode() && has_cpuid_feature(entry, cope);

[Severity: High]
Does using is_kernel_in_hyp_mode() here erroneously enable VHE-specific
features for pKVM (nVHE)?

During early boot feature detection, is_kernel_in_hyp_mode() evaluates to
true because the host kernel boots at EL2 and only drops privileges to
EL1 later during device_initcall_sync (finalize_pkvm()).

This early check could force ARM64_HAS_HDBSS to be enabled on pKVM
systems, propagating the KVM_PGTABLE_S2_DBM flag to hypervisor-managed
stage-2 page tables.

Should this use is_vhe_hyp_mode() instead to avoid incorrectly matching
during the nVHE privileged window?

> +}

[ ... ]

> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 91a7dfad66866..e16729f0b7bd2 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c

[ ... ]

> @@ -731,9 +731,13 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p
>  	if (prot & KVM_PGTABLE_PROT_R)
>  		attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
>  
> -	if (prot & KVM_PGTABLE_PROT_W)
> +	if (prot & KVM_PGTABLE_PROT_W) {
>  		attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
>  
> +		if (pgt->flags & KVM_PGTABLE_S2_DBM)
> +			attr |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;

[Severity: Critical]
Can setting KVM_PTE_LEAF_ATTR_HI_S2_DBM here lead to a silent bypass of
software write-protection?

When a page is mapped writable, both DBM and S2AP_W are set. If KVM later
write-protects the page via kvm_pgtable_stage2_wrprotect(), only S2AP_W
is cleared:

kvm_pgtable_stage2_wrprotect() {
    ...
    return stage2_update_leaf_attrs(pgt, addr, size, 0,
                                    KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W,
                                    NULL, NULL,
                                    KVM_PGTABLE_WALK_IGNORE_EAGAIN);
}

This leaves DBM intact. Hardware interprets DBM=1, S2AP_W=0 as a clean
writable page and automatically sets S2AP_W=1 on the next write without
generating a Permission Fault.

For pKVM, this allows a guest to bypass memory isolation. For nested
virtualization, nested guest writes will silently set S2AP_W=1 without
trapping to the host, resulting in lost dirty pages during live migration.

> +	}
> +
>  	if (!kvm_lpa2_is_enabled())
>  		attr |= FIELD_PREP(KVM_PTE_LEAF_ATTR_LO_S2_SH, sh);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629111820.1873540-1-leo.bras@arm.com?part=1