Re: [PATCH v3 15/27] KVM: S390: Refactor gmap

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Memory allocation failure in `gmap_commit_memory_region` silently leaves stale guest hardware page table mappings, risking Guest-to-Host sandbox escapes.
- [High] Non-atomic clearing of the KVM dirty bitmap via `memset` races with concurrent memory faults, causing permanently lost dirty page records.
--

commit a87cd956788430d74dd24a5b4e2b190986ff3010
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: S390: Refactor gmap

This commit refactors gmap code so that a second s390 (host) KVM
implementation can use it. It moves relevant definitions into the shared
kvm_host.h and moves mmu code and traces from s390 to gmap.

> diff --git a/arch/s390/kvm/gmap/mmu.c b/arch/s390/kvm/gmap/mmu.c
> new file mode 100644
> index 0000000000000..d424aa361e795
> --- /dev/null
> +++ b/arch/s390/kvm/gmap/mmu.c

[ ... ]

> +int gmap_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
> +{
> +	int r;
> +	unsigned long n;
> +	struct kvm_memory_slot *memslot;
> +	int is_dirty;
> +
> +	if (kvm_is_ucontrol(kvm))
> +		return -EINVAL;
> +
> +	mutex_lock(&kvm->slots_lock);
> +
> +	r = -EINVAL;
> +	if (log->slot >= KVM_USER_MEM_SLOTS)
> +		goto out;
> +
> +	r = kvm_get_dirty_log(kvm, log, &is_dirty, &memslot);
> +	if (r)
> +		goto out;
> +
> +	/* Clear the dirty log */
> +	if (is_dirty) {
> +		n = kvm_dirty_bitmap_bytes(memslot);
> +		memset(memslot->dirty_bitmap, 0, n);

[Severity: High]
This is a pre-existing issue, but does using memset here to clear the
dirty_bitmap create a race condition?

Because memset performs non-atomic byte writes, could it race with concurrent
mark_page_dirty calls that use atomic bitset operations?

If a concurrent dirty page update is overwritten and lost, could this cause
guest memory corruption during live migration since the missed dirty page
would not be transferred to the destination?

> +	}
> +	r = 0;
> +out:
> +	mutex_unlock(&kvm->slots_lock);
> +	return r;
> +}

[ ... ]

> +void gmap_commit_memory_region(struct kvm *kvm,
> +			       struct kvm_memory_slot *old,
> +			       const struct kvm_memory_slot *new,
> +			       enum kvm_mr_change change)
> +{
> +	struct kvm_s390_mmu_cache *mc = NULL;
> +	int rc = 0;
> +
> +	if (change == KVM_MR_FLAGS_ONLY)
> +		return;
> +
> +	mc = kvm_s390_new_mmu_cache();
> +	if (!mc) {
> +		rc = -ENOMEM;
> +		goto out;

[Severity: Critical]
This isn't a bug introduced by this patch, but is it safe to perform a
fallible allocation in gmap_commit_memory_region? Because KVM commit hooks
return void, the common code cannot roll back memslot changes if this fails.

If kvm_s390_new_mmu_cache fails with -ENOMEM here, we jump out and skip
updating the hardware DAT tables via dat_delete_slot. Could the guest
retain hardware-level access to a removed memory region as a result?

If userspace reuses this virtual address range for sensitive internal data
rather than unmapping it, could the guest read or write it, leading to a
sandbox escape? Should this fallible allocation be moved to
gmap_prepare_memory_region instead?

> +	}
> +
> +	scoped_guard(write_lock, &kvm->mmu_lock) {
> +		switch (change) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529155050.2902245-1-seiden@linux.ibm.com?part=15