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

[sashiko-bot@kernel.org]

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?

[ ... ]

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

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