[PATCH v2 0/7] KVM: s390: Misc fixes

[PATCH v2 0/7] KVM: s390: Misc fixes

[Claudio Imbrenda]

Fix a bunch of small issues that came up during the previous round of fixes.

They are mostly extremely unlikely races, but they should be fixed
nonetheless.

v1->v2
* Drop some patches that have been picked upstream in the meantime.
* Drop patch 3, as it was trying to fix a bug that does not exist
* Avoid the NULL gmap dereference by using a flag
* Fix the return value of kvm_s390_[gp]et_skeys too
* Use kvm->slots_arch_lock instead of kvm->slots_lock for CMMA and ESSA
  handling, to avoid potential deadlocks with the RCU.
* Three new patches to fix other issues that came out while fixing the
  other issues


Claudio Imbrenda (7):
  KVM: s390: Fix unlikely NULL gmap dereference
  KVM: s390: Return -EFAULT instead of PGM_ADDRESSING
  KVM: s390: Fix race in __do_essa()
  KVM: s390: cmma: Fix dirty tracking when removing memslot
  KVM: s390: ucontrol: Add missing locking around gmap_remove_child()
  KVM: s390: Fix overclearing ESCA in case of error
  KVM: s390: Return -EINTR if a signal was pending while faulting-in

 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/dat.c              |  7 +++++-
 arch/s390/kvm/faultin.c          |  2 +-
 arch/s390/kvm/kvm-s390.c         | 39 ++++++++++++++++++++++++--------
 arch/s390/kvm/priv.c             |  5 ++--
 5 files changed, 40 insertions(+), 14 deletions(-)

-- 
2.55.0

[PATCH v2 1/7] KVM: s390: Fix unlikely NULL gmap dereference

[Claudio Imbrenda]

When creating a new vCPU, kvm_vm_ioctl_create_vcpu() will call
kvm_arch_vcpu_postcreate() after the file descriptor for the new vCPU
has been created. The new file descriptor has not been returned yet,
but a malicious userspace program could try to guess it.

If a malicious userspace program manages to start the newly created vCPU
before kvm_arch_vcpu_postcreate() is called, __vcpu_run() will try to
dereference vcpu->arch.gmap and trigger a NULL pointer dereference.

Fix this by moving the initialization of vcpu->arch.gmap into
kvm_arch_vcpu_create(), which is called before the file descriptor for
the vCPU is created.

Fixes: dafd032a15f8 ("KVM: s390: move vcpu specific initalization to a later point")
Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 1 +
 arch/s390/kvm/kvm-s390.c         | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index eaa34c5bd3c1..edf75b6ad20c 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -440,6 +440,7 @@ struct kvm_vcpu_arch {
 	bool skey_enabled;
 	/* Indicator if the access registers have been loaded from guest */
 	bool acrs_loaded;
+	bool initialized;
 	struct kvm_s390_pv_vcpu pv;
 	union diag318_info diag318_info;
 	struct kvm_s390_mmu_cache *mc;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 23c817595e28..374dae6dae81 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3611,6 +3611,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 	if (test_kvm_facility(vcpu->kvm, 74) || vcpu->kvm->arch.user_instr0 ||
 	    vcpu->kvm->arch.user_operexec)
 		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;
+
+	WRITE_ONCE(vcpu->arch.initialized, true);
 }
 
 static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)
@@ -5037,6 +5039,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 	    kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
 		return -EINVAL;
 
+	if (!vcpu->arch.initialized)
+		return -EINVAL;
+
 	vcpu_load(vcpu);
 
 	if (guestdbg_exit_pending(vcpu)) {
-- 
2.55.0

[PATCH v2 2/7] KVM: s390: Return -EFAULT instead of PGM_ADDRESSING

[Claudio Imbrenda]

If kvm_s390_set_cmma_bits() is asked to set CMMA values outside of a
memslot, PGM_ADDRESSING (5) is returned, instead of a negative error
value. Same issue with kvm_s390_{g,s}et_skeys().

Fix by returning -EFAULT whenever the return value would be > 0, which
is consistent with the behaviour before the gmap rewrite.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 374dae6dae81..53691aaf6534 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2212,7 +2212,7 @@ static int kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
 	}
 
 	kvfree(keys);
-	return r;
+	return r <= 0 ? r : -EFAULT;
 }
 
 static int kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
@@ -2274,7 +2274,7 @@ static int kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
 	kvm_s390_free_mmu_cache(mc);
 out:
 	kvfree(keys);
