Re: [PATCH v1 3/3] KVM: s390: vsie: Use mmu cache to allocate rmap

[sashiko-bot@kernel.org]

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

New issues:
- [High] Slab out-of-bounds write due to undersized MMU cache allocations

Pre-existing issues:
- [High] Incomplete forward progress guarantee leaves preexisting livelock unresolved
--

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

KVM: s390: vsie: Use mmu cache to allocate rmap

Use kvm_s390_mmu_cache_alloc_rmap() to allocate the rmap in
gmap_insert_rmap(), instead of a normal kzalloc_obj() with GFP_ATOMIC.

> This guarantees forward progress.

[Severity: High]
This is a pre-existing issue, but does this actually guarantee
forward progress?

Immediately after allocating the rmap, gmap_insert_rmap() calls
radix_tree_insert(&sg->host_to_rmap, p_gfn, rmap).

Because the radix tree is initialized with GFP_ATOMIC, node allocations
can still fail under memory pressure and return -ENOMEM. The fault loop in
_gaccess_shadow_fault() catches this, drops the lock, calls
kvm_s390_mmu_cache_topup(), and retries:

again:
	rc = kvm_s390_mmu_cache_topup(vcpu->arch.mc);
	...
		rc = _gaccess_do_shadow(vcpu->arch.mc, sg, saddr, walk);
	...
	if (rc == -ENOMEM)
		goto again;

Since kvm_s390_mmu_cache_topup() does not call radix_tree_preload(), the
retry attempts another atomic allocation, potentially leading to an infinite
livelock when atomic memory is exhausted.

> Fixes: a2c17f9270cc ("KVM: s390: New gmap code")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> CC: stable@vger.kernel.org # 7.1

[ ... ]

> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> index 52d55ddea8d4f..1d289f8fa3b20 100644
> --- a/arch/s390/kvm/gmap.c
> +++ b/arch/s390/kvm/gmap.c
> @@ -1000,7 +1000,8 @@ int gmap_pv_destroy_range(struct gmap *gmap, gfn_t start, gfn_t end, bool interr
>  	return 0;
>  }
>  
> -int gmap_insert_rmap(struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn, int level)
> +int gmap_insert_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gfn,
> +		     gfn_t r_gfn, int level)
>  {
>  	struct vsie_rmap *rmap __free(kvfree) = NULL;
>  	struct vsie_rmap *temp;
> @@ -1010,7 +1011,7 @@ int gmap_insert_rmap(struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn, int level)
>  	KVM_BUG_ON(!is_shadow(sg), sg->kvm);
>  	lockdep_assert_held(&sg->host_to_rmap_lock);
>  
> -	rmap = kzalloc_obj(*rmap, GFP_ATOMIC);
> +	rmap = kvm_s390_mmu_cache_alloc_rmap(mc);

[Severity: High]
Does this result in a 1-byte allocation for the rmap?

Since mc->rmaps is an array of void pointers, when kvm_s390_mmu_cache_topup()
populates the cache in arch/s390/kvm/dat.c:

	o = kzalloc_obj(*mc->rmaps[0], GFP_KERNEL_ACCOUNT);

it evaluates sizeof(void), which is 1 byte. When gmap_insert_rmap()
fetches this 1-byte object from the cache and writes to rmap->r_gfn
(at offset 8):

	rmap->r_gfn = r_gfn;

can this cause a slab out-of-bounds write and memory corruption?

>  	if (!rmap)
>  		return -ENOMEM;

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