Re: [PATCH v3 04/11] KVM: arm64: dirty_bit: Add base FEAT_HACDBS cleaning routine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] `hacdbs_sched_in` restores a potentially stale VMID from `save.vttbr_el2`, leading to cross-VM memory corruption.
- [High] Direct EL2 system register accesses from host code without a VHE check causes kernel panics on nVHE systems.
- [High] Writes to control-plane system registers are batched and not immediately followed by an `isb()` barrier.
- [High] Missing synchronization barrier (`DSB`) before changing the translation context after disabling the HACDBS asynchronous engine.
- [High] The hardware polling loop in `dirty_bit_clear` lacks a timeout or signal check, risking an unkillable task hang.
- [Medium] Using `virt_to_phys()` on a dynamic buffer array which could be non-contiguous (e.g. allocated with vmalloc).
- [Medium] The commit message explicitly asserts the code runs with preemption disabled, but the implementation explicitly enables preemption.
- [Low] Left-shift of a negative integer in macro definition.
--

--- Patch [4]: [PATCH v3 04/11] KVM: arm64: dirty_bit: Add base FEAT_HACDBS cleaning routine ---
commit b837f416f1871772a3bac546015e6fcac9c7accb
Author: Leonardo Bras <leo.bras@arm.com>

KVM: arm64: dirty_bit: Add base FEAT_HACDBS cleaning routine

Introduce the basic cleaning routine that is going to be used for both
dirty-bitmap and dirty-ring routines. It sets the required registers with
the input buffer, and wait for HACDBS to finish.

> It is ran with preemption disabled, as a task being scheduled in could
> change the translation registers used by HACDBS and end up corrupting the
> current dirty-bit tracking and the sched-in task's S2 pagetables.

[Severity: Medium]
The commit message asserts that the code runs with preemption disabled, but
looking at dirty_bit_clear(), preemption is explicitly enabled immediately
after registering the notifier:

dirty_bit_clear() {
    preempt_disable();
    preempt_notifier_register(&save.notifier);
    preempt_enable();
}

Is this description accurate, or does the implementation contradict the
commit message's synchronization premise?

> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> index 32fe938d6bf70..33826bbd16d91 100644
> --- a/arch/arm64/kvm/dirty_bit.c
> +++ b/arch/arm64/kvm/dirty_bit.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <asm/kvm_dirty_bit.h>
> +#include <asm/kvm_mmu.h>
>  
>  /* HDBSS entry field definitions */
>  #define HDBSS_ENTRY_VALID BIT(0)
> @@ -14,3 +15,178 @@
>  	(((x) << HDBSS_ENTRY_TTWL_SHIFT) & HDBSS_ENTRY_TTWL_MASK)
>  #define HDBSS_ENTRY_TTWL_RESV HDBSS_ENTRY_TTWL(-4)

[Severity: Low]
Does this code trigger undefined behavior by left-shifting a negative
signed integer (-4) via the HDBSS_ENTRY_TTWL macro?

[ ... ]

> +static void hacdbs_start(u64 *hw_entries, int size)
> +{
> +	u64 br;
> +	/* Each entry is 8 bytes */
> +	int size_b = size * sizeof(hw_entries[0]);
> +	int size_p2 = max(roundup_pow_of_two(size_b), PAGE_SIZE);
> +
> +	/* If not using the full size of the array, put a stop entry at the end */
> +	if (size_b < size_p2)
> +		hw_entries[size] = HDBSS_ENTRY_VALID | HDBSS_ENTRY_TTWL_RESV;
> +
> +	sysreg_clear_set_s(SYS_HACDBSCONS_EL2,
> +			   HACDBSCONS_EL2_ERR_REASON | HACDBSCONS_EL2_INDEX, 0);

[Severity: High]
Can this sequence leave the CPU executing with architecturally undefined
pipeline state?

In hacdbs_start(), the write to the control-plane system register
SYS_HACDBSCONS_EL2 is followed by pointer arithmetic and a dsb() rather
than an isb() as the very next instruction.

> +
> +	br = (virt_to_phys(hw_entries) & HACDBSBR_EL2_BADDR_MASK) |
> +	     FIELD_PREP(HACDBSBR_EL2_SZ, ilog2(size_p2) - 12) |
> +	     FIELD_PREP(HACDBSBR_EL2_EN, 1);

[Severity: Medium]
Will this return an invalid physical address if hw_entries is allocated via
vmalloc() or kvmalloc()?

Using virt_to_phys() on a dynamic buffer array assumes it resides in the
physically contiguous linear map, which could lead to hardware DMA targeting
the wrong physical memory pages.

> +
> +	/* All writes to the array have to complete before starting */
> +	dsb(ishst);
> +
> +	write_sysreg_s(br, SYS_HACDBSBR_EL2);

[Severity: High]
Does this code cause a kernel panic on nVHE systems?

In hacdbs_start(), directly writing to the EL2 system register
SYS_HACDBSBR_EL2 from the KVM host kernel executing at EL1 without a
has_vhe() guard will trigger an immediate Undefined Instruction trap.

> +	isb();
> +}

[ ... ]

