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

[Leonardo Bras <leo.bras@arm.com>]

On Mon, Jun 29, 2026 at 11:34:56AM +0000, sashiko-bot@kernel.org wrote:
> 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?
> 

Copied from HDBSS patchset, so notes for Tian Zheng ^

> > +}
> 
> [ ... ]
> 
> > 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.
> 

Is there any case where we want to write protect a page that was marked as 
writeable, but not for dirty-tracking? 
If so, we need to also clear DBM for that, and make sure it is done only 
in those cases. 

The whole idea of HAFDBS / HDBSS is that the page is not actually write 
protected, it just doing dirty-detection, and this is done much much 
faster.

Thanks!
Leo