Re: [PATCH v1 3/8] KVM: s390: Fix unlikely race with KVM_CAP_S390_USER_OPEREXEC

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The WRITE_ONCE() fix for KVM_CAP_S390_USER_OPEREXEC is architecturally insufficient to prevent hardware Store-Load reordering, leaving the concurrency race exploitable.
- [High] KVM_CAP_S390_USER_INSTR0 suffers from the identical concurrency vulnerability as USER_OPEREXEC, but was completely missed by the patch.
--

commit d2c961d1e461c8babbad1df33fbe8ed3bddfddba
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix unlikely race with KVM_CAP_S390_USER_OPEREXEC

This commit attempts to fix a race condition when enabling
KVM_CAP_S390_USER_OPEREXEC concurrently with vCPU creation. It uses
WRITE_ONCE() to ensure the capability flag is set before updating all
vCPUs.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 4a6d903e35231..784f7d9c79c74 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c

[ ... ]

> @@ -955,7 +955,7 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>  		break;
>  	case KVM_CAP_S390_USER_OPEREXEC:
>  		VM_EVENT(kvm, 3, "%s", "ENABLE: CAP_S390_USER_OPEREXEC");
> -		kvm->arch.user_operexec = 1;
> +		WRITE_ONCE(kvm->arch.user_operexec, 1);
>  		icpt_operexc_on_all_vcpus(kvm);

[Severity: High]
This isn't a bug introduced by this patch, but is WRITE_ONCE() sufficient here
to prevent hardware Store-Load reordering?

On s390x, hardware Store-Load reordering is permitted. When Thread A enables
the capability, it stores to user_operexec and then loads online_vcpus inside
icpt_operexc_on_all_vcpus():

Thread A:
    WRITE_ONCE(kvm->arch.user_operexec, 1);
    // icpt_operexc_on_all_vcpus()
    kvm_for_each_vcpu(i, vcpu, kvm) { ... } 

Thread B concurrently creates a vCPU, incrementing online_vcpus and then
loading user_operexec in kvm_arch_vcpu_postcreate():

Thread B:
    atomic_inc(&kvm->online_vcpus);
    if (... || vcpu->kvm->arch.user_operexec)
        vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;

Without a full memory barrier (smp_mb()), Thread A's CPU can execute the load
of online_vcpus before the store to user_operexec is globally visible. Thread
A might miss the new vCPU, and Thread B might read user_operexec == 0. Does
this require an explicit smp_mb() between the store and the loop?

[Severity: High]
This is a pre-existing issue, but does KVM_CAP_S390_USER_INSTR0 suffer from the
exact same concurrency vulnerability?

In the same kvm_vm_ioctl_enable_cap() function, KVM_CAP_S390_USER_INSTR0 uses
the identical pattern:

    kvm->arch.user_instr0 = 1;
    icpt_operexc_on_all_vcpus(kvm);

Since kvm_arch_vcpu_postcreate() also reads user_instr0:

    if (test_kvm_facility(vcpu->kvm, 74) || vcpu->kvm->arch.user_instr0 ||
        vcpu->kvm->arch.user_operexec)
        vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;

Could a concurrent vCPU creation fail to enable the ICTL_OPEREXC intercept if
it reads user_instr0 == 0 while the enablement loop misses the new vCPU?

>  		r = 0;
>  		break;

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