-	return r;
+	return r <= 0 ? r : -EFAULT;
 }
 
 /*
@@ -2384,7 +2384,7 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
 
 	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
 
-	return r;
+	return r <= 0 ? r : -EFAULT;
 }
 
 /**
-- 
2.55.0

[PATCH v2 3/7] KVM: s390: Fix race in __do_essa()

[Claudio Imbrenda]

An unlikely race between __do_essa() and kvm_s390_vm_start_migration(),
kvm_s390_vm_stop_migration(), or dat_get_cmma() was possible.

Fix by locking kvm->slots_lock. Since this is not a hot path, the
overhead of an additional mutex is negligible.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 9 ++++-----
 arch/s390/kvm/priv.c     | 5 +++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 53691aaf6534..9e3b8b5c6aa6 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1298,7 +1298,8 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
 {
 	int res = -ENXIO;
 
-	mutex_lock(&kvm->slots_lock);
+	guard(mutex)(&kvm->slots_arch_lock);
+
 	switch (attr->attr) {
 	case KVM_S390_VM_MIGRATION_START:
 		res = kvm_s390_vm_start_migration(kvm);
@@ -1309,7 +1310,6 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
 	default:
 		break;
 	}
-	mutex_unlock(&kvm->slots_lock);
 
 	return res;
 }
@@ -2996,9 +2996,8 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		r = -EFAULT;
 		if (copy_from_user(&args, argp, sizeof(args)))
 			break;
-		mutex_lock(&kvm->slots_lock);
-		r = kvm_s390_get_cmma_bits(kvm, &args);
-		mutex_unlock(&kvm->slots_lock);
+		scoped_guard(mutex, &kvm->slots_arch_lock)
+			r = kvm_s390_get_cmma_bits(kvm, &args);
 		if (!r) {
 			r = copy_to_user(argp, &args, sizeof(args));
 			if (r)
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index ad0ddc433a73..d4c91e416167 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -1260,8 +1260,9 @@ static int handle_essa(struct kvm_vcpu *vcpu)
 		/* Retry the ESSA instruction */
 		kvm_s390_retry_instr(vcpu);
 	} else {
-		scoped_guard(read_lock, &vcpu->kvm->mmu_lock)
-			i = __do_essa(vcpu, orc);
+		scoped_guard(mutex, &vcpu->kvm->slots_arch_lock)
+			scoped_guard(read_lock, &vcpu->kvm->mmu_lock)
+				i = __do_essa(vcpu, orc);
 		if (i < 0)
 			return i;
 		/* Account for the possible extra cbrl entry */
-- 
2.55.0

[PATCH v2 4/7] KVM: s390: cmma: Fix dirty tracking when removing memslot

[Claudio Imbrenda]

When a memslot is removed, all ptes that mapped the slot are cleared or
even deallocated. If this happens while the system is in migration
mode, and if cmma-dirty pages are removed, the cmma-dirty counter will
not reflect reality.

Fix by appropriately decrementing the cmma-dirty counter when removing
a memslot.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/dat.c      |  7 ++++++-
 arch/s390/kvm/kvm-s390.c | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index 5f1960ec982d..cc0b8b1330c2 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -844,6 +844,7 @@ static long _dat_slot_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_wal
 	struct slot_priv *p = walk->priv;
 	union crste dummy = { .val = p->token };
 	union pte new_pte, pte = READ_ONCE(*ptep);
+	union pgste pgste;
 
 	new_pte = _PTE_TOK(dummy.tok.type, dummy.tok.par);
 
@@ -851,7 +852,11 @@ static long _dat_slot_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_wal
 	if (pte.val == new_pte.val)
 		return 0;
 
-	dat_ptep_xchg(ptep, new_pte, gfn, walk->asce, false);
+	pgste = pgste_get_lock(ptep);
+	pgste = __dat_ptep_xchg(ptep, pgste, new_pte, gfn, walk->asce, false);
+	pgste.cmma_d = 0;
+	pgste_set_unlock(ptep, pgste);
+
 	return 0;
 }
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 9e3b8b5c6aa6..5c2408fc5a8c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5796,11 +5796,19 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 	return 0;
 }
 
