Re: [PATCH v3 6/8] KVM: s390: Take srcu when importing watchpoint data

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

New issues:
- [Medium] Mixing goto-based error handling and scope-based cleanup helpers in the same function.

Pre-existing issues:
- [High] VCPU guest debug hardware breakpoints are leaked when the VCPU is destroyed.
- [High] Integer truncation in watchpoint length check bypasses MAX_WP_SIZE, leading to a massive kmalloc and WARN_ON_ONCE.
--

--- Patch [6]: [PATCH v3 6/8] KVM: s390: Take srcu when importing watchpoint data ---
commit e0e29f54e03d9a52cf7d59a9894e63da62df0c7d
Author: Christian Borntraeger <[email protected]>

KVM: s390: Take srcu when importing watchpoint data

This commit adds a scoped_guard around kvm_s390_import_bp_data to ensure
the srcu lock is held while backing up original guest memory contents of a
watchpoint. This prevents concurrent memslot updates from freeing the
memslots array during the read operation.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 611dcb5342101..0a197e055cad0 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4248,8 +4248,10 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  		/* enforce guest PER */
>  		kvm_s390_set_cpuflags(vcpu, CPUSTAT_P);
>  
> -		if (dbg->control & KVM_GUESTDBG_USE_HW_BP)
> -			rc = kvm_s390_import_bp_data(vcpu, dbg);
> +		if (dbg->control & KVM_GUESTDBG_USE_HW_BP) {
> +			scoped_guard(srcu, &vcpu->kvm->srcu)

[Severity: Medium]
Does mixing scoped_guard with the existing goto out; error handling
in this function violate the cleanup subsystem guidelines?

The documentation states that for a given routine, we should convert all
resources that need a goto cleanup to scope-based cleanup, or convert
none of them.

> +				rc = kvm_s390_import_bp_data(vcpu, dbg);

[Severity: High]
This isn't a bug introduced by this patch, but does the guest debug memory
get leaked when the VCPU is destroyed?

Looking at the memory allocated here in kvm_s390_import_bp_data():

arch/s390/kvm/guestdbg.c:kvm_s390_import_bp_data() {
    ...
    wp_info = kmalloc_objs(*wp_info, nr_wp, GFP_KERNEL_ACCOUNT);
    ...
}

It seems the cleanup path kvm_arch_vcpu_destroy() fails to call
kvm_s390_clear_bp_data(vcpu) to release these resources. Could this allow
a host userspace process to slowly drain host memory by repeatedly
creating and terminating VMs with hardware breakpoints?

[Severity: High]
This is a pre-existing issue, but is there an integer truncation here that
bypasses the MAX_WP_SIZE check?

When __import_wp_info() is called:

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 userspace passes a large length like 0x100000005, wp_info->len
(a 32-bit signed integer) would truncate it to 5, passing the validation
check. However, kmalloc then receives the untruncated 64-bit bp_data->len,
attempting a large allocation that could trigger a warning and cause host
instability.

> +		}
>  	} else {
>  		kvm_s390_clear_cpuflags(vcpu, CPUSTAT_P);
>  		vcpu->arch.guestdbg.last_bp = 0;

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