Re: [PATCH v2 1/7] KVM: s390: Fix unlikely NULL gmap dereference

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Commit message and implementation mismatch regarding vCPU initialization
- [Medium] Data race and lack of synchronization barriers on vcpu->arch.initialized

Pre-existing issues:
- [Critical] Incomplete fix leaves KVM_S390_MEM_OP vulnerable to NULL pointer dereference
- [High] Data race on vcpu->arch.sie_block->epoch with KVM_SET_ONE_REG silently drops userspace configuration
--

--- Patch [1]: [PATCH v2 1/7] KVM: s390: Fix unlikely NULL gmap dereference ---
commit f0bb4780c8538aa3e30c05d74a7d340ba636dd73
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix unlikely NULL gmap dereference
    
When creating a new vCPU, kvm_vm_ioctl_create_vcpu() will call
kvm_arch_vcpu_postcreate() after the file descriptor for the new vCPU
has been created. The new file descriptor has not been returned yet,
but a malicious userspace program could try to guess it.

[ ... ]

> Fix this by moving the initialization of vcpu->arch.gmap into
> kvm_arch_vcpu_create(), which is called before the file descriptor for
> the vCPU is created.

[Severity: Medium]
The commit message states that the initialization of vcpu->arch.gmap is being
moved to kvm_arch_vcpu_create(), but the implementation instead adds a new
vcpu->arch.initialized flag and leaves the gmap initialization in
kvm_arch_vcpu_postcreate(). Is this description accurate?

[ ... ]

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 23c817595e28d..374dae6dae81b 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]
This is a pre-existing issue, but does overwriting the epoch here race with
userspace's KVM_SET_ONE_REG?

In kvm_arch_vcpu_postcreate(), the code overwrites the epoch without holding
vcpu->mutex:

	mutex_lock(&vcpu->kvm->lock);
	preempt_disable();
	vcpu->arch.sie_block->epoch = vcpu->kvm->arch.epoch;
	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
	preempt_enable();
	mutex_unlock(&vcpu->kvm->lock);

Since the file descriptor is already exposed, userspace could concurrently
execute KVM_SET_ONE_REG for KVM_REG_S390_EPOCHDIFF:

kvm_arch_vcpu_ioctl_set_one_reg() {
    ...
	case KVM_REG_S390_EPOCHDIFF:
		r = get_user(vcpu->arch.sie_block->epoch,
			     (u64 __user *)reg->addr);
    ...
}

Would the unlocked write in kvm_arch_vcpu_postcreate() silently clobber a
valid userspace configuration if KVM_SET_ONE_REG occurs just before it?

[Severity: Medium]
Could this WRITE_ONCE() allow compiler reordering? 

Since there is no release barrier like smp_store_release(), couldn't the
compiler sink the initialization of vcpu->arch.gmap (which still happens
earlier in this function) below this flag update?

>  }
>  
>  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 (!vcpu->arch.initialized)
> +		return -EINVAL;
> +

[Severity: Medium]
Since this flag is written concurrently in kvm_arch_vcpu_postcreate(), does
this plain C read create a data race?  Should this be using READ_ONCE() or
smp_load_acquire() to ensure the memory accesses to vcpu->arch.gmap are safely
ordered?

[Severity: Critical]
This is a pre-existing issue, but does this fix leave KVM_S390_MEM_OP
vulnerable to a NULL pointer dereference?

The patch prevents KVM_RUN from executing before initialization, but doesn't
add a similar check for KVM_S390_MEM_OP. A concurrent KVM_S390_MEM_OP
handler will proceed:

kvm_s390_vcpu_mem_op() {
    ...
		r = check_gva_range(vcpu, mop->gaddr, mop->ar, mop->size,
				    acc_mode, mop->key);
    ...
}

If the KVM_S390_MEMOP_F_SKEY_PROTECTION flag is set, it routes to:

vcpu_check_access_key_gpa() {
    ...
	scoped_guard(read_lock, &vcpu->kvm->mmu_lock)
		r = dat_get_storage_key(vcpu->arch.gmap->asce, gpa_to_gfn(gpa), &storage_key);
    ...
}

Could this unconditionally dereference vcpu->arch.gmap->asce while it is still
NULL, resulting in a kernel panic?

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

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