Re: [PATCH v5 7/9] KVM: s390: Fix cmma dirty tracking

[sashiko-bot@kernel.org]

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

New issues:
- [High] A stalled VCPU thread in `handle_essa()` racing with `kvm_s390_vm_start_migration()` double-counts `cmma_dirty_pages`, causing an infinite loop in userspace.

Pre-existing issues:
- [Critical] Uninitialized kernel memory from `vmalloc` is leaked to userspace via the CMMA state buffer.
- [High] CMMA state of newly mapped pages can be silently lost during the start of migration due to a TOCTOU race with `gmap_set_cmma_all_dirty` running while `migration_mode == 0`.
--

commit d55126b5a3d84f06909a4a559b0185411825d1f2
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix cmma dirty tracking

This commit updates migration mode initialization to accurately count pages
with allocated PTEs instead of counting all pages in memslots. It also fixes
dat_get_cmma() to wrap around when searching for cmma-dirty pages.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index cffac7782c4bf..0ad4ebc80ebaf 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -1253,6 +1253,9 @@ int dat_get_cmma(union asce asce, gfn_t *start, unsigned int *count, u8 *values,
>  	};
>  
>  	_dat_walk_gfn_range(*start, asce_end(asce), asce, &ops, DAT_WALK_IGN_HOLES, &state);
> +	/* If no dirty pages were found, wrap around and continue searching */
> +	if (*start && state.start == -1)
> +		_dat_walk_gfn_range(0, *start, asce, &ops, DAT_WALK_IGN_HOLES, &state);

[Severity: Critical]
This is a pre-existing issue, but does the values buffer leak uninitialized
kernel memory to userspace?

In kvm_s390_get_cmma_bits(), the values buffer is allocated via vmalloc()
without zeroing:

arch/s390/kvm/kvm-s390.c:kvm_s390_get_cmma_bits() {
    ...
    values = vmalloc(args->count);
    if (!values)
        return -ENOMEM;
    ...
}

When dat_get_cmma() iterates over guest memory, __dat_get_cmma_pte() skips
unmapped holes and non-dirty pages (cmma_d == 0). This leaves uninitialized
gaps in the state->values array between the first and last dirty pages in the
batch.

Since the entire array up to args->count is later copied to userspace via
copy_to_user(), could this expose sensitive heap data?

[ ... ]
> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> index e6e786811db86..a9a37fdeb809b 100644
> --- a/arch/s390/kvm/gmap.c
> +++ b/arch/s390/kvm/gmap.c
> @@ -1075,7 +1075,13 @@ int gmap_protect_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gf
>  
>  static long __set_cmma_dirty_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
>  {
> -	__atomic64_or(PGSTE_CMMA_D_BIT, &pgste_of(ptep)->val);
> +	union pgste pgste;
> +
> +	pgste = pgste_get_lock(ptep);
> +	pgste.cmma_d = 1;
> +	pgste_set_unlock(ptep, pgste);
> +	atomic64_inc(walk->priv);

[Severity: High]
Can this cause double-counting of cmma_dirty_pages if a VCPU thread in
handle_essa() races with kvm_s390_vm_start_migration()?

If a VCPU locklessly reads migration_mode == 1 in handle_essa(), stalls, and
userspace stops then restarts migration:
kvm_s390_vm_start_migration() resets cmma_dirty_pages = 0 and starts
gmap_set_cmma_all_dirty().

The stalled VCPU thread could then resume, enter __do_essa(), set cmma_d = 1,
and increment cmma_dirty_pages. When __set_cmma_dirty_pte() reaches the same
page later, it lacks a check for !pgste.cmma_d, blindly setting cmma_d = 1
and incrementing cmma_dirty_pages again.

Would this over-counting cause userspace (like QEMU) to spin infinitely when
calling the KVM_GET_CMMA_BITS ioctl, as it decrements its remaining counter
expecting more pages than actually exist?

[ ... ]
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 93141a68e0dde..cdd8b41d24ed2 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
>  	/* mark all the pages as dirty */
> +	atomic64_set(&kvm->arch.cmma_dirty_pages, 0);
>  	gmap_set_cmma_all_dirty(kvm->arch.gmap);
> -	atomic64_set(&kvm->arch.cmma_dirty_pages, ram_pages);
> -	kvm->arch.migration_mode = 1;
> +	WRITE_ONCE(kvm->arch.migration_mode, 1);

[Severity: High]
This isn't a bug introduced by this patch, but does kvm_s390_vm_start_migration()
have a race condition because gmap_set_cmma_all_dirty() runs entirely while
migration_mode == 0?

During gmap_set_cmma_all_dirty(), if a VCPU faults in a new page at an address
already scanned by the walker, the PTE is created with cmma_d = 0. Since
migration_mode is still 0, the VCPU can execute ESSA instructions which
hardware handles natively.

This would modify the CMMA state in the PGSTE but leave cmma_d = 0. When the
migration scan finishes and sets migration_mode = 1, does the newly dirtied
page remain untracked, causing guest state corruption across live migration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622160739.202044-1-imbrenda@linux.ibm.com?part=7