Re: [PATCH v2 13/13] KVM: arm64: Enable KVM_HW_DIRTY_BIT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Malicious userspace can trigger a host kernel panic by providing an invalid memslot ID in the dirty ring.
- [High] Unbounded stall in `dirty_bit_clear()` due to a missed wakeup race with `wfi()`.
- [High] Missing `isb()` immediately after writes to control-plane system registers `HCR_EL2` and `SYS_HACDBSCONS_EL2`.
- [High] Off-by-one error in `split_end` calculation skips huge page splits for subsequent PMD blocks.
- [High] kvm_arch_dirty_clear_enabled() uses this_cpu_read() without preemption disabled.
--

commit 7b3668eabb9d9e558d0cab4dd205a0b0b5c74a14
Author: Leonardo Bras <leo.bras@arm.com>

KVM: arm64: Enable KVM_HW_DIRTY_BIT

This commit sets the Kconfig flag to enable hardware accelerated
dirty-bitmap and dirty-ring cleaning for arm64 when ACPI is enabled.

> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 449154f9a4852..db8487bf738b7 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -34,6 +34,7 @@ menuconfig KVM
>  	select HAVE_KVM_IRQ_BYPASS
>  	select HAVE_KVM_READONLY_MEM
>  	select HAVE_KVM_VCPU_RUN_PID_CHANGE
> +	select HAVE_KVM_HW_DIRTY_BIT if ACPI
>  	select SCHED_INFO
>  	select GUEST_PERF_EVENTS if PERF_EVENTS
>  	select KVM_GUEST_MEMFD

I noticed a few preexisting issues in the hardware dirty tracking
implementation enabled by this configuration.

arch/arm64/kvm/dirty_bit.c:dirty_bit_clear() {
    ...
    do {
        wfi();
    } while (this_cpu_read(hacdbs_pcp.status) == HACDBS_RUNNING);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but can this wfi() sleep
unconditionally without disabling local interrupts first?

If the HACDBS hardware finishes and its interrupt is handled immediately
after hacdbs_start() but before the wfi() instruction, wouldn't the wakeup
event be missed, causing a regression with an unbounded stall?


arch/arm64/kvm/dirty_bit.c:__kvm_arch_dirty_ring_clear() {
    ...
    memslot = kvm_dirty_ring_get_memslot(kvm, entry->slot);
    slot = entry->slot;
    slot_offset = memslot->base_gfn;
    ...
}

[Severity: Critical]
This is a pre-existing issue, but is it safe to blindly dereference memslot
here without checking if kvm_dirty_ring_get_memslot() returned NULL?

If unprivileged userspace provides an invalid or out-of-bounds slot ID in
the dirty ring, kvm_dirty_ring_get_memslot() will return NULL, which would
trigger a panic regression when memslot->base_gfn is accessed.


arch/arm64/kvm/dirty_bit.c:dirty_bit_clear() {
    ...
    write_sysreg(hcr_el2 | HCR_EL2_VM, HCR_EL2);
    __load_stage2(&kvm->arch.mmu);
    ...
}

arch/arm64/kvm/dirty_bit.c:hacdbs_start() {
    ...
    sysreg_clear_set_s(SYS_HACDBSCONS_EL2,
                       HACDBSCONS_EL2_ERR_REASON | HACDBSCONS_EL2_INDEX, 0);

    br = (virt_to_phys(hw_entries) & HACDBSBR_EL2_BADDR_MASK) |
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but do these writes to HCR_EL2
and SYS_HACDBSCONS_EL2 need an immediate isb()?

In dirty_bit_clear(), HCR_EL2 is written and immediately followed by
__load_stage2(). In hacdbs_start(), SYS_HACDBSCONS_EL2 is written without
an immediate isb(). Could this cause a regression where the CPU executes
subsequent instructions with architecturally undefined pipeline state?


arch/arm64/kvm/dirty_bit.c:__kvm_arch_dirty_log_clear() {
    ...
    kvm_mmu_split_huge_pages(kvm, start, end);
    split_end = gpa_to_gfn(ALIGN_DOWN(end, PMD_SIZE) + PMD_SIZE - 1);
    ...
}

[Severity: High]
This is a pre-existing issue, but does the calculation for split_end have
an off-by-one error?

If the dirtied range perfectly aligns with a PMD boundary (where end is a
multiple of PMD_SIZE), split_end would advance to cover the entire next
PMD block.

Subsequent iterations would then evaluate offset + BITS_PER_LONG > split_end
as false, which skips the kvm_mmu_split_huge_pages() call for that next
block.


arch/arm64/include/asm/kvm_dirty_bit.h:kvm_arch_dirty_clear_enabled() {
    ...
    static inline bool kvm_arch_dirty_clear_enabled(struct kvm *kvm)
    {
        return this_cpu_read(hacdbs_pcp.status) == HACDBS_IDLE &&
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does
kvm_arch_dirty_clear_enabled() use this_cpu_read() in a preemptible
context?

This function is called from kvm_arch_dirty_log_clear() and
kvm_arch_dirty_ring_clear(), where preemption is enabled. Could the thread
migrate to another CPU right after the check, leading to a regression where
dirty clearing logic runs on a CPU with the hardware scanner disabled?

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