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

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

Pre-existing issues:
- [Critical] Missing SRCU read lock around KVM memslot access during KVM_SET_GUEST_DEBUG causes a Use-After-Free vulnerability.
- [High] Pre-existing memory leak of guest debug arrays (`hw_wp_info`, `hw_bp_info`) and per-watchpoint `old_data` buffers on vCPU destruction.
- [High] Integer truncation of `bp_data->len` allows unconstrained memory allocation, leading to a kernel WARNING and potential Denial of Service.
--

--- Patch [5]: [PATCH v2 5/7] KVM: s390: Fix old_data leak in guest debug error path ---
commit a3583776c5b24b471704343b4d20b599ed4ab8ca
Author: Christian Borntraeger <[email protected]>

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.

Cc: [email protected]

> 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: Critical]
This is a pre-existing issue, but does this call path lack SRCU protection?

When KVM_SET_GUEST_DEBUG calls __import_wp_info(), it eventually invokes
read_guest_abs() which dereferences the KVM memslots array via
gfn_to_memslot().

Neither kvm_vcpu_ioctl() nor kvm_arch_vcpu_ioctl_set_guest_debug() appears
to acquire the kvm->srcu read lock. Can a concurrent
KVM_SET_USER_MEMORY_REGION ioctl free the memslot structure while it is
being accessed, triggering a use-after-free?


[Severity: High]
This is a pre-existing issue, but is there an integer truncation bug inside
__import_wp_info() for the length parameter?

bp_data->len is a 64-bit user-controlled value, which is assigned to
wp_info->len (a 32-bit signed int):

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);
    ...
}

By providing a large 64-bit value (e.g., 0x100000005), the lower 32 bits
would pass the bounds check (5 <= MAX_WP_SIZE), but the untruncated 64-bit
value is then passed to kmalloc().

Can this lead to an unbounded memory allocation exceeding MAX_PAGE_ORDER,
triggering a kernel warning?

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

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does kvm_arch_vcpu_destroy() leak these
dynamically allocated buffers when tearing down a vCPU?

If the VM is destroyed while guest debugging is active,
kvm_arch_vcpu_destroy() seems to skip calling kvm_s390_clear_bp_data()
to free hw_wp_info, hw_bp_info, and the old_data buffers:

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_destroy() {
    ...
    VCPU_EVENT(vcpu, 3, "%s", "free cpu");
    ...
}

Could a host userspace process repeatedly create and destroy vCPUs with
guest debug enabled to exhaust host memory?

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

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