Re: [PATCH v8 05/13] KVM: s390: ucontrol: Fix sca_clear_ext_call()

[Claudio Imbrenda]

On Mon, 3 Aug 2026 16:50:30 +0200
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:

> On 8/3/26 14:40, Claudio Imbrenda wrote:
> > When cleaning up a UCONTROL VM, sca_clear_ext_call() will touch memory
> > outside of the allocated ESCA block, and UCONTROL VMs don't even use
> > ESCA.
> > 
> > Fix by not touching ESCA for UCONTROL VMs, and fence the
> > KVM_S390_INTERRUPT ioctl altogether. Add extra checks in
> > sca_ext_call_pending() and sca_inject_ext_call() to make sure UCONTROL
> > VMs won't touch ESCA.
> > 
> > Fencing does not cause regressions with userspace, since UCONTROL VMs
> > never used KVM_S390_INTERRUPT ioctls.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > Fixes: 7d43bafcff17 ("KVM: s390: Make provisions for ESCA utilization")


[...]


> > @@ -84,10 +90,13 @@ static int sca_inject_ext_call(struct kvm_vcpu *vcpu, int src_id)
> >   static void sca_clear_ext_call(struct kvm_vcpu *vcpu)
> >   {
> >   	struct esca_block *sca = vcpu->kvm->arch.sca;
> > -	union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
> > +	union esca_sigp_ctrl *sigp_ctrl;
> >   
> > -	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized)
> > +	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized || kvm_is_ucontrol(vcpu->kvm))
> >   		return;
> > +
> > +	/* Initialize after the above check, to prevent going out of bounds */
> > +	sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;  
> 
> Not sure why you only add this here and not at the other two occurences.

because the other two were added later and I forgot about the comment :)

> But I don't think we need these comments at all.
> 
> Dereferencing things before a check is not a great idea in most cases. 
> Especially if the check validates if the memory has been set up at all :)
> 
> >   	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_ECALL_PEND);
> >   
> >   	WRITE_ONCE(sigp_ctrl->value, 0);
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 5b2727d7dfd1..21574f57be72 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2934,6 +2934,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> >   	case KVM_S390_INTERRUPT: {
> >   		struct kvm_s390_interrupt s390int;
> >   
> > +		r = -EINVAL;
> > +		if (kvm_is_ucontrol(kvm))
> > +			break;
> >   		r = -EFAULT;
> >   		if (copy_from_user(&s390int, argp, sizeof(s390int)))
> >   			break;
> > @@ -5456,6 +5459,8 @@ long kvm_arch_vcpu_unlocked_ioctl(struct file *filp, unsigned int ioctl,
> >   		struct kvm_s390_interrupt s390int;
> >   		struct kvm_s390_irq s390irq = {};
> >   
> > +		if (kvm_is_ucontrol(vcpu->kvm))
> > +			return -EINVAL;
> >   		if (copy_from_user(&s390int, argp, sizeof(s390int)))
> >   			return -EFAULT;
> >   		if (s390int_to_s390irq(&s390int, &s390irq))  
> 
> Do we need changes to the api documentation for this rc?

The existing documentation is already not describing which error codes
are possible and when.