> +static void hacdbs_sched_in(struct preempt_notifier *notifier, int cpu)
> +{
> +	struct hacdbs_sched_save *save = container_of(notifier,
> +						      struct hacdbs_sched_save,
> +						      notifier);
> +
> +	write_sysreg_s(save->save.vtcr_el2, SYS_VTCR_EL2);
> +	write_sysreg_s(save->save.vttbr_el2, SYS_VTTBR_EL2);

[Severity: High]
Could this direct EL2 register access cause a panic on nVHE systems?

Similar to hacdbs_start(), hacdbs_sched_in() accesses SYS_VTCR_EL2 and
SYS_VTTBR_EL2 from EL1 without a VHE check.

[Severity: High]
Is a synchronization barrier missing here?

In hacdbs_sched_in(), the write to SYS_VTCR_EL2 is immediately followed by a
write to SYS_VTTBR_EL2 rather than an isb(), meaning subsequent instructions
may observe undefined pipeline state.

[Severity: Critical]
Can this restore a stale VMID and cause cross-VM memory corruption?

hacdbs_sched_in() restores the cached save.vttbr_el2 (which contains the
VMID). Since this is a generic host thread, if KVM's VMID allocator rolls
over while the task sleeps, the VMID could be reassigned.

This could cause the HACDBS engine to operate on another VM's memory.

> +	isb();
> +
> +	if (save->save.hcr_vm_set) {
> +		sysreg_clear_set_hcr(0, HCR_EL2_VM);
> +		isb();
> +	}
> +
> +	write_sysreg_s(save->save.hacdbscons_el2, SYS_HACDBSCONS_EL2);
> +	write_sysreg_s(save->save.hacdbsbr_el2, SYS_HACDBSBR_EL2);

[Severity: High]
Does this sequence lack an immediate isb() after writing SYS_HACDBSCONS_EL2,
allowing the CPU to execute the SYS_HACDBSBR_EL2 write with undefined
pipeline state?

> +	isb();
> +}
> +
> +static void hacdbs_sched_out(struct preempt_notifier *notifier,
> +			     struct task_struct *next)
> +{
> +	struct hacdbs_sched_save *save = container_of(notifier,
> +						      struct hacdbs_sched_save,
> +						      notifier);
> +
> +	if (read_sysreg_s(SYS_HACDBSBR_EL2) & HACDBSBR_EL2_EN) {
> +		save->save.hacdbsbr_el2 = HACDBSBR_EL2_EN;
> +		sysreg_clear_set_s(SYS_HACDBSBR_EL2, HACDBSBR_EL2_EN, 0);
> +		isb();
> +	} else {
> +		save->save.hacdbsbr_el2 = 0;
> +	}
> +
> +	save->save.hacdbscons_el2 = read_sysreg_s(SYS_HACDBSCONS_EL2);
> +	save->save.hacdbsbr_el2 |= read_sysreg_s(SYS_HACDBSBR_EL2);
> +	save->save.vttbr_el2 = read_sysreg_s(SYS_VTTBR_EL2);

[Severity: Critical]
Is it safe to cache the VTTBR (including the VMID) into task-local memory
here in hacdbs_sched_out()?

As mentioned for hacdbs_sched_in(), if a VMID rollover occurs while this
task is preempted, the cached VMID becomes stale.

> +	save->save.vtcr_el2 = read_sysreg_s(SYS_VTCR_EL2);
> +
> +	if (read_sysreg_s(SYS_HCR_EL2) & HCR_EL2_VM) {
> +		sysreg_clear_set_hcr(HCR_EL2_VM, 0);

[Severity: High]
Does this code change the translation regime (clearing HCR_EL2_VM) without
a preceding dsb()?

The engine was disabled earlier in hacdbs_sched_out() (by clearing
HACDBSBR_EL2_EN), but without a dsb(), the asynchronous engine might not be
definitively halted before the translation context changes.

> +		isb();
> +		save->save.hcr_vm_set = true;
> +	} else {
> +		save->save.hcr_vm_set = false;
> +	}
> +}

[ ... ]

> +static int dirty_bit_clear(struct kvm *kvm, u64 *hw_entries, int size)
> +{
> +	int ret;
> +	u64 cons;
> +	struct hacdbs_sched_save save;
> +
> +	preempt_notifier_init(&save.notifier, &hacdbs_preempt_ops);
> +	preempt_disable();
> +	preempt_notifier_register(&save.notifier);
> +	preempt_enable();
> +
> +	__load_stage2(&kvm->arch.mmu);
> +	sysreg_clear_set_hcr(0, HCR_EL2_VM);
> +	isb();
> +
> +	hacdbs_start(hw_entries, size);
> +
> +	do {
> +		cons = read_sysreg_s(SYS_HACDBSCONS_EL2);
> +		if (FIELD_GET(HACDBSCONS_EL2_ERR_REASON, cons))
> +			break;
> +
> +		if (FIELD_GET(HACDBSCONS_EL2_INDEX, cons) >= size)
> +			break;
> +
> +		udelay(1);
> +	} while (true);

[Severity: High]
Could this continuous hardware polling loop hang the CPU permanently?

In dirty_bit_clear(), this loop waits for the hardware engine to finish, but
it lacks a timeout mechanism, bounded retry, or a fatal_signal_pending()
check.

> +
> +	ret = hacdbs_stop(size);
> +
> +	sysreg_clear_set_hcr(HCR_EL2_VM, 0);

[Severity: High]
Can this translation regime change lead to speculative execution or out of
context memory accesses?

In dirty_bit_clear(), hacdbs_stop() clears the enable bit, but clearing
HCR_EL2_VM immediately after lacks an intervening dsb() to ensure the
hardware engine is definitively halted.

> +	isb();
> +
> +	/*
> +	 * No DSB is needed here, as kvm_flush_remote_tlbs_memslot() that happens
> +	 * later in generic dirty-cleaning code already performs a DSB before
> +	 * doing the TLBI.
> +	 */
> +
> +	preempt_disable();
> +	preempt_notifier_unregister(&save.notifier);
> +	preempt_enable();
> +
> +	return ret;
> +}

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