+static long cmma_d_count_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
+{
+	if (pgste_of(ptep)->cmma_d)
+		atomic64_dec(walk->priv);
+	return 0;
+}
+
 void kvm_arch_commit_memory_region(struct kvm *kvm,
 				struct kvm_memory_slot *old,
 				const struct kvm_memory_slot *new,
 				enum kvm_mr_change change)
 {
+	const struct dat_walk_ops ops = { .pte_entry = cmma_d_count_pte, };
 	struct kvm_s390_mmu_cache *mc = NULL;
 	int rc = 0;
 
@@ -5814,6 +5822,12 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 	}
 
 	scoped_guard(write_lock, &kvm->mmu_lock) {
+		if (kvm->arch.migration_mode && kvm->arch.use_cmma) {
+			_dat_walk_gfn_range(old->base_gfn, old->base_gfn + old->npages,
+					    kvm->arch.gmap->asce, &ops, DAT_WALK_IGN_HOLES,
+					    &kvm->arch.cmma_dirty_pages);
+		}
+
 		switch (change) {
 		case KVM_MR_DELETE:
 			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
-- 
2.55.0

[PATCH v2 5/7] KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

[Claudio Imbrenda]

gmap_remove_child() needs to be called while holding the children_lock
of the parent gmap. This was not the case in the error handling path of
kvm_arch_vcpu_create() for UCONTROL guests.

Fix by adding the missing lock.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 5c2408fc5a8c..fc0a884d7f54 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3866,7 +3866,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 
 out_ucontrol_uninit:
 	if (kvm_is_ucontrol(vcpu->kvm)) {
-		gmap_remove_child(vcpu->arch.gmap);
+		scoped_guard(spinlock, &vcpu->kvm->arch.gmap->children_lock)
+			gmap_remove_child(vcpu->arch.gmap);
 		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
 	}
 out_free_sie_block:
-- 
2.55.0

[PATCH v2 6/7] KVM: s390: Fix overclearing ESCA in case of error

[Claudio Imbrenda]

If an attempt is made to create a vCPU with an already existing ID,
the duplicated vCPU will be destroyed. When destroying a vCPU, its
ESCA entry will be cleared. In the above scenario, the spurious
duplicate vCPU is destroyed, but the ESCA entry corresponding to the
original vCPU is cleared.

Fix by skipping clearing the ESCA entry if the vCPU creation was not
successful, i.e. if the pointer to the ESCA in the state description is
not set.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index fc0a884d7f54..6339a327a7be 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3458,7 +3458,7 @@ static void sca_del_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct esca_block *sca = vcpu->kvm->arch.sca;
 
-	if (!kvm_s390_use_sca_entries())
+	if (!kvm_s390_use_sca_entries() || !vcpu->arch.sie_block->scaol)
 		return;
 
 	clear_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);
-- 
2.55.0

[PATCH v2 7/7] KVM: s390: Return -EINTR if a signal was pending while faulting-in

[Claudio Imbrenda]

If a signal is pending while trying to fault-in a page, return -EINTR
instead of -EAGAIN.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: e907ae530133 ("KVM: s390: Add helper functions for fault handling")
---
 arch/s390/kvm/faultin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/faultin.c b/arch/s390/kvm/faultin.c
index fee80047bd94..235b9c66ce54 100644
--- a/arch/s390/kvm/faultin.c
+++ b/arch/s390/kvm/faultin.c
@@ -93,7 +93,7 @@ int kvm_s390_faultin_gfn(struct kvm_vcpu *vcpu, struct kvm *kvm, struct guest_fa
 			return PGM_ADDRESSING;
 		/* Signal pending: try again. */
 		if (f->pfn == KVM_PFN_ERR_SIGPENDING)
-			return -EAGAIN;
+			return -EINTR;
 		/* Check if it's read-only memory; don't try to actually handle that case. */
 		if (f->pfn == KVM_PFN_ERR_RO_FAULT)
 			return -EOPNOTSUPP;
-- 
2.55.0

Re: [PATCH v2 2/7] KVM: s390: Return -EFAULT instead of PGM_ADDRESSING

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch fails to fix the `KVM_S390_KEYOP` ioctl, which silently ignores and leaks positive program exception codes (e.g., `PGM_ADDRESSING`) to userspace in the exact same way as the patched functions.
--

--- Patch [2]: [PATCH v2 2/7] KVM: s390: Return -EFAULT instead of PGM_ADDRESSING ---
commit 4941b5a35b7857c25a5086d12db876b148a9642d
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Return -EFAULT instead of PGM_ADDRESSING

If kvm_s390_set_cmma_bits() is asked to set CMMA values outside of a
memslot, PGM_ADDRESSING (5) is returned, instead of a negative error
value. Same issue with kvm_s390_{g,s}et_skeys().

Fix by returning -EFAULT whenever the return value would be > 0, which
is consistent with the behaviour before the gmap rewrite.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 374dae6dae81b..53691aaf6534c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2212,7 +2212,7 @@ static int kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
>  	}
>  
>  	kvfree(keys);
> -	return r;
> +	return r <= 0 ? r : -EFAULT;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the same issue exist in
kvm_s390_keyop() and the KVM_S390_KEYOP ioctl handler?

When memory functions like dat_get_storage_key() return PGM_ADDRESSING (5),
kvm_s390_keyop() passes the positive exception code back to its caller without
converting it to -EFAULT:

arch/s390/kvm/kvm-s390.c:kvm_s390_keyop() {
    ...
	case KVM_S390_KEYOP_ISKE:
		r = dat_get_storage_key(asce, gfn, &skey);
		if (!r)
			return skey.skey;
		break;
    ...
	return r;
}

The KVM_S390_KEYOP handler then treats any positive return value from
kvm_s390_keyop() as a success, silently leaking the exception code into
kop.key and returning 0 to userspace:

arch/s390/kvm/kvm-s390.c:kvm_arch_vm_ioctl() {
    ...
		r = kvm_s390_keyop(mc, kvm, kop.operation, kop.guest_addr, skey);
		kvm_s390_free_mmu_cache(mc);
		if (r < 0)
			break;

		kop.key = r;
		r = 0;
		if (copy_to_user(argp, &kop, sizeof(kop)))
    ...
}

Since kvm_s390_keyop() conflates valid storage keys with exception codes,
userspace is fundamentally unable to detect that the memory access failed.

Should KVM_S390_KEYOP be updated to handle positive return codes similarly
to the functions fixed in this patch?

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

Re: [PATCH v2 5/7] KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

[sashiko-bot]

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

New issues:
- [Low] The patch introduces a scoped_guard() into a function that already uses goto-based error handling.

Pre-existing issues:
- [High] Missing cleanup for vcpu->arch.mc in the error paths of kvm_arch_vcpu_create().
- [High] Preexisting memory leak of vcpu->arch.pv.stor_base and sida_addr in kvm_s390_pv_create_cpu() on UV call failure.
--

--- Patch [5]: [PATCH v2 5/7] KVM: s390: ucontrol: Add missing locking around gmap_remove_child() ---
commit 08ba9ce959b1a049a33ef520bb8230d16cec1943
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

This commit adds missing locking for the parent gmap's children_lock when
calling gmap_remove_child(). This fixes an issue in the error handling path
of kvm_arch_vcpu_create() for UCONTROL guests.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -3866,7 +3866,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  out_ucontrol_uninit:
>  	if (kvm_is_ucontrol(vcpu->kvm)) {
> -		gmap_remove_child(vcpu->arch.gmap);
> +		scoped_guard(spinlock, &vcpu->kvm->arch.gmap->children_lock)
> +			gmap_remove_child(vcpu->arch.gmap);

[Severity: Low]
Does this mix scoped_guard() with goto-based error handling?

The cleanup guidelines suggest converting all resources that need a goto 
cleanup to scope-based cleanup, or converting none of them, rather than 
mixing them in the same function.

>  		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
>  	}
>  out_free_sie_block:
>  	free_page((unsigned long)(vcpu->arch.sie_block));
>  	return rc;
>  }

