Re: [PATCH v3 05/11] kvm: Add arch-generic interface for hw-accelerated dirty-bitmap cleaning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Successful hardware dirty log extraction completely bypasses the generic software dirty bitmap, dropping internally-dirtied pages.
- [Critical] Hardware dirty bits are completely ignored when `KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2` is enabled.
- [High] Fallback software loop clobbers partial hardware dirty log output and leaks garbage data if `kvm_arch_dirty_log_clear()` fails.
- [Medium] The `kvm_arch_dirty_log_clear` API overloads two distinct behaviors based on an undocumented `NULL` parameter, risking a NULL pointer dereference in arch implementations.
--

--- Patch [5]: [PATCH v3 05/11] kvm: Add arch-generic interface for hw-accelerated dirty-bitmap cleaning ---
commit 4c5b443921b3fcc7c6b76fa46a7c11e8755ad50f
Author: Leonardo Bras <leo.bras@arm.com>

kvm: Add arch-generic interface for hw-accelerated dirty-bitmap cleaning

This patch introduces kvm_arch_dirty_log_clear() to allow architectures
to implement hardware-accelerated dirty-bitmap clearing. It adds hooks in
both kvm_get_dirty_log_protect() and kvm_clear_dirty_log_protect() to try
the hardware implementation first, falling back to the software loop if it
fails or is unimplemented.

> diff --git a/include/linux/kvm_dirty_bit.h b/include/linux/kvm_dirty_bit.h
> new file mode 100644
> index 0000000000000..fa4f6b67b6235
> --- /dev/null
> +++ b/include/linux/kvm_dirty_bit.h

[ ... ]

> +static inline int kvm_arch_dirty_log_clear(struct kvm *kvm,
> +					   struct kvm_memory_slot *memslot,
> +					   struct kvm_clear_dirty_log *log,
> +					   unsigned long *bitmap,
> +					   bool *flush)

[Severity: Medium]
The signature here strongly implies the log parameter is a valid clear
operation struct. However, when this is called from
kvm_get_dirty_log_protect(), NULL is passed for the log parameter.

Could this establish an undocumented API contract where new arch
implementations blindly dereference the log pointer and cause a host
kernel panic?

> +{
> +	return -ENXIO;
> +}

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 45e784462ec6e..7a13ef69b6e3e 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)

[Severity: Critical]
Since this hardware clear is invoked inside the else block of the
if (kvm->manual_dirty_log_protect) check, are hardware dirty bits
completely ignored when manual protection is enabled?

Modern VMMs often enable manual dirty log protection by default. Will
skipping the hardware tracking here result in userspace only receiving
the software dirty bitmap, potentially causing silent memory corruption
on the destination after a live migration?

[Severity: Medium]
This is the call site passing NULL for the log parameter.

As mentioned above, does this pose a risk of NULL pointer dereferences
for architectural implementations expecting a valid
struct kvm_clear_dirty_log pointer?

> +			goto out;
> +

[Severity: Critical]
If kvm_arch_dirty_log_clear() succeeds, this goto out bypasses the generic
software dirty bitmap processing loop completely.

Does this mean KVM's internally-dirtied pages, such as those from MMIO
emulation or virtual timers marked via mark_page_dirty(), are silently
dropped from the dirty log?

[Severity: High]
If the arch-specific hardware routine fails partway through and returns
an error code, could it leave partial hardware bits in dirty_bitmap_buffer?

If it falls through to the software loop, the software loop might overwrite
hardware bits with software bits (if the software bit is 1) and preserve
garbage hardware bits (if the software bit is 0). Should
dirty_bitmap_buffer be cleared on failure?

>  		KVM_MMU_LOCK(kvm);
>  		for (i = 0; i < n / sizeof(long); i++) {
>  			unsigned long mask;

[ ... ]

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

[Severity: High]
If the arch hook modifies dirty_bitmap_buffer before failing and
returning an error, does it corrupt the mask used by the fallback
clear loop?

The fallback software loop uses dirty_bitmap_buffer to decide which bits
to clear. If it contains garbage from a failed hardware clear, could this
cause the wrong pages to be cleared?

> +		goto out;
> +

[Severity: Critical]
Similar to the GET path, this goto out bypasses the loop that clears bits
in memslot->dirty_bitmap using atomic_long_fetch_andnot().

If the hardware clear succeeds, will the software bits remain set
indefinitely since they are bypassed from being cleared from KVM's
internal tracking?

>  	KVM_MMU_LOCK(kvm);
>  	for (offset = log->first_page, i = offset / BITS_PER_LONG,
>  		 n = DIV_ROUND_UP(log->num_pages, BITS_PER_LONG); n--;

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