[PATCH v3 0/8] KVM: s390: More fixes

[PATCH v3 0/8] KVM: s390: More fixes

[Christian Borntraeger]

v2->v3:
- add length check fix for guest debug
- add RB

v1->v2: several sashiko reports added
- machine check stack exposure
- extend zeroing to pfault done interrupts
- old_data leak
- srcu protection missing
- guest debug allocations not freed on destroy
- uppercase patch title


Christian Borntraeger (8):
  KVM: s390: Remove user triggerable WARN_ON
  KVM: s390: Zero initialize data structures for inject_pfault_token
  KVM: s390: Zero initialize irq in reinject_machine_check
  KVM: s390: Fix memory leak in guest debug handling
  KVM: s390: Fix old_data leak in guest debug error path
  KVM: s390: Take srcu when importing watchpoint data
  KVM: s390: Free guest debug data on vcpu destroy
  KVM: s390: Fix length check __import_wp_info()

 arch/s390/kvm/guestdbg.c  |  9 +++++++--
 arch/s390/kvm/interrupt.c |  4 ++--
 arch/s390/kvm/kvm-s390.c  | 11 +++++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.53.0

[PATCH v3 1/8] KVM: s390: Remove user triggerable WARN_ON

[Christian Borntraeger]

pin_map_page() fails legitimately whenever the userspace mapping behind
the adapter route has gone away, e.g. when the VMM unmaps that memory.
As this can happen without a kernel programming error, remove the
WARN_ON.