[Severity: High]
This is a pre-existing issue, but does this error path leak vcpu->arch.mc?

When kvm_arch_vcpu_create() allocates vcpu->arch.mc via 
kvm_s390_new_mmu_cache(), does the out_free_sie_block path miss freeing it 
before returning rc? 

Since kvm_arch_vcpu_destroy() isn't invoked on creation failure, it seems 
vcpu->arch.mc might remain permanently allocated.


[Severity: High]
This is a pre-existing issue, but does kvm_s390_pv_create_cpu() leak 
vcpu->arch.pv.stor_base and sida_addr if uv_call() fails?

In arch/s390/kvm/pv.c:kvm_s390_pv_create_cpu():

    if (cc) {
        ...
        kvm_s390_pv_destroy_cpu(vcpu, &dummy, &dummy);
        return -EIO;
    }

If this fails, it calls kvm_s390_pv_destroy_cpu():

arch/s390/kvm/pv.c:kvm_s390_pv_destroy_cpu():

    if (!kvm_s390_pv_cpu_get_handle(vcpu))
        return 0;

Since vcpu->arch.pv.handle is only populated on the uv_call() success path, 
will this return early and skip cleaning up stor_base and sida_addr?

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

Re: [PATCH v2 3/7] KVM: s390: Fix race in __do_essa()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Replacing `kvm->slots_lock` with `kvm->slots_arch_lock` removes protection for `kvm_memslots(kvm)` iteration in `kvm_s390_vm_start_migration()`, leading to lockdep splats and Use-After-Free.
- [Medium] The patch updates migration ioctls and `GET_CMMA_BITS` to use `slots_arch_lock`, but leaves `SET_CMMA_BITS` using `slots_lock`, breaking their mutual exclusion.
- [Medium] The commit message claims to use `kvm->slots_lock`, but the patch implements the fix using `kvm->slots_arch_lock`.
--

