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

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

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