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

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 <[email protected]>

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: [email protected]

> 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/[email protected]?part=4