--- Patch [3]: [PATCH v2 3/7] KVM: s390: Fix race in __do_essa() ---
commit 71095d9c71a69950c39647a27ad09d197d37fa2b
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix race in __do_essa()

This commit addresses an unlikely race condition between __do_essa() and 
migration or cmma handlers. It intends to fix this by acquiring an additional 
lock during these operations.

> KVM: s390: Fix race in __do_essa()
> 
> An unlikely race between __do_essa() and kvm_s390_vm_start_migration(),
> kvm_s390_vm_stop_migration(), or dat_get_cmma() was possible.
> 
> Fix by locking kvm->slots_lock. Since this is not a hot path, the
> overhead of an additional mutex is negligible.

[Severity: Medium]
The commit message states we are locking kvm->slots_lock, but the code
below acquires kvm->slots_arch_lock. Could the commit message be updated
to match the actual implementation?

> Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 9 ++++-----
>  arch/s390/kvm/priv.c     | 5 +++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 53691aaf6534c..9e3b8b5c6aa66 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1298,7 +1298,8 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
>  {
>  	int res = -ENXIO;
>  
> -	mutex_lock(&kvm->slots_lock);
> +	guard(mutex)(&kvm->slots_arch_lock);

[Severity: High]
By switching from slots_lock to slots_arch_lock here, does this leave the
memslots iteration in kvm_s390_vm_start_migration() unprotected?

When kvm_s390_vm_start_migration() executes, it accesses the memslots:

kvm_s390_vm_start_migration() {
    ...
    slots = kvm_memslots(kvm);
    if (!slots || kvm_memslots_empty(slots))
        return -EINVAL;
    ...
    kvm_for_each_memslot(ms, bkt, slots) {
    ...
}

If we aren't holding kvm->slots_lock or kvm->srcu, can a concurrent memslot
update (like KVM_SET_USER_MEMORY_REGION) free the memslots array while it
is being accessed, potentially triggering a use-after-free and a lockdep
splat?

> +
>  	switch (attr->attr) {
>  	case KVM_S390_VM_MIGRATION_START:
>  		res = kvm_s390_vm_start_migration(kvm);

[ ... ]

> @@ -2996,9 +2996,8 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  		r = -EFAULT;
>  		if (copy_from_user(&args, argp, sizeof(args)))
>  			break;
> -		mutex_lock(&kvm->slots_lock);
> -		r = kvm_s390_get_cmma_bits(kvm, &args);
> -		mutex_unlock(&kvm->slots_lock);
> +		scoped_guard(mutex, &kvm->slots_arch_lock)
> +			r = kvm_s390_get_cmma_bits(kvm, &args);

[Severity: Medium]
Since GET_CMMA_BITS is updated to use kvm->slots_arch_lock here, is there a
risk of breaking mutual exclusion with KVM_S390_SET_CMMA_BITS?

Looking at KVM_S390_SET_CMMA_BITS further down in kvm_arch_vm_ioctl():

case KVM_S390_SET_CMMA_BITS: {
    ...
    mutex_lock(&kvm->slots_lock);
    r = kvm_s390_set_cmma_bits(kvm, &args);
    mutex_unlock(&kvm->slots_lock);
    break;
}

Since they now use different locks, can KVM_S390_SET_CMMA_BITS execute
concurrently with KVM_S390_GET_CMMA_BITS and migration operations, leading
to unpredictable CMMA state transitions?

>  		if (!r) {
>  			r = copy_to_user(argp, &args, sizeof(args));
>  			if (r)

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

Re: [PATCH v2 6/7] KVM: s390: Fix overclearing ESCA in case of error

[sashiko-bot]

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

New issues:
- [High] The condition `!vcpu->arch.sie_block->scaol` incorrectly assumes `scaol` is always non-zero for a valid SCA physical address, which can lead to silently skipping ESCA cleanup for valid vCPUs.

Pre-existing issues:
- [Critical] In `sca_add_vcpu()`, the hardware-visible ESCA `mcn` validity bit is set before the `sda` (State Description Address) pointer is written, exposing an uninitialized (zero) `sda` to concurrent hardware SIGP operations.
- [High] The patch fixes the clearing of the ESCA entry in `sca_del_vcpu()` during duplicate vCPU destruction, but misses an identical issue in `sca_clear_ext_call()` which also corrupts the original vCPU's ESCA state.
- [High] Memory leak of `vcpu->arch.mc` on the error path of `kvm_arch_vcpu_create()`.
--

--- Patch [6]: [PATCH v2 6/7] KVM: s390: Fix overclearing ESCA in case of error ---
commit 2c6e0529648df791186dd5602fe55245586d90d7
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix overclearing ESCA in case of error
    
If an attempt is made to create a vCPU with an already existing ID,
the duplicated vCPU will be destroyed. When destroying a vCPU, its
ESCA entry will be cleared. This patch fixes a regression where the
original vCPU ESCA entry was erroneously cleared by checking if the
pointer to the ESCA is set.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fc0a884d7f549..6339a327a7be2 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3458,7 +3458,7 @@ static void sca_del_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	struct esca_block *sca = vcpu->kvm->arch.sca;
>  
> -	if (!kvm_s390_use_sca_entries())
> +	if (!kvm_s390_use_sca_entries() || !vcpu->arch.sie_block->scaol)

[Severity: High]
Is it possible for scaol to legitimately be zero here?

Since scaol stores the lower 32 bits of the ESCA block physical address, if
the block allocated via alloc_pages_exact() happens to be 4GB aligned
(e.g., 0x100000000), scaol would evaluate to zero.

Would this cause the cleanup to be skipped for a fully initialized vCPU,
leaving the mcn bit set and sda pointing to a freed sie_block? Can this
cause a use-after-free if hardware receives a SIGP for the freed vCPU ID?

>  		return;
>  
>  	clear_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);

[Severity: High]
This is a pre-existing issue, but it looks like a similar regression exists
in sca_clear_ext_call().

When a duplicate vCPU is destroyed, kvm_arch_vcpu_destroy() calls
kvm_s390_clear_local_irqs(), which invokes sca_clear_ext_call(). Without a
similar initialization check, this unconditionally zeroes the sigp_ctrl:

arch/s390/kvm/interrupt.c:sca_clear_ext_call() {
    ...
    WRITE_ONCE(sigp_ctrl->value, 0);
    ...
}

Will this silently drop pending external calls or emergency signals for the
original, legitimate vCPU?


[Severity: High]
This is a pre-existing issue, but I noticed a potential memory leak on the
error path of kvm_arch_vcpu_create().

If an initialization step fails after vcpu->arch.mc is allocated via
kvm_s390_new_mmu_cache(), the error path frees vcpu->arch.sie_block but
appears to omit freeing vcpu->arch.mc:

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_create() {
    ...
out_free_sie_block:
    free_page((unsigned long)(vcpu->arch.sie_block));
    return rc;
}

Does this code leak the memory allocated for the mmu_cache?


[Severity: Critical]
This is a pre-existing issue, but the initialization order in sca_add_vcpu()
seems risky.

The mcn validity bitmask is set before the sda pointer is written:

arch/s390/kvm/kvm-s390.c:sca_add_vcpu() {
    ...
    set_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);
    sca->cpu[vcpu->vcpu_id].sda = virt_to_phys(vcpu->arch.sie_block);
    ...
}

Can a concurrent hardware SIGP instruction from another vCPU observe the
valid mcn bit and read an uninitialized sda pointer before the host software
writes the SIE block address?

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

Re: [PATCH v2 7/7] KVM: s390: Return -EINTR if a signal was pending while faulting-in

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Returning -EINTR from kvm_s390_faultin_gfn bypasses explicit -EAGAIN handling in callers, causing skipped instructions (missed PSW rewinds) and failed PV_UNPACK ioctl retries.
--

--- Patch [7]: [PATCH v2 7/7] KVM: s390: Return -EINTR if a signal was pending while faulting-in ---
commit f252af4580d9ab98014328b1d85bbc46f505b01c
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Return -EINTR if a signal was pending while faulting-in
    
If a signal is pending while trying to fault-in a page, return -EINTR
instead of -EAGAIN.

> diff --git a/arch/s390/kvm/faultin.c b/arch/s390/kvm/faultin.c
> index fee80047bd94f..235b9c66ce542 100644
> --- a/arch/s390/kvm/faultin.c
> +++ b/arch/s390/kvm/faultin.c
> @@ -93,7 +93,7 @@ int kvm_s390_faultin_gfn(struct kvm_vcpu *vcpu, struct kvm *kvm, struct guest_fa
>  			return PGM_ADDRESSING;
>  		/* Signal pending: try again. */
>  		if (f->pfn == KVM_PFN_ERR_SIGPENDING)
> -			return -EAGAIN;
> +			return -EINTR;

[Severity: Critical]
Will returning -EINTR here bypass explicit -EAGAIN handling in callers?

For example, in arch/s390/kvm/intercept.c:handle_mvpg_pei(), there is
the following loop:

	do {
		rc = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(dstaddr), true);
	} while (rc == -EAGAIN);
	if (rc)
		return rc;

	kvm_s390_retry_instr(vcpu);

If a non-fatal signal is pending, handle_mvpg_pei() will return -EINTR
and bypass kvm_s390_retry_instr(vcpu). Since the hardware has already
advanced the guest PSW for the intercept, does exiting to userspace without
rewinding the PSW cause the guest to resume at the next instruction,
silently skipping the MVPG instruction?

Similarly, in arch/s390/kvm/vsie.c:vsie_run(), returning -EINTR from
map_prefix() skips the rc == -EAGAIN check:

		rc = map_prefix(vcpu, vsie_page, sg);
...
		if (rc == -EAGAIN)
			rc = 0;

		/* Exit the loop if the guest needs to process the intercept */
		if (rc || scb_s->icptcode)
			break;

This breaks out of vsie_run() and exits to userspace without rewinding the
SIE instruction.

Finally, in arch/s390/kvm/pv.c:kvm_s390_pv_unpack(), the loop handles -EAGAIN
explicitly to absorb non-fatal signals:

	while (offset < size) {
		ret = unpack_one(kvm, addr, tweak, offset, rc, rrc);
		if (ret == -EAGAIN) {
			cond_resched();
			if (fatal_signal_pending(current))
				break;
			continue;
		}
		if (ret)
			break;

Returning -EINTR terminates the unpacking prematurely. When userspace
retries the KVM_PV_UNPACK ioctl, it restarts from offset 0, which could
cause the Ultravisor to reject the attempt to unpack already-secured pages,
permanently failing the VM boot process.

>  		/* Check if it's read-only memory; don't try to actually handle that case. */
>  		if (f->pfn == KVM_PFN_ERR_RO_FAULT)
>  			return -EOPNOTSUPP;

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

Re: [PATCH v2 1/7] KVM: s390: Fix unlikely NULL gmap dereference

[sashiko-bot]

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

New issues:
- [Medium] Commit message and implementation mismatch regarding vCPU initialization
- [Medium] Data race and lack of synchronization barriers on vcpu->arch.initialized

Pre-existing issues:
- [Critical] Incomplete fix leaves KVM_S390_MEM_OP vulnerable to NULL pointer dereference
- [High] Data race on vcpu->arch.sie_block->epoch with KVM_SET_ONE_REG silently drops userspace configuration
--

--- Patch [1]: [PATCH v2 1/7] KVM: s390: Fix unlikely NULL gmap dereference ---
commit f0bb4780c8538aa3e30c05d74a7d340ba636dd73
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix unlikely NULL gmap dereference
    
When creating a new vCPU, kvm_vm_ioctl_create_vcpu() will call
kvm_arch_vcpu_postcreate() after the file descriptor for the new vCPU
has been created. The new file descriptor has not been returned yet,
but a malicious userspace program could try to guess it.

[ ... ]

> Fix this by moving the initialization of vcpu->arch.gmap into
> kvm_arch_vcpu_create(), which is called before the file descriptor for
> the vCPU is created.

[Severity: Medium]
The commit message states that the initialization of vcpu->arch.gmap is being
moved to kvm_arch_vcpu_create(), but the implementation instead adds a new
vcpu->arch.initialized flag and leaves the gmap initialization in
kvm_arch_vcpu_postcreate(). Is this description accurate?

[ ... ]

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 23c817595e28d..374dae6dae81b 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3611,6 +3611,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  	if (test_kvm_facility(vcpu->kvm, 74) || vcpu->kvm->arch.user_instr0 ||
>  	    vcpu->kvm->arch.user_operexec)
>  		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;
> +
> +	WRITE_ONCE(vcpu->arch.initialized, true);

[Severity: High]
This is a pre-existing issue, but does overwriting the epoch here race with
userspace's KVM_SET_ONE_REG?

In kvm_arch_vcpu_postcreate(), the code overwrites the epoch without holding
vcpu->mutex:

	mutex_lock(&vcpu->kvm->lock);
	preempt_disable();
	vcpu->arch.sie_block->epoch = vcpu->kvm->arch.epoch;
	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
	preempt_enable();
	mutex_unlock(&vcpu->kvm->lock);

Since the file descriptor is already exposed, userspace could concurrently
execute KVM_SET_ONE_REG for KVM_REG_S390_EPOCHDIFF:

kvm_arch_vcpu_ioctl_set_one_reg() {
    ...
	case KVM_REG_S390_EPOCHDIFF:
		r = get_user(vcpu->arch.sie_block->epoch,
			     (u64 __user *)reg->addr);
    ...
}

Would the unlocked write in kvm_arch_vcpu_postcreate() silently clobber a
valid userspace configuration if KVM_SET_ONE_REG occurs just before it?

[Severity: Medium]
Could this WRITE_ONCE() allow compiler reordering? 

Since there is no release barrier like smp_store_release(), couldn't the
compiler sink the initialization of vcpu->arch.gmap (which still happens
earlier in this function) below this flag update?

>  }
>  
>  static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)

[ ... ]

> @@ -5037,6 +5039,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  	    kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
>  		return -EINVAL;
>  
> +	if (!vcpu->arch.initialized)
> +		return -EINVAL;
> +

[Severity: Medium]
Since this flag is written concurrently in kvm_arch_vcpu_postcreate(), does
this plain C read create a data race?  Should this be using READ_ONCE() or
smp_load_acquire() to ensure the memory accesses to vcpu->arch.gmap are safely
ordered?

[Severity: Critical]
This is a pre-existing issue, but does this fix leave KVM_S390_MEM_OP
vulnerable to a NULL pointer dereference?

The patch prevents KVM_RUN from executing before initialization, but doesn't
add a similar check for KVM_S390_MEM_OP. A concurrent KVM_S390_MEM_OP
handler will proceed:

kvm_s390_vcpu_mem_op() {
    ...
		r = check_gva_range(vcpu, mop->gaddr, mop->ar, mop->size,
				    acc_mode, mop->key);
    ...
}

If the KVM_S390_MEMOP_F_SKEY_PROTECTION flag is set, it routes to:

vcpu_check_access_key_gpa() {
    ...
	scoped_guard(read_lock, &vcpu->kvm->mmu_lock)
		r = dat_get_storage_key(vcpu->arch.gmap->asce, gpa_to_gfn(gpa), &storage_key);
    ...
}

Could this unconditionally dereference vcpu->arch.gmap->asce while it is still
NULL, resulting in a kernel panic?

>  	vcpu_load(vcpu);
>  
>  	if (guestdbg_exit_pending(vcpu)) {

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

Re: [PATCH v2 4/7] KVM: s390: cmma: Fix dirty tracking when removing memslot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Data race and double-decrement of `cmma_dirty_pages` due to two-pass CMMA cleanup
--

--- Patch [4]: [PATCH v2 4/7] KVM: s390: cmma: Fix dirty tracking when removing memslot ---
commit 119121acc5129e77726627587bbb91e16dea4543
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: cmma: Fix dirty tracking when removing memslot

This patch fixes inaccurate dirty tracking during migration by appropriately
decrementing the cmma-dirty counter when a memslot is removed. It does this
by walking the page tables and decrementing the counter prior to deleting
the slot.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 9e3b8b5c6aa66..5c2408fc5a8cb 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c

[ ... ]

> @@ -5796,11 +5796,19 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  	return 0;
>  }
>  
> +static long cmma_d_count_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
> +{
> +	if (pgste_of(ptep)->cmma_d)
> +		atomic64_dec(walk->priv);
> +	return 0;
> +}
> +

[Severity: High]
Can this two-pass CMMA cleanup cause a double-decrement of the global counter?

The first pass in cmma_d_count_pte() locklessly reads cmma_d and decrements
the counter, but leaves cmma_d = 1.

>  void kvm_arch_commit_memory_region(struct kvm *kvm,
>  				struct kvm_memory_slot *old,
>  				const struct kvm_memory_slot *new,
>  				enum kvm_mr_change change)
>  {
> +	const struct dat_walk_ops ops = { .pte_entry = cmma_d_count_pte, };
>  	struct kvm_s390_mmu_cache *mc = NULL;
>  	int rc = 0;
>  
> @@ -5814,6 +5822,12 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  	}
>  
>  	scoped_guard(write_lock, &kvm->mmu_lock) {
> +		if (kvm->arch.migration_mode && kvm->arch.use_cmma) {
> +			_dat_walk_gfn_range(old->base_gfn, old->base_gfn + old->npages,
> +					    kvm->arch.gmap->asce, &ops, DAT_WALK_IGN_HOLES,
> +					    &kvm->arch.cmma_dirty_pages);
> +		}
> +
>  		switch (change) {
>  		case KVM_MR_DELETE:
>  			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);

[Severity: High]
The wide window between the first walk here (decrementing) and the second walk
in dat_delete_slot() (which clears cmma_d) appears to allow a concurrent
KVM_S390_GET_CMMA_BITS ioctl to intervene.

Since the ioctl takes slots_arch_lock but not mmu_lock, could it acquire the
PGSTE lock, observe cmma_d == 1, clear it, and decrement the
cmma_dirty_pages counter a second time for the same page?

If this happens, the counter would underflow and wrap to UINT64_MAX, causing
KVM_S390_GET_CMMA_BITS to permanently report pending dirty pages and break VM
live migration.

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