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

[Janosch Frank <frankja@linux.ibm.com>]

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@linux.ibm.com>
> Fixes: 7d43bafcff17 ("KVM: s390: Make provisions for ESCA utilization")
> ---
>   arch/s390/kvm/interrupt.c | 19 ++++++++++++++-----
>   arch/s390/kvm/kvm-s390.c  |  5 +++++
>   2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 2acdff130fa6..6b3f97a7513b 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -45,13 +45,16 @@ static struct kvm_s390_gib *gib;
>   static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>   {
>   	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_test_cpuflags(vcpu, CPUSTAT_ECALL_PEND))
>   		return 0;
> +	if (kvm_is_ucontrol(vcpu->kvm))
> +		return 0;
>   
>   	BUG_ON(!kvm_s390_use_sca_entries());
>   
> +	sigp_ctrl = sca->cpu[vcpu->vcpu_id].sigp_ctrl;
>   	if (src_id)
>   		*src_id = sigp_ctrl.scn;
>   
> @@ -60,13 +63,16 @@ static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>   
>   static int sca_inject_ext_call(struct kvm_vcpu *vcpu, int src_id)
>   {
> -	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 old_val, new_val = {.scn = src_id, .c = 1};
> +	struct esca_block *sca = vcpu->kvm->arch.sca;
> +	union esca_sigp_ctrl *sigp_ctrl;
>   	int expect, rc;
>   
>   	BUG_ON(!kvm_s390_use_sca_entries());
> +	if (kvm_is_ucontrol(vcpu->kvm))
> +		return -EINVAL;
>   
> +	sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
>   	old_val = READ_ONCE(*sigp_ctrl);
>   	old_val.c = 0;
>   
> @@ -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.
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?