Fixes: 1e95e3bc6b05 ("KVM: s390: Enable adapter_indicators_set to use mapped pages")
Cc: Douglas Freimuth <freimuth@linux.ibm.com>
Cc: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Douglas Freimuth <freimuth@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 8f24bcd1a6d3..23f0e0821474 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -3013,7 +3013,7 @@ static int adapter_indicators_set(struct kvm *kvm,
 	if (!summary_info) {
 		spin_unlock_irqrestore(&adapter->maps_lock, flags);
 		summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
-		if (WARN_ON_ONCE(!summary_page))
+		if (!summary_page)
 			return -1;
 		idx = srcu_read_lock(&kvm->srcu);
 		map = page_address(summary_page);
-- 
2.53.0

Re: [PATCH v3 1/8] KVM: s390: Remove user triggerable WARN_ON

[sashiko-bot]

> pin_map_page() fails legitimately whenever the userspace mapping behind
> the adapter route has gone away, e.g. when the VMM unmaps that memory.
> As this can happen without a kernel programming error, remove the
> WARN_ON.
> 
> Fixes: 1e95e3bc6b05 ("KVM: s390: Enable adapter_indicators_set to use mapped pages")
> Cc: Douglas Freimuth <freimuth@linux.ibm.com>
> Cc: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v3 2/8] KVM: s390: Zero initialize data structures for inject_pfault_token

[Christian Borntraeger]

__kvm_inject_pfault_token() only sets .type and .u.ext.ext_params2 of
the on-stack struct kvm_s390_irq but the full ext substructure is copied
into the cpu local variable on inject. ext_params and pad contain stale
stack values.

Interrupt delivery only uses ext_params2, so nothing leaks to the guest,
but a host user can use the migration ioctls to get to the data.

Fix by zero-initializing the irq struct.
Do the same for the inti data structure.

Fixes: 383d0b050106 ("KVM: s390: handle pending local interrupts via bitmap")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 4a38de3d6758..c47c3a930db7 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4473,8 +4473,8 @@ int kvm_s390_try_set_tod_clock(struct kvm *kvm, const struct kvm_s390_vm_tod_clo
 static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
 				     unsigned long token)
 {
-	struct kvm_s390_interrupt inti;
-	struct kvm_s390_irq irq;
+	struct kvm_s390_interrupt inti = {};
+	struct kvm_s390_irq irq = {};
 	struct kvm_s390_interrupt_info *inti_mem = NULL;
 	int ret = 0;
 
-- 
2.53.0

Re: [PATCH v3 2/8] KVM: s390: Zero initialize data structures for inject_pfault_token

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Invalid Free / Kernel Panic in kvm_s390_reinject_machine_check() due to kfree() on a stack pointer.
- [High] Stack Information Leak to userspace in kvm_s390_reinject_machine_check() during local machine check reinjection.
--

--- Patch [2]: [PATCH v3 2/8] KVM: s390: Zero initialize data structures for inject_pfault_token ---
commit 99d37a57e11716f103a611788767010736475aa3
Author: Christian Borntraeger <borntraeger@linux.ibm.com>

KVM: s390: Zero initialize data structures for inject_pfault_token

This commit fixes a stack information leak by zero-initializing the
kvm_s390_interrupt and kvm_s390_irq structures in
__kvm_inject_pfault_token. This prevents stale stack values from leaking to
host userspace via migration ioctls.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 518a69c55e85c..611dcb5342101 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4474,8 +4474,8 @@ int kvm_s390_try_set_tod_clock(struct kvm *kvm, const struct kvm_s390_vm_tod_clo
>  static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
>  				     unsigned long token)
>  {
> -	struct kvm_s390_interrupt inti;
> -	struct kvm_s390_irq irq;
> +	struct kvm_s390_interrupt inti = {};
> +	struct kvm_s390_irq irq = {};
>  	struct kvm_s390_interrupt_info *inti_mem = NULL;

[Severity: Critical]
This isn't a bug introduced by this patch, but while reviewing this fix,
I noticed a similar pre-existing issue in arch/s390/kvm/interrupt.c that
appears to pass a stack pointer to kfree().

In kvm_s390_reinject_machine_check(), a stack-allocated interrupt info
structure is passed to __inject_vm():

kvm_s390_reinject_machine_check() {
    ...
    struct kvm_s390_interrupt_info inti;
    ...
    rc = __inject_vm(vcpu->kvm, &inti);

When the machine check is a floating machine check, this routes to
__inject_float_mchk(), which unconditionally frees the pointer:

__inject_float_mchk() {
    ...
    kfree(inti);

Could this result in a kernel panic if a nested guest execution triggers
this path, attempting to free a stack-allocated variable?

[Severity: High]
This is also a pre-existing issue, but I noticed another potential stack
information leak in arch/s390/kvm/interrupt.c.

Does kvm_s390_reinject_machine_check() leak uninitialized stack memory
during local machine check reinjection?

kvm_s390_reinject_machine_check() {
    ...
    struct kvm_s390_irq irq;
    ...
    kvm_s390_inject_vcpu(vcpu, &irq);

Because irq is uninitialized on the stack, the padding and fixed_logout
array contain stale data. __inject_mchk() then copies this data into the
persistent vCPU state:

__inject_mchk() {
    ...
    memcpy(&mchk->fixed_logout, &irq->u.mchk.fixed_logout,
           sizeof(mchk->fixed_logout));

Later, when userspace retrieves the interrupt state, store_local_irq()
copies it back out:

store_local_irq() {
    ...
    irq->u.mchk = li->irq.mchk;

Could this allow a host userspace process to access uninitialized kernel
stack memory via the KVM migration ioctls, similar to the issue fixed in
this patch?

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

[PATCH v3 3/8] KVM: s390: Zero initialize irq in reinject_machine_check

[Christian Borntraeger]

kvm_s390_reinject_machine_check() fills cr14, mcic, ext_damage_code and
failing_storage_address of the on-stack struct kvm_s390_irq, but struct
kvm_s390_mchk_info also has a pad word and a 16 byte fixed_logout array.
struct mcck_volatile_info carries no logout data, so there is nothing to
copy there and both stay whatever was on the stack.

__inject_mchk() then memcpy()s fixed_logout into the vcpu local
interrupt state unconditionally. This will reach the guest during
deliver and userspace during migration.

Reflecting zeroes is the correct behaviour here, as KVM has no logout
data for a reinjected machine check.

This needs a host machine check while the cpu is in SIE so not trivial
to trigger.

Fixes: 4d62fcc0b692 ("KVM: s390: Inject machine check into the guest")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 23f0e0821474..009d6a845d59 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -3109,7 +3109,7 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
 				     struct mcck_volatile_info *mcck_info)
 {
 	struct kvm_s390_interrupt_info inti;
-	struct kvm_s390_irq irq;
+	struct kvm_s390_irq irq = {};
 	struct kvm_s390_mchk_info *mchk;
 	union mci mci;
 	__u64 cr14 = 0;         /* upper bits are not used */
-- 
2.53.0

Re: [PATCH v3 3/8] KVM: s390: Zero initialize irq in reinject_machine_check

[sashiko-bot]

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

Pre-existing issues:
- [High] Calling kfree() on a stack-allocated variable `inti` when injecting a floating machine check leads to a kernel panic.
--

--- Patch [3]: [PATCH v3 3/8] KVM: s390: Zero initialize irq in reinject_machine_check ---
commit 318473328452694fe22802ec70c8f2cf0f840578
Author: Christian Borntraeger <borntraeger@linux.ibm.com>

KVM: s390: Zero initialize irq in reinject_machine_check

This commit zero-initializes the on-stack struct kvm_s390_irq in
kvm_s390_reinject_machine_check(). This prevents uninitialized memory from
being copied into the vcpu local interrupt state and reaching the guest or
userspace.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 23f0e0821474a..009d6a845d598 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -3109,7 +3109,7 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
>  				     struct mcck_volatile_info *mcck_info)
>  {
>  	struct kvm_s390_interrupt_info inti;

[Severity: High]
This is a pre-existing issue, but does this stack-allocated variable lead to
a kernel panic during floating machine check injection?

If mci.ck is set, this function passes the stack-allocated inti pointer to
__inject_vm():

	if (mci.ck) {
		/* Inject the floating machine check */
		inti.type = KVM_S390_MCHK;
		rc = __inject_vm(vcpu->kvm, &inti);
	}

__inject_vm() then calls __inject_float_mchk(), which unconditionally calls
kfree() on the provided pointer:

arch/s390/kvm/interrupt.c:__inject_float_mchk() {
    ...
    kfree(inti);
    ...
}

Will this cause a panic when kfree() attempts to free the local stack
pointer?

> -	struct kvm_s390_irq irq;
> +	struct kvm_s390_irq irq = {};
[ ... ]

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

[PATCH v3 4/8] KVM: s390: Fix memory leak in guest debug handling

[Christian Borntraeger]

bp_data is freed only for the error case by kfree(bp_data).
Every successful KVM_SET_GUEST_DEBUG will leak bp_data.

Fixes: 27291e2165b6 ("KVM: s390: hardware support for guest debugging")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/kvm/guestdbg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index 69835e1d4f20..4c02dbebb2eb 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -267,6 +267,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	vcpu->arch.guestdbg.hw_bp_info = bp_info;
 	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
 	vcpu->arch.guestdbg.hw_wp_info = wp_info;
+	kfree(bp_data);
 	return 0;
 error:
 	kfree(bp_data);
-- 
2.53.0

Re: [PATCH v3 4/8] KVM: s390: Fix memory leak in guest debug handling

[sashiko-bot]

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

Pre-existing issues:
- [High] Guest debug dynamically allocated data (hw_wp_info, hw_bp_info, and old_data buffers) are leaked when a VCPU is destroyed while guest debugging is active.
- [High] Accessing guest memory (memslots) via `read_guest_abs()` without holding the SRCU read lock.
- [High] Integer truncation of `bp_data->len` allows unprivileged host-userspace to trigger a page allocator WARN_ON_ONCE via kmalloc().
--

--- Patch [4]: [PATCH v3 4/8] KVM: s390: Fix memory leak in guest debug handling ---
commit 4fe6d9b7b9fc213b17879f6cacc90f0c1ed0f351
Author: Christian Borntraeger <borntraeger@linux.ibm.com>

KVM: s390: Fix memory leak in guest debug handling

bp_data is freed only for the error case by kfree(bp_data). Every successful
KVM_SET_GUEST_DEBUG will leak bp_data.

Fixes: 27291e2165b6 ("KVM: s390: hardware support for guest debugging")
Cc: stable@vger.kernel.org

> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index 69835e1d4f207..4c02dbebb2eb3 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -267,6 +267,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  	vcpu->arch.guestdbg.hw_bp_info = bp_info;
>  	vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
>  	vcpu->arch.guestdbg.hw_wp_info = wp_info;
> +	kfree(bp_data);

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the dynamically
allocated data (hw_wp_info, hw_bp_info, and old_data buffers) is leaked
when a VCPU is destroyed.

If a userspace process calls the KVM_SET_GUEST_DEBUG ioctl to enable hardware
breakpoints or watchpoints, it allocates memory for these buffers. If the
process then destroys the VCPU without disabling debugging, does
kvm_arch_vcpu_destroy() fail to call kvm_s390_clear_bp_data(vcpu) to free
these buffers?

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_destroy() {
    ...
	free_page((unsigned long)(vcpu->arch.sie_block));
	kvm_s390_free_mmu_cache(vcpu->arch.mc);
}

Could this allow an unprivileged user with access to KVM to repeatedly create
and destroy VCPUs to trigger a host-side memory exhaustion?


[Severity: High]
This is a pre-existing issue, but does this code access guest memory
without holding the SRCU read lock?

When __import_wp_info() is called earlier in this function:

arch/s390/kvm/guestdbg.c:__import_wp_info() {
    ...
	/* try to backup the original value */
	ret = read_guest_abs(vcpu, wp_info->phys_addr, wp_info->old_data,
			     wp_info->len);
    ...
}

The call chain goes through kvm_s390_import_bp_data() -> __import_wp_info() ->
read_guest_abs() -> kvm_read_guest() without holding srcu_read_lock(&kvm->srcu).

Could concurrent memslot modifications (e.g., via other VCPU threads or
memory hotplug) result in a use-after-free when KVM attempts to traverse the
unprotected memslot tree?


[Severity: High]
This is a pre-existing issue, but is there a potential integer truncation
when handling bp_data->len?

In __import_wp_info(), the 64-bit bp_data->len from userspace is truncated
to a 32-bit signed integer wp_info->len:

arch/s390/kvm/guestdbg.c:__import_wp_info() {
    ...
	wp_info->len = bp_data->len;
    ...
	if (wp_info->len < 0 || wp_info->len > MAX_WP_SIZE)
		return -EINVAL;

	wp_info->old_data = kmalloc(bp_data->len, GFP_KERNEL_ACCOUNT);
    ...
}

If bp_data->len is set to 0x100000001, it would truncate to wp_info->len = 1,
bypassing the MAX_WP_SIZE check. The original 64-bit bp_data->len is then
passed to kmalloc().

Since the allocation size is >4GB, it exceeds MAX_PAGE_ORDER. Because the
GFP_KERNEL_ACCOUNT flag lacks __GFP_NOWARN, will the kernel's page allocator
emit a WARN_ON_ONCE that could be triggered by an unprivileged host-userspace
process?

>  	return 0;
>  error:
>  	kfree(bp_data);

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

[PATCH v3 5/8] KVM: s390: Fix old_data leak in guest debug error path

[Christian Borntraeger]

__import_wp_info() allocates a per-watchpoint old_data buffer to back up
the original guest memory contents. If a later watchpoint of the same
KVM_SET_GUEST_DEBUG request fails to import, kvm_s390_import_bp_data()
jumps to the error label, which frees the wp_info array but not the
old_data buffers of the entries that were imported successfully. Up to
MAX_BP_COUNT - 1 buffers of up to MAX_WP_SIZE bytes are leaked per failed
request, and the request can be repeated.

Create error handling for cleaning up all created old_data memory
areas.

Fixes: 27291e2165b6 ("KVM: s390: hardware support for guest debugging")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/kvm/guestdbg.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index 4c02dbebb2eb..f7c94d54efbe 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -252,7 +252,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 			ret = __import_wp_info(vcpu, &bp_data[i],
 					       &wp_info[nr_wp]);
 			if (ret)
-				goto error;
+				goto error_wp;
 			nr_wp++;
 			break;
 		case KVM_HW_BP:
@@ -269,6 +269,10 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	vcpu->arch.guestdbg.hw_wp_info = wp_info;
 	kfree(bp_data);
 	return 0;
+
+error_wp:
+	while (nr_wp--)
+		kfree(wp_info[nr_wp].old_data);
 error:
 	kfree(bp_data);
 	kfree(wp_info);
-- 
2.53.0

Re: [PATCH v3 5/8] KVM: s390: Fix old_data leak in guest debug error path

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing SRCU lock when accessing memslots during guest debug setup leads to Use-After-Free.
--

--- Patch [5]: [PATCH v3 5/8] KVM: s390: Fix old_data leak in guest debug error path ---
commit efad1ff98c2af0fcd50545a3067b444c29f0bc33
Author: Christian Borntraeger <borntraeger@linux.ibm.com>

KVM: s390: Fix old_data leak in guest debug error path

__import_wp_info() allocates a per-watchpoint old_data buffer to back up
the original guest memory contents. If a later watchpoint of the same
KVM_SET_GUEST_DEBUG request fails to import, kvm_s390_import_bp_data()
jumps to the error label, which frees the wp_info array but not the
old_data buffers of the entries that were imported successfully. Up to
MAX_BP_COUNT - 1 buffers of up to MAX_WP_SIZE bytes are leaked per failed
request, and the request can be repeated.

Create error handling for cleaning up all created old_data memory
areas.

> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index 4c02dbebb2eb3..f7c94d54efbe2 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -252,7 +252,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  			ret = __import_wp_info(vcpu, &bp_data[i],
>  					       &wp_info[nr_wp]);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to call
__import_wp_info() here without holding the SRCU read lock?

__import_wp_info() in arch/s390/kvm/guestdbg.c attempts to read guest memory:

arch/s390/kvm/guestdbg.c:__import_wp_info() {
    ...
	/* try to backup the original value */
	ret = read_guest_abs(vcpu, wp_info->phys_addr, wp_info->old_data,
			     wp_info->len);
    ...
}

This ultimately accesses the KVM memslots array.

Because the KVM_SET_GUEST_DEBUG ioctl entry point doesn't acquire the SRCU read
lock before calling kvm_s390_import_bp_data():

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_ioctl_set_guest_debug() {
    ...
		if (dbg->control & KVM_GUESTDBG_USE_HW_BP)
			rc = kvm_s390_import_bp_data(vcpu, dbg);
    ...
}

Could a concurrent memslot update (such as another thread calling
KVM_SET_USER_MEMORY_REGION on the VM) free the memslots array while it is being
accessed by this debug ioctl, leading to a Use-After-Free?

>  			if (ret)
> -				goto error;
> +				goto error_wp;
>  			nr_wp++;
>  			break;

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

[PATCH v3 6/8] KVM: s390: Take srcu when importing watchpoint data

[Christian Borntraeger]

__import_wp_info() backs up the original guest memory contents of a
watchpoint with read_guest_abs(), which is kvm_read_guest() and therefore
resolves the memslot via __kvm_memslots(). That requires kvm->srcu (or
kvm->slots_lock) to be held, otherwise a concurrent memslot update can
free the memslots array under us once its SRCU grace period has elapsed.

As this is not fast path, following lock ordering (mutex first, then
srcu) take the big hammer and hold the srcu for the full import.

Fixes: 27291e2165b6 ("KVM: s390: hardware support for guest debugging")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index c47c3a930db7..e0548926dc49 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4247,8 +4247,10 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 		/* enforce guest PER */
 		kvm_s390_set_cpuflags(vcpu, CPUSTAT_P);
 
-		if (dbg->control & KVM_GUESTDBG_USE_HW_BP)
-			rc = kvm_s390_import_bp_data(vcpu, dbg);
+		if (dbg->control & KVM_GUESTDBG_USE_HW_BP) {
+			scoped_guard(srcu, &vcpu->kvm->srcu)
+				rc = kvm_s390_import_bp_data(vcpu, dbg);
+		}
 	} else {
 		kvm_s390_clear_cpuflags(vcpu, CPUSTAT_P);
 		vcpu->arch.guestdbg.last_bp = 0;
-- 
2.53.0

Re: [PATCH v3 6/8] KVM: s390: Take srcu when importing watchpoint data

[sashiko-bot]

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

New issues:
- [Medium] Mixing goto-based error handling and scope-based cleanup helpers in the same function.

Pre-existing issues:
- [High] VCPU guest debug hardware breakpoints are leaked when the VCPU is destroyed.
- [High] Integer truncation in watchpoint length check bypasses MAX_WP_SIZE, leading to a massive kmalloc and WARN_ON_ONCE.
--

--- Patch [6]: [PATCH v3 6/8] KVM: s390: Take srcu when importing watchpoint data ---
commit e0e29f54e03d9a52cf7d59a9894e63da62df0c7d
Author: Christian Borntraeger <borntraeger@linux.ibm.com>

KVM: s390: Take srcu when importing watchpoint data

This commit adds a scoped_guard around kvm_s390_import_bp_data to ensure
the srcu lock is held while backing up original guest memory contents of a
watchpoint. This prevents concurrent memslot updates from freeing the
memslots array during the read operation.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 611dcb5342101..0a197e055cad0 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4248,8 +4248,10 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  		/* enforce guest PER */
>  		kvm_s390_set_cpuflags(vcpu, CPUSTAT_P);
>  
> -		if (dbg->control & KVM_GUESTDBG_USE_HW_BP)
> -			rc = kvm_s390_import_bp_data(vcpu, dbg);
> +		if (dbg->control & KVM_GUESTDBG_USE_HW_BP) {
> +			scoped_guard(srcu, &vcpu->kvm->srcu)

[Severity: Medium]
Does mixing scoped_guard with the existing goto out; error handling
in this function violate the cleanup subsystem guidelines?

The documentation states that for a given routine, we should convert all
resources that need a goto cleanup to scope-based cleanup, or convert
none of them.

> +				rc = kvm_s390_import_bp_data(vcpu, dbg);

[Severity: High]
This isn't a bug introduced by this patch, but does the guest debug memory
get leaked when the VCPU is destroyed?

Looking at the memory allocated here in kvm_s390_import_bp_data():

arch/s390/kvm/guestdbg.c:kvm_s390_import_bp_data() {
    ...
    wp_info = kmalloc_objs(*wp_info, nr_wp, GFP_KERNEL_ACCOUNT);
    ...
}

It seems the cleanup path kvm_arch_vcpu_destroy() fails to call
kvm_s390_clear_bp_data(vcpu) to release these resources. Could this allow
a host userspace process to slowly drain host memory by repeatedly
creating and terminating VMs with hardware breakpoints?

[Severity: High]
This is a pre-existing issue, but is there an integer truncation here that
bypasses the MAX_WP_SIZE check?

When __import_wp_info() is called:

arch/s390/kvm/guestdbg.c:__import_wp_info() {
    ...
    wp_info->len = bp_data->len;
    if (wp_info->len < 0 || wp_info->len > MAX_WP_SIZE)
        return -EINVAL;

    wp_info->old_data = kmalloc(bp_data->len, GFP_KERNEL_ACCOUNT);
    ...
}

If userspace passes a large length like 0x100000005, wp_info->len
(a 32-bit signed integer) would truncate it to 5, passing the validation
check. However, kmalloc then receives the untruncated 64-bit bp_data->len,
attempting a large allocation that could trigger a warning and cause host
instability.

> +		}
>  	} else {
>  		kvm_s390_clear_cpuflags(vcpu, CPUSTAT_P);
>  		vcpu->arch.guestdbg.last_bp = 0;

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

[PATCH v3 7/8] KVM: s390: Free guest debug data on vcpu destroy

[Christian Borntraeger]

kvm_s390_clear_bp_data() is only called from
kvm_arch_vcpu_ioctl_set_guest_debug(), i.e. when user space changes or
disables debugging. A vCPU that is destroyed while hardware breakpoints
are still armed - the normal case when the VMM just exits or crashes -
leaks hw_bp_info, hw_wp_info and all old_data buffers, since generic KVM
frees the vCPU right after kvm_arch_vcpu_destroy().

That is bounded by MAX_BP_COUNT entries, so roughly 8 KiB per vCPU, but
it is unbounded over VM lifetimes. The allocations are
GFP_KERNEL_ACCOUNT, so the charge also outlives the exiting process and
pins dying memcgs.

Fix by clearing the debug data on vCPU destruction. Calling it
unconditionally is fine: struct kvm_vcpu is zero allocated, so for a vCPU
that never enabled debugging the counters are 0 and the pointers NULL.

Fixes: 27291e2165b6 ("KVM: s390: hardware support for guest debugging")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e0548926dc49..ccbdb109e22c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3409,6 +3409,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	trace_kvm_s390_destroy_vcpu(vcpu->vcpu_id);
 	kvm_s390_clear_local_irqs(vcpu);
 	kvm_clear_async_pf_completion_queue(vcpu);
+	kvm_s390_clear_bp_data(vcpu);
 	if (!kvm_is_ucontrol(vcpu->kvm))
 		sca_del_vcpu(vcpu);
 	kvm_s390_update_topology_change_report(vcpu->kvm, 1);
-- 
2.53.0

Re: [PATCH v3 7/8] KVM: s390: Free guest debug data on vcpu destroy

[sashiko-bot]

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

Pre-existing issues:
- [High] Integer truncation in `__import_wp_info()` allows bounds check bypass and host Denial of Service via a massive `kmalloc()` allocation.
--

--- Patch [7]: [PATCH v3 7/8] KVM: s390: Free guest debug data on vcpu destroy ---
commit 9d3d46230dbc0cbf211fd1bcc98310a2f3252d6f
Author: Christian Borntraeger <borntraeger@linux.ibm.com>

KVM: s390: Free guest debug data on vcpu destroy

kvm_s390_clear_bp_data() is only called from
kvm_arch_vcpu_ioctl_set_guest_debug(), i.e. when user space changes or
disables debugging. A vCPU that is destroyed while hardware breakpoints
are still armed leaks hw_bp_info, hw_wp_info and all old_data buffers.
This commit fixes the issue by clearing the debug data on vCPU destruction.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 0a197e055cad0..095c9ba0aa5cc 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3410,6 +3410,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>  	trace_kvm_s390_destroy_vcpu(vcpu->vcpu_id);
>  	kvm_s390_clear_local_irqs(vcpu);
>  	kvm_clear_async_pf_completion_queue(vcpu);
> +	kvm_s390_clear_bp_data(vcpu);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the debug
data cleanup I noticed a potential integer truncation in __import_wp_info().

Could a malicious user bypass the bounds check by providing a massive 64-bit
length?

In arch/s390/include/asm/kvm_host.h, struct kvm_hw_wp_info_arch defines
len as a 32-bit integer:

struct kvm_hw_wp_info_arch {
	unsigned long addr;
	unsigned long phys_addr;
	int len;
	char *old_data;
};

When __import_wp_info() in arch/s390/kvm/guestdbg.c imports the data:

	wp_info->len = bp_data->len;
	...
	if (wp_info->len < 0 || wp_info->len > MAX_WP_SIZE)
		return -EINVAL;

	wp_info->old_data = kmalloc(bp_data->len, GFP_KERNEL_ACCOUNT);

If a user-space process calls the KVM_SET_GUEST_DEBUG ioctl and sets
bp_data->len to a value like 0x100000005, would the 32-bit truncation
cause wp_info->len to become 5?

If so, this would pass the MAX_WP_SIZE check, but the kmalloc() call
would use the original 64-bit bp_data->len, resulting in an enormous
allocation request.

Since GFP_KERNEL_ACCOUNT lacks __GFP_NOWARN, couldn't this trigger a
WARN_ON_ONCE splat in the page allocator and cause a host panic on
systems with panic_on_warn enabled?

>  	if (!kvm_is_ucontrol(vcpu->kvm))
>  		sca_del_vcpu(vcpu);
>  	kvm_s390_update_topology_change_report(vcpu->kvm, 1);

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

[PATCH v3 8/8] KVM: s390: Fix length check __import_wp_info()

[Christian Borntraeger]

struct kvm_hw_breakpoint::len is a __u64 that is fully controlled by user
space. This is then assigned to wp_info->len, which is an int. The bounds
check is done on the truncated value while the allocation uses the
untruncated one:

	wp_info->len = bp_data->len;
	[...]
	if (wp_info->len < 0 || wp_info->len > MAX_WP_SIZE)
		return -EINVAL;

	wp_info->old_data = kmalloc(bp_data->len, GFP_KERNEL_ACCOUNT);

Use the validated value for the allocation as intended. Without this
fix userspace can trigger >4GB allocations which will fail and result
in a WARN due to MAX_PAGE_ORDER.

Fixes: 27291e2165b6 ("KVM: s390: hardware support for guest debugging")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 arch/s390/kvm/guestdbg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index f7c94d54efbe..9a6149e310bb 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -184,7 +184,7 @@ static int __import_wp_info(struct kvm_vcpu *vcpu,
 	if (wp_info->len < 0 || wp_info->len > MAX_WP_SIZE)
 		return -EINVAL;
 
-	wp_info->old_data = kmalloc(bp_data->len, GFP_KERNEL_ACCOUNT);
+	wp_info->old_data = kmalloc(wp_info->len, GFP_KERNEL_ACCOUNT);
 	if (!wp_info->old_data)
 		return -ENOMEM;
 	/* try to backup the original value */
-- 
2.53.0

Re: [PATCH v3 8/8] KVM: s390: Fix length check __import_wp_info()

[sashiko-bot]

> struct kvm_hw_breakpoint::len is a __u64 that is fully controlled by user
> space. This is then assigned to wp_info->len, which is an int. The bounds
> check is done on the truncated value while the allocation uses the
> untruncated one:
> 
> 	wp_info->len = bp_data->len;
> 	[...]
> 	if (wp_info->len < 0 || wp_info->len > MAX_WP_SIZE)
> 		return -EINVAL;
> 
> 	wp_info->old_data = kmalloc(bp_data->len, GFP_KERNEL_ACCOUNT);
> 
> Use the validated value for the allocation as intended. Without this
> fix userspace can trigger >4GB allocations which will fail and result
> in a WARN due to MAX_PAGE_ORDER.
> 
> Fixes: 27291e2165b6 ("KVM: s390: hardware support for guest debugging")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v3 0/8] KVM: s390: More fixes

[Claudio Imbrenda]

On Wed,  5 Aug 2026 13:04:47 +0200
Christian Borntraeger <borntraeger@linux.ibm.com> wrote:

> v2->v3:
> - add length check fix for guest debug
> - add RB
> 
> v1->v2: several sashiko reports added
> - machine check stack exposure
> - extend zeroing to pfault done interrupts
> - old_data leak
> - srcu protection missing
> - guest debug allocations not freed on destroy
> - uppercase patch title

Whole series:
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> 
> 
> Christian Borntraeger (8):
>   KVM: s390: Remove user triggerable WARN_ON
>   KVM: s390: Zero initialize data structures for inject_pfault_token
>   KVM: s390: Zero initialize irq in reinject_machine_check
>   KVM: s390: Fix memory leak in guest debug handling
>   KVM: s390: Fix old_data leak in guest debug error path
>   KVM: s390: Take srcu when importing watchpoint data
>   KVM: s390: Free guest debug data on vcpu destroy
>   KVM: s390: Fix length check __import_wp_info()
> 
>  arch/s390/kvm/guestdbg.c  |  9 +++++++--
>  arch/s390/kvm/interrupt.c |  4 ++--
>  arch/s390/kvm/kvm-s390.c  | 11 +++++++----
>  3 files changed, 16 insertions(+), 8 deletions(-)
>