Re: [PATCH v2 12/13] KVM: arm64: Add hardware-accelerated dirty-ring cleaning routine

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unvalidated userspace memory accesses in dirty ring clearing lead to host DoS and memory corruption.
- [High] Missing TLB invalidation and DSB in dirty ring hardware path.
- [High] Hardware buffer allocation lacks strict physical alignment guarantees.

Pre-existing issues:
- [High] Missing architecturally required context synchronization (ISB) after control register writes.
- [High] Unsafe wfi() polling loop causes lost wakeups and CPU lockups.
- [Medium] Hardware dirty log clearing erroneously falls back to software due to incorrect return value processing.
--

commit 9f463875fd0f7eddcaa7053b557d7a2cc21e83c1
Author: Leonardo Bras <leo.bras@arm.com>

KVM: arm64: Add hardware-accelerated dirty-ring cleaning routine

This commit implements an arm64 version of kvm_arch_dirty_ring_clear using
FEAT_HACDBS. It translates dirty-ring entries into HDBSS entries and uses
the hardware mechanism to mark writable-dirty pages as clean. Invalid
entries are then cleared, falling back to software cleaning on error.

> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> --- a/arch/arm64/kvm/dirty_bit.c
> +++ b/arch/arm64/kvm/dirty_bit.c
[ ... ]
> static int dirty_bit_clear(struct kvm *kvm, u64 *hw_entries, int size)
> {
> 	u64 hcr_el2;
> 	int ret;
> 
> 	preempt_disable();
> 
> 	hcr_el2 = read_sysreg(HCR_EL2);
> 	write_sysreg(hcr_el2 | HCR_EL2_VM, HCR_EL2);
> 	__load_stage2(&kvm->arch.mmu);

[Severity: High]
This is a pre-existing issue, but does this missing ISB after control register
writes expose the pipeline to undefined architectural states?

Writing to HCR_EL2 requires an immediate context synchronization event.
Executing __load_stage2 immediately after without an isb means it might
operate under a stale configuration.

> 
> 	hacdbs_start(hw_entries, size);
> 
> 	do {
> 		wfi();
> 	} while (this_cpu_read(hacdbs_pcp.status) == HACDBS_RUNNING);

[Severity: High]
This is a pre-existing issue, but can this unprotected wfi wait loop cause
a lost wakeup and an indefinite CPU stall?

Because local interrupts are not masked around this check and wait, a timely
interrupt could satisfy the condition prematurely before wfi executes.

[ ... ]
> 	if (idx != 0) {
> 		ret = dirty_bit_clear(kvm, hw_entries, idx);
> 		*flush = *flush || ret > 0;
> 	}
> out_err:
> 	if (unlikely(ret != idx)) {

[Severity: Medium]
This is a pre-existing issue, but does this logic incorrectly evaluate a
successful batch outcome as an error?

If the loop finishes exactly on a batch boundary, idx is reset to 0, but ret
holds the success count of the previous batch. This causes the mismatch check
to evaluate to true, forcing a -EAGAIN return and triggering an unnecessary
software fallback.

[ ... ]
> @@ -256,6 +256,72 @@ int __kvm_arch_dirty_log_clear(struct kvm *kvm,
>  	return ret;
>  }
>  
> +int __kvm_arch_dirty_ring_clear(struct kvm *kvm, struct kvm_dirty_ring *ring,
> +				int *nr_entries_reset)
> +{
> +	u64 *hw_entries;
> +	u64 slot_offset = 0;
> +	u64 ttwl;
> +	int i, ret;
> +	u32 slot = -1;
> +
> +	if (signal_pending(current))
> +		return -EINTR;
> +
> +	ttwl = HDBSS_ENTRY_TTWL(KVM_PGTABLE_LAST_LEVEL);
> +
> +	hw_entries = kmalloc(max(ring->size * sizeof(u64), PAGE_SIZE), GFP_KERNEL);

[Severity: High]
Does this kmalloc call lack the strict physical alignment guarantees required
by the hardware accelerator?

Since kmalloc does not guarantee natural physical alignment for large buffers,
is it possible for physical address truncation to occur? This could cause the
hardware accelerator to parse unaligned memory and corrupt arbitrary page DBM
bits.

> +	if (!hw_entries)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ring->size; i++) {
> +		struct kvm_dirty_gfn *entry;
> +		gfn_t gfn;
> +
> +		entry = &ring->dirty_gfns[(ring->reset_index + i) &
> +					  (ring->size - 1)];
> +
> +		if (!kvm_dirty_gfn_harvested(entry))
> +			break;
> +
> +		if (entry->slot != slot) {
> +			struct kvm_memory_slot *memslot;
> +
> +			memslot = kvm_dirty_ring_get_memslot(kvm, entry->slot);
> +			slot = entry->slot;
> +			slot_offset = memslot->base_gfn;

[Severity: Critical]
Can an invalid, user-provided entry->slot lead to a NULL pointer dereference
here?

The memslot pointer is dereferenced directly to read base_gfn without a NULL
check.

> +		}
> +
> +		gfn = slot_offset + entry->offset;

[Severity: Critical]
Is entry->offset validated against the memslot bounds?

Using an unvalidated user-provided offset here allows arbitrary Stage-2 GPA
computation. This could enable Stage-2 memory corruption or privilege
escalation.

> +
> +		hw_entries[i] = (gfn_to_gpa(gfn) & HDBSS_ENTRY_IPA) |
> +				ttwl | HDBSS_ENTRY_VALID;
> +	}
> +
> +	ret = dirty_bit_clear(kvm, hw_entries, i);

[Severity: High]
Does this path miss an architecturally required TLB invalidation and DSB?

dirty_bit_clear relies on the caller to handle the TLBI (as noted in its
internal comments for the log clearing path), but this ring clearing routine
appears to jump directly to invalidating the entries and returning.

This could leave stale writable TLB entries in the guest.

> +
> +	/* Set as invalid all successfully cleaned entries */
> +	for (int j = 0; j < ret; j++) {
[ ... ]

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