Re: [PATCH v2 4/7] 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:
- [Critical] Missing SRCU read lock protection in KVM guest debug memory access leading to Use-After-Free risk.
- [High] Hardware breakpoint and watchpoint data structures are permanently leaked when a VCPU is destroyed.
- [High] Integer truncation of `bp_data->len` bypasses bounds checking and allows maliciously large memory allocations.
--

--- Patch [4]: [PATCH v2 4/7] KVM: s390: Fix memory leak in guest debug handling ---
commit ed2dab291478e4d3632d170d668feb5781798aba
Author: Christian Borntraeger <[email protected]>

KVM: s390: Fix memory leak in guest debug handling

This commit fixes a memory leak in the s390 guest debug handling. It ensures
that the bp_data array allocated during KVM_SET_GUEST_DEBUG is properly freed
on successful paths, rather than only in the error case.

> 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: Critical]
This is a pre-existing issue, but since this ioctl path is being modified,
is there a missing SRCU read lock when accessing guest memory?

When a user issues KVM_SET_GUEST_DEBUG, the ioctl executes the following path:
kvm_arch_vcpu_ioctl_set_guest_debug() -> kvm_s390_import_bp_data() ->
__import_wp_info() -> read_guest_abs()

In arch/s390/kvm/guestdbg.c:__import_wp_info():
	ret = read_guest_abs(vcpu, wp_info->phys_addr, wp_info->old_data,
			     wp_info->len);

The ioctl path only holds vcpu->mutex, but read_guest_abs() translates the
guest physical address using gfn_to_memslot(). Without holding kvm->srcu, could
concurrent memslot updates (like KVM_SET_USER_MEMORY_REGION) free the memslot
while it is being accessed, leading to a use-after-free?


[Severity: High]
This isn't a bug introduced by this patch, but does the guest debug length
check properly prevent large memory allocations?

In arch/s390/kvm/guestdbg.c:__import_wp_info():
	wp_info->len = bp_data->len;
	wp_info->addr = bp_data->addr;
	wp_info->phys_addr = bp_data->phys_addr;
	wp_info->old_data = NULL;

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

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

The user-controlled bp_data->len is a 64-bit value, but wp_info->len is a
32-bit integer. If a malicious user passes a carefully crafted 64-bit length
such as 4294967301, could it truncate to 5 when assigned to wp_info->len,
bypass the MAX_WP_SIZE check, and then trigger an oversized allocation when
the original 64-bit bp_data->len is passed to kmalloc()?


[Severity: High]
This is a pre-existing issue, but does VCPU destruction completely clean up
the guest debug allocations?

When userspace creates a VCPU and sets hardware breakpoints via
KVM_SET_GUEST_DEBUG, arrays are dynamically allocated and attached to
vcpu->arch.guestdbg (hw_bp_info, hw_wp_info, and old_data).

In 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);

It appears kvm_arch_vcpu_destroy() is missing a call to
kvm_s390_clear_bp_data(). Could this allow unprivileged users to leak kernel
memory by repeatedly creating and destroying VCPUs?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4