Re: [PATCH v2 6/7] KVM: s390: Take srcu when importing watchpoint data

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

New issues:
- [High] Holding kvm->srcu across a copy_from_user() operation introduces a userfaultfd deadlock vulnerability.
- [Low] Mixing goto-based error handling and scope-based cleanup helpers in the same function.

Pre-existing issues:
- [High] Hardware breakpoint and watchpoint data allocated via KVM_SET_GUEST_DEBUG is leaked upon VCPU destruction.
- [High] Integer truncation bypasses length validation, allowing unbounded kmalloc requests and kernel log spam.
--

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

KVM: s390: Take srcu when importing watchpoint data

This commit wraps the call to kvm_s390_import_bp_data() in an SRCU read-side
critical section using scoped_guard(). This is done to ensure that the
memslots array is not freed concurrently during the watchpoint import
process.

> 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: Low]
This isn't a bug, but does mixing scoped_guard() with the existing goto-based
error handling in this function violate the cleanup guidelines?

According to include/linux/cleanup.h, mixing goto-based error handling with
scope-based cleanup helpers in the same function can create confusing
ownership semantics. The guidelines suggest converting all resources that
need a goto cleanup to scope-based cleanup, or converting none of them. In
this function, vcpu_put(vcpu) is still handled via a goto out; label.

> +				rc = kvm_s390_import_bp_data(vcpu, dbg);

[Severity: High]
Could holding the SRCU read lock across this call introduce a userfaultfd
deadlock?

Inside kvm_s390_import_bp_data(), the very first operation is:

arch/s390/kvm/guestdbg.c:kvm_s390_import_bp_data() {
    ...
    bp_data = memdup_array_user(dbg->arch.hw_bp, dbg->arch.nr_hw_bp,
                                sizeof(*bp_data));
    ...
}

If user space maps this array using userfaultfd, the VCPU thread will block
in copy_from_user() while holding the kvm->srcu read lock. If the user space
userfaultfd handler attempts to execute a VM-wide KVM ioctl that modifies
memslots (such as KVM_SET_USER_MEMORY_REGION), that ioctl will call
synchronize_srcu(&kvm->srcu). The VCPU thread would wait for the page fault
to be resolved, and the userfaultfd handler would wait for the SRCU lock to
be released.

[Severity: High]
This isn't a bug introduced by this patch, but does kvm_s390_import_bp_data()
leak the dynamically allocated debug information upon VCPU destruction?

kvm_s390_import_bp_data() allocates memory for wp_info, bp_info, and old_data,
and assigns them to vcpu->arch.guestdbg. While these are freed during a
subsequent KVM_SET_GUEST_DEBUG ioctl via kvm_s390_clear_bp_data(vcpu), it
appears kvm_s390_clear_bp_data() is never invoked during VCPU destruction:

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_destroy() {
    ...
    // No call to kvm_s390_clear_bp_data()
    ...
}

Could an unprivileged user who can create VCPUs repeatedly set hardware
breakpoints and then close the VCPU file descriptor to leak this memory?

[Severity: High]
This is a pre-existing issue, but can an integer truncation in
__import_wp_info() bypass length validation and cause unbounded kmalloc
requests?

When kvm_s390_import_bp_data() processes watchpoints, it calls
__import_wp_info(), where the user-provided __u64 length is truncated to an
int for the bounds check:

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 a malicious user provides a length of 4GB + 1, the truncated integer
becomes 1, bypassing the bounds check. The code then calls kmalloc() with
the original __u64 size. Since it lacks __GFP_NOWARN, won't this trigger a
memory allocation warning dump in the kernel log?

> +		}
>  	} 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