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

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

Pre-existing issues:
- [High] Missing SRCU lock when accessing memslots during guest debug setup leads to Use-After-Free.
--

--- Patch [5]: [PATCH v3 5/8] KVM: s390: Fix old_data leak in guest debug error path ---
commit efad1ff98c2af0fcd50545a3067b444c29f0bc33
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. Up to
MAX_BP_COUNT - 1 buffers of up to MAX_WP_SIZE bytes are leaked per failed
request, and the request can be repeated.

Create error handling for cleaning up all created old_data memory
areas.

> 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: High]
This isn't a bug introduced by this patch, but is it safe to call
__import_wp_info() here without holding the SRCU read lock?

__import_wp_info() in arch/s390/kvm/guestdbg.c attempts to read guest memory:

arch/s390/kvm/guestdbg.c:__import_wp_info() {
    ...
	/* try to backup the original value */
	ret = read_guest_abs(vcpu, wp_info->phys_addr, wp_info->old_data,
			     wp_info->len);
    ...
}

This ultimately accesses the KVM memslots array.

Because the KVM_SET_GUEST_DEBUG ioctl entry point doesn't acquire the SRCU read
lock before calling kvm_s390_import_bp_data():

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_ioctl_set_guest_debug() {
    ...
		if (dbg->control & KVM_GUESTDBG_USE_HW_BP)
			rc = kvm_s390_import_bp_data(vcpu, dbg);
    ...
}

Could a concurrent memslot update (such as another thread calling
KVM_SET_USER_MEMORY_REGION on the VM) free the memslots array while it is being
accessed by this debug ioctl, leading to a Use-After-Free?

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

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