Re: [PATCH v2 3/7] KVM: s390: Fix race in __do_essa()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Replacing `kvm->slots_lock` with `kvm->slots_arch_lock` removes protection for `kvm_memslots(kvm)` iteration in `kvm_s390_vm_start_migration()`, leading to lockdep splats and Use-After-Free.
- [Medium] The patch updates migration ioctls and `GET_CMMA_BITS` to use `slots_arch_lock`, but leaves `SET_CMMA_BITS` using `slots_lock`, breaking their mutual exclusion.
- [Medium] The commit message claims to use `kvm->slots_lock`, but the patch implements the fix using `kvm->slots_arch_lock`.
--

--- Patch [3]: [PATCH v2 3/7] KVM: s390: Fix race in __do_essa() ---
commit 71095d9c71a69950c39647a27ad09d197d37fa2b
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix race in __do_essa()

This commit addresses an unlikely race condition between __do_essa() and 
migration or cmma handlers. It intends to fix this by acquiring an additional 
lock during these operations.

> KVM: s390: Fix race in __do_essa()
> 
> An unlikely race between __do_essa() and kvm_s390_vm_start_migration(),
> kvm_s390_vm_stop_migration(), or dat_get_cmma() was possible.
> 
> Fix by locking kvm->slots_lock. Since this is not a hot path, the
> overhead of an additional mutex is negligible.

[Severity: Medium]
The commit message states we are locking kvm->slots_lock, but the code
below acquires kvm->slots_arch_lock. Could the commit message be updated
to match the actual implementation?

> Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 9 ++++-----
>  arch/s390/kvm/priv.c     | 5 +++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 53691aaf6534c..9e3b8b5c6aa66 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1298,7 +1298,8 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
>  {
>  	int res = -ENXIO;
>  
> -	mutex_lock(&kvm->slots_lock);
> +	guard(mutex)(&kvm->slots_arch_lock);

[Severity: High]
By switching from slots_lock to slots_arch_lock here, does this leave the
memslots iteration in kvm_s390_vm_start_migration() unprotected?

When kvm_s390_vm_start_migration() executes, it accesses the memslots:

kvm_s390_vm_start_migration() {
    ...
    slots = kvm_memslots(kvm);
    if (!slots || kvm_memslots_empty(slots))
        return -EINVAL;
    ...
    kvm_for_each_memslot(ms, bkt, slots) {
    ...
}

If we aren't holding kvm->slots_lock or kvm->srcu, can a concurrent memslot
update (like KVM_SET_USER_MEMORY_REGION) free the memslots array while it
is being accessed, potentially triggering a use-after-free and a lockdep
splat?

> +
>  	switch (attr->attr) {
>  	case KVM_S390_VM_MIGRATION_START:
>  		res = kvm_s390_vm_start_migration(kvm);

[ ... ]

> @@ -2996,9 +2996,8 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  		r = -EFAULT;
>  		if (copy_from_user(&args, argp, sizeof(args)))
>  			break;
> -		mutex_lock(&kvm->slots_lock);
> -		r = kvm_s390_get_cmma_bits(kvm, &args);
> -		mutex_unlock(&kvm->slots_lock);
> +		scoped_guard(mutex, &kvm->slots_arch_lock)
> +			r = kvm_s390_get_cmma_bits(kvm, &args);

[Severity: Medium]
Since GET_CMMA_BITS is updated to use kvm->slots_arch_lock here, is there a
risk of breaking mutual exclusion with KVM_S390_SET_CMMA_BITS?

Looking at KVM_S390_SET_CMMA_BITS further down in kvm_arch_vm_ioctl():

case KVM_S390_SET_CMMA_BITS: {
    ...
    mutex_lock(&kvm->slots_lock);
    r = kvm_s390_set_cmma_bits(kvm, &args);
    mutex_unlock(&kvm->slots_lock);
    break;
}

Since they now use different locks, can KVM_S390_SET_CMMA_BITS execute
concurrently with KVM_S390_GET_CMMA_BITS and migration operations, leading
to unpredictable CMMA state transitions?

>  		if (!r) {
>  			r = copy_to_user(argp, &args, sizeof(args));
>  			if (r)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713150857.269954-1-imbrenda@linux.ibm.com?part=3