Re: [PATCH 3/3] KVM: s390: 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] In `kvm_s390_import_bp_data()`, if importing a hardware watchpoint fails on a subsequent iteration, the dynamically allocated `old_data` buffers from watchpoints imported in previous iterations are permanently leaked.
- [High] The KVM_SET_GUEST_DEBUG ioctl calls `read_guest_abs()` (which internally accesses KVM memslots) without holding the `kvm->srcu` lock, violating KVM memslot locking rules and risking a Use-After-Free.
- [High] Leak of hardware watchpoint and breakpoint structures upon vCPU destruction because `kvm_s390_clear_bp_data()` is never called in the vCPU teardown path.
--

--- Patch [3]: [PATCH 3/3] KVM: s390: memory leak in guest debug handling ---
commit 56d3bf3acb3de4323dcc94259c523dacec14841e
Author: Christian Borntraeger <[email protected]>

KVM: s390: 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")
Signed-off-by: Christian Borntraeger <[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);
>  	return 0;
>  error:
>  	kfree(bp_data);
>  	kfree(wp_info);

[Severity: High]
This is a pre-existing issue, but do we also leak the dynamically allocated
old_data buffers for watchpoints on this error path?

When iterating over the breakpoints, if a later hardware watchpoint fails
validation, the function jumps to this error path. It frees the wp_info
array, but does not appear to loop over the already processed wp_info
elements to free the wp_info->old_data buffers that were allocated in
__import_wp_info().

Could an unprivileged user trigger this error path repeatedly to exhaust
memory?

>  	kfree(bp_info);
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does read_guest_abs() get called without
holding the kvm->srcu lock when importing watchpoints?

arch/s390/kvm/guestdbg.c:__import_wp_info() {
    ...
    wp_info->old_data = kmalloc(bp_data->len, GFP_KERNEL_ACCOUNT);
    ...
    ret = read_guest_abs(vcpu, wp_info->phys_addr, wp_info->old_data,
                         wp_info->len);
    ...
}

The KVM_SET_GUEST_DEBUG ioctl takes vcpu->mutex but not kvm->srcu. The call
chain __import_wp_info() -> read_guest_abs() -> kvm_read_guest() ->
gfn_to_memslot() expects kvm->srcu to be held to prevent use-after-free if
a memslot is concurrently removed. Should the SRCU lock be acquired before
reading guest memory here?

[Severity: High]
This is a pre-existing issue, but do we leak the guest debug structures if a
vCPU is destroyed while debugging is still enabled?

arch/s390/kvm/guestdbg.c:kvm_s390_import_bp_data() {
    ...
    vcpu->arch.guestdbg.hw_bp_info = bp_info;
    vcpu->arch.guestdbg.nr_hw_wp = nr_wp;
    vcpu->arch.guestdbg.hw_wp_info = wp_info;
    ...
}

If the userspace VMM process exits or crashes, kvm_arch_vcpu_destroy() is
called. However, kvm_arch_vcpu_destroy() in arch/s390/kvm/kvm-s390.c does not
appear to call kvm_s390_clear_bp_data() to clean up these dynamically
allocated arrays.

Would it be possible for an attacker to exhaust host memory by repeatedly
spawning and destroying debug-enabled VMs?

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