Re: [PATCH v5 04/13] KVM: s390: ucontrol: Fix sca_clear_ext_call()

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

New issues:
- [High] The newly added `KVM_BUG_ON` checks for UCONTROL VMs violate KVM subsystem guidelines against userspace-reachable assertions, creating a Denial of Service vulnerability.

Pre-existing issues:
- [Medium] Incomplete fix leaves an out-of-bounds memory read in `sca_ext_call_pending()` for UCONTROL VMs.
- [Low] Inconsistent fix leaves an out-of-bounds pointer calculation in `sca_inject_ext_call()` for UCONTROL VMs.
--

--- Patch [4]: [PATCH v5 04/13] KVM: s390: ucontrol: Fix sca_clear_ext_call() ---
commit 497994f9fead637806bdf9fa1fbe4ded72deee52
Author: Claudio Imbrenda <[email protected]>

KVM: s390: ucontrol: Fix sca_clear_ext_call()

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 opportunistically
add checks in sca_ext_call_pending() and sca_inject_ext_call().

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 2acdff130fa6c..d02724bd1829c 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -50,6 +50,8 @@ static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>  	if (!kvm_s390_test_cpuflags(vcpu, CPUSTAT_ECALL_PEND))
>  		return 0;
>  
> +	if (KVM_BUG_ON(kvm_is_ucontrol(vcpu->kvm), vcpu->kvm))
> +		return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but does this function still perform an
out-of-bounds memory read before we even reach this new check?

At the top of sca_ext_call_pending(), sigp_ctrl is initialized by value:

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;

For a UCONTROL VM, vcpu_id can be up to 254, but the ESCA array only has
248 slots. Does this cause an unconditional out-of-bounds memory read when
entering the function?

[Severity: High]
Could this KVM_BUG_ON check be triggered by host userspace?

According to the KVM subsystem guidelines, userspace-reachable BUG_ON or
WARN_ON calls act as a denial of service. If a host userspace process invokes
the KVM_S390_INTERRUPT ioctl to inject a KVM_S390_INT_EXTERNAL_CALL into a
UCONTROL VM, wouldn't this wedge the VM and potentially trigger a panic?

>  	BUG_ON(!kvm_s390_use_sca_entries());
>  
>  	if (src_id)

[ ... ]

> @@ -66,6 +68,8 @@ static int sca_inject_ext_call(struct kvm_vcpu *vcpu, int src_id)
>  	int expect, rc;
>  
>  	BUG_ON(!kvm_s390_use_sca_entries());
> +	if (KVM_BUG_ON(kvm_is_ucontrol(vcpu->kvm), vcpu->kvm))
> +		return -EINVAL;

[Severity: High]
Similar to the check in sca_ext_call_pending() above, is this KVM_BUG_ON
reachable from host userspace and acting as a denial of service?

[Severity: Low]
This isn't a bug introduced by this patch, but is the pointer calculation
for sigp_ctrl at the top of sca_inject_ext_call() also occurring before
this new check?

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;
        ...

While it is just an address calculation, for UCONTROL VMs it calculates an
out-of-bounds pointer. Could this be moved after the check, similar to how
it was fixed in sca_clear_ext_call() below?

>  
>  	old_val = READ_ONCE(*sigp_ctrl);
>  	old_val.c = 0;

[ ... ]

> @@ -84,10 +88,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;
>  	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_ECALL_PEND);
>  
>  	WRITE_ONCE(sigp_ctrl->value, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4