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

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

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

See prev patches


> 
> > 
> > 	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.
> 

iden

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

iden

> [ ... ]
> > @@ -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.

I recall it had size-alignmen guarantees. Largest buffer size should be 
64k x 8B, so it should be ok.


> 
> > +	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;
> 

Correct. I have to verify the return value of get_memslot before using it.
Will do in v3

> [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.

Correct. Will verify the offset against memslot size before this.
Will be done in v3.

> 
> > +
> > +		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.

There is a dsb() and then a TLBI before returning to guest.

> 
> 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

Thanks!
Leo