Re: [PATCH v3 01/10] KVM: s390: Fix unlikely NULL gmap dereference

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing compiler barrier in vCPU initialization publication allows compiler reordering, potentially defeating the NULL pointer dereference fix.
--

--- Patch [1]: [PATCH v3 01/10] KVM: s390: Fix unlikely NULL gmap dereference ---
commit 5328cd0469f8706fce5babcef6c93d98cfcef985
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix unlikely NULL gmap dereference

This commit fixes a potential NULL pointer dereference if a malicious
userspace program guesses a newly created vCPU file descriptor and
starts it before kvm_arch_vcpu_postcreate() finishes. It adds an
initialized flag that is checked before allowing vCPU ioctls to run.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 23c817595e28d..eff0644b8517d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3611,6 +3611,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  	if (test_kvm_facility(vcpu->kvm, 74) || vcpu->kvm->arch.user_instr0 ||
>  	    vcpu->kvm->arch.user_operexec)
>  		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;
> +
> +	WRITE_ONCE(vcpu->arch.initialized, true);

[Severity: High]
Could the compiler reorder the gmap assignment after this WRITE_ONCE()?

Since WRITE_ONCE() only guarantees volatile access but does not provide a
general compiler barrier, it might be possible for the compiler to legally
reorder the preceding non-volatile store (vcpu->arch.gmap = ...) to occur
after the initialized flag is set.

If this happens, a concurrent thread could read initialized as true but
still observe a NULL gmap in __vcpu_run(), triggering the exact crash this
patch intends to fix. 

Would it be safer to use smp_store_release() here to ensure the gmap
initialization is safely published?

>  }
>  
>  static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)

[ ... ]

> @@ -5037,6 +5039,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  	    kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
>  		return -EINVAL;
>  
> +	if (!READ_ONCE(vcpu->arch.initialized))
> +		return -EINVAL;

[Severity: High]
If smp_store_release() is used in kvm_arch_vcpu_postcreate(), would it make
sense to use smp_load_acquire() here to properly pair with it and guarantee
we observe the fully initialized vCPU state?

> +
>  	vcpu_load(vcpu);
>  
>  	if (guestdbg_exit_pending(vcpu)) {

[ ... ]

> @@ -5521,6 +5526,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	long r;
>  	u16 rc, rrc;
>  
> +	if (!READ_ONCE(vcpu->arch.initialized))
> +		return -EINVAL;

[Severity: High]
Similarly, should this be smp_load_acquire() to ensure memory visibility
guarantees match across all the vCPU ioctl paths?

> +
>  	vcpu_load(vcpu);
>  
>  	switch (ioctl) {

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