Re: [PATCH v2 07/13] kvm: Add arch-generic interface for hw-accelerated dirty-bitmap cleaning

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

On Mon, Jun 29, 2026 at 11:38:18AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Data loss (lost dirty pages) when kvm_arch_dirty_log_clear fails during KVM_GET_DIRTY_LOG.
> - [High] Out-of-bounds read of userspace buffer in kvm_arch_dirty_log_clear during KVM_CLEAR_DIRTY_LOG.
> --
> 
> commit ea34df3acf2397922e1a2152d707b3b9f6012964
> Author: Leonardo Bras <leo.bras@arm.com>
> 
> kvm: Add arch-generic interface for hw-accelerated dirty-bitmap cleaning
> 
> Introduce kvm_arch_dirty_log_clear() that allow implementation of
> arch-specific hardware-accelerated dirty-log routines.
> 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index e44c20c049610..a25b8902cdfc1 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -2262,6 +2263,10 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
> >  		dirty_bitmap_buffer = kvm_second_dirty_bitmap(memslot);
> >  		memset(dirty_bitmap_buffer, 0, n);
> >  
> > +		if (kvm_arch_dirty_log_clear(kvm, memslot, NULL,
> > +					     dirty_bitmap_buffer, &flush) >= 0)
> > +			goto out;
> > +
> 
> [Severity: Critical]
> If kvm_arch_dirty_log_clear() successfully clears some pages in hardware but
> subsequently fails and returns an error, will the fallback loop overwrite the
> successfully cleared bits?
> 
> It appears that if the arch hook returns an error, KVM falls back to the
> generic software loop which includes:
> 
>     mask = xchg(&dirty_bitmap[i], 0);
>     dirty_bitmap_buffer[i] = mask;
>     
> Because the generic fallback loop blindly overwrites dirty_bitmap_buffer[i]
> with the remaining uncleared bits, could this result in silently losing the
> dirty pages that the arch hook managed to clear before failing?

Yes, it falls back to software. 
But it will only overwrite the bits IF the entry is not empty.

One thing that could happen, though, is that the error happened in 
the middle of the 64-page slot, and the output buffer would contain bits 
set, while the dirty-bitmap also contains them. That would cause the bits 
to be overwritten. To avoid that, we could ask to make sure to only clear 
the dirty-bitmap word/slot if all bits have been cleaned. 

> 
> [ ... ]
> 
> > @@ -2373,6 +2379,10 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
> >  	if (copy_from_user(dirty_bitmap_buffer, log->dirty_bitmap, n))
> >  		return -EFAULT;
> >  
> > +	if (kvm_arch_dirty_log_clear(kvm, memslot, log, dirty_bitmap_buffer,
> > +				     &flush) >= 0)
> > +		goto out;
> > +
> 
> [Severity: High]
> Could passing dirty_bitmap_buffer directly to the arch-specific routine lead
> to an out-of-bounds read?
> 
> When userspace calls KVM_CLEAR_DIRTY_LOG with a first_page > 0, KVM copies
> exactly num_pages bits into dirty_bitmap_buffer starting at index 0.
> 
> If the arch implementation of kvm_arch_dirty_log_clear() assumes the buffer
> represents the entire memslot and reads from it using an absolute offset like
> log->first_page / BITS_PER_LONG, would it read past the end of this
> partially-sized array?
> 

Dealt with this in the implementation. Maybe it should be clear to whoever 
implements it will have to take that into account as well.