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

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 27.07.26 um 17:02 schrieb Claudio Imbrenda:
> 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.
> 
> If a malicious userspace program manages to start the newly created vCPU
> before kvm_arch_vcpu_postcreate() is called, __vcpu_run() will try to
> dereference vcpu->arch.gmap and trigger a NULL pointer dereference.
> 
> Fix this by adding a new field to struct kvm_vcpu_arch to keep track of
> the initialization status of the vCPU. Refuse to run a vCPU that is not
> fully initialized.
> 
> Fixes: dafd032a15f8 ("KVM: s390: move vcpu specific initalization to a later point")
> Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h | 1 +
>   arch/s390/kvm/kvm-s390.c         | 8 ++++++++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index eaa34c5bd3c1..edf75b6ad20c 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -440,6 +440,7 @@ struct kvm_vcpu_arch {
>   	bool skey_enabled;
>   	/* Indicator if the access registers have been loaded from guest */
>   	bool acrs_loaded;
> +	bool initialized;
>   	struct kvm_s390_pv_vcpu pv;
>   	union diag318_info diag318_info;
>   	struct kvm_s390_mmu_cache *mc;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 150b5dd2170e..22535f76ae05 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3613,6 +3613,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);

while highly unlikely, I think sashiko is right, the compiler could reoder the
gmap assignment. A barrier() before the WRITE_ONCE would do but smp_store_release
does not hurt (and it has some sanitizer support).

>   }>   
>   static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)
> @@ -5039,6 +5041,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;
> +

I think we really do not need smp_load_acquire here and below, but it certainly
is nicer to have equivalent pairs. So if you use a barrier, its fine to leave it
as is, but maybe simply use smp_load_acquire here and below and smp_store_release
above.

>   	vcpu_load(vcpu);
>   
>   	if (guestdbg_exit_pending(vcpu)) {
> @@ -5523,6 +5528,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>   	long r;
>   	u16 rc, rrc;
>   
> +	if (!READ_ONCE(vcpu->arch.initialized))
> +		return -EINVAL;
> +
>   	vcpu_load(vcpu);
>   
>   	switch (ioctl) {