Re: [PATCH v5 16/21] KVM: s390: vsie: Shadow VSIE SCA in guest-1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] `get_vsie_page_cpu_nr()` incorrectly wraps positive return codes (like 1 for validity intercepts) in `ERR_PTR()`, bypassing `IS_ERR()` checks and causing a host kernel panic.
- [High] Stale `sca_gpa` in reused `vsie_sca` causes incorrect `xa_erase()` of valid shadow SCAs.
- [High] `get_vsie_sca()` incorrectly translates guest validity intercepts or unbacked SCA errors into KVM_RUN `-EINVAL` exits, abruptly crashing the VM.
- [Medium] Guest-triggerable `WARN_ON_ONCE` if `xa_store()` fails due to memory pressure.
--

commit 763d66e9883805b058573509abca88b10cdfb124
Author: Christoph Schlameuss <schlameuss@linux.ibm.com>

KVM: s390: vsie: Shadow VSIE SCA in guest-1
    
This commit restructures kvm_s390_handle_vsie() to create a guest-1 shadow
of the SCA if guest-2 attempts to enter SIE with an SCA. The original SCA
is pinned in guest-2 memory, and references to existing shadow SCAs are
stored in a map to efficiently reuse them.

> diff --git a/arch/s390/kvm/s390/vsie.c b/arch/s390/kvm/s390/vsie.c
> index 3c94faa35b44e..4bd11eaa7d81d 100644
> --- a/arch/s390/kvm/s390/vsie.c
> +++ b/arch/s390/kvm/s390/vsie.c
[ ... ]
> @@ -958,6 +1051,128 @@ static struct vsie_sca *alloc_vsie_sca(void)
>  	return vsie_sca;
>  }
>  
> +/* Clear the vsie_sca struct but keep the vsie_page references, mutex and ref_count */
> +static void clear_vsie_sca(struct vsie_sca *vsie_sca)
[ ... ]
> +/* Pin and get an existing or new guest-3 system control area.*/
> +static struct vsie_sca *get_vsie_sca(struct kvm_vcpu *vcpu, struct kvm_s390_sie_block *scb_o)
> +{
[ ... ]
> +	/* validate scb_o as we do not unshadow on error here */
> +	rc = validate_scao(vcpu, scb_o, sca_gpa);
> +	if (rc)
> +		return ERR_PTR(-EINVAL);

[Severity: High]
Does this code incorrectly translate a guest validity intercept into a fatal
host KVM error?

When validate_scao() fails, it returns a positive validity intercept value
(like 1). Returning ERR_PTR(-EINVAL) here overrides that intercept and
returns -EINVAL instead.

> +
> +	down_read(&kvm->arch.vsie.vsie_sca_lock);
> +	vsie_sca = get_vsie_sca_existing(kvm, sca_gpa, is_esca);
> +	up_read(&kvm->arch.vsie.vsie_sca_lock);
> +	if (vsie_sca)
> +		return vsie_sca;
[ ... ]
> +	/* check again under write lock if we are still under our vsie_sca limit */
> +	if (vsie_sca_new && kvm->arch.vsie.sca_count < max_vsie_sca) {
[ ... ]
> +	} else {
> +		/* reuse previously created vsie_sca allocation for different osca */
> +		vsie_sca = get_vsie_sca_unused(kvm);
> +		/* with nr_vcpus scas one must be reusable */
> +		if (IS_ERR(vsie_sca))
> +			goto out;
> +
> +		xa_erase(&kvm->arch.vsie.osca_to_sca, vsie_sca->sca_gpa >> SCA_ALIGNMENT_SHIFT);

[Severity: High]
If we are reusing an orphaned vsie_sca, does its sca_gpa still hold the old,
stale address? If so, does this xa_erase() inadvertently remove a valid
active shadow SCA from the osca_to_sca xarray?

[ ... ]
> +	rc = pin_sca(kvm, vsie_sca);
> +	if (rc) {
> +		vsie_sca->sca_gpa = ULONG_MAX;
> +		put_vsie_sca(vsie_sca);
> +		vsie_sca = ERR_PTR(rc);
> +		goto out;
> +	}
> +
> +	WARN_ON_ONCE(xa_store(&kvm->arch.vsie.osca_to_sca, vsie_sca->sca_gpa >> SCA_ALIGNMENT_SHIFT,
> +			      vsie_sca, GFP_KERNEL_ACCOUNT));

[Severity: Medium]
If xa_store() fails due to memory pressure, it returns an error pointer.
Since this is a non-null value, will this WARN_ON_ONCE trigger?

Guest-reachable warnings can be exploited for log spam or DoS.

> +
> +out:
> +	up_write(&kvm->arch.vsie.vsie_sca_lock);
> +	if (vsie_sca_new)
> +		free_vsie_sca(kvm, vsie_sca_new);
> +	return vsie_sca;
> +}
[ ... ]
> @@ -1815,11 +2027,165 @@ static struct vsie_page *get_vsie_page(struct kvm_vcpu *vcpu, unsigned long addr
[ ... ]
> +static struct vsie_page *get_vsie_page_cpu_nr(struct kvm_vcpu *vcpu, struct vsie_sca *vsie_sca,
> +					      gpa_t scb_gpa, u16 cpu_nr)
> +{
[ ... ]
> +	if (vsie_page->scb_gpa != scb_gpa || vsie_page->sca_gpa != vsie_sca->sca_gpa) {
> +		scoped_guard(mutex, &vcpu->kvm->arch.vsie.mutex) {
> +			unpin_scb(vcpu->kvm, vsie_page);
> +			rc = init_vsie_page(vcpu, vsie_page, scb_gpa);
> +		}
> +		if (rc) {
> +			put_vsie_page(vsie_page);
> +			return ERR_PTR(rc);
> +		}

[Severity: Critical]
If init_vsie_page() returns a positive validity intercept (such as 1), does
wrapping it in ERR_PTR(rc) return (void *)1?

Since 1 is not >= -MAX_ERRNO, IS_ERR() downstream will evaluate to false,
treating this as a valid pointer.

> +
> +		reset_vsie_page(vcpu->kvm, vsie_page);
> +	}
> +
> +	return vsie_page;
> +}
[ ... ]
> +static int _shadow_sca_cpu(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
> +			   struct vsie_sca *vsie_sca, hpa_t sca_o_entry_hpa,
> +			   unsigned int cpu_nr, bool is_esca)
> +{
[ ... ]
> +	vsie_page_n = get_vsie_page_cpu_nr(vcpu, vsie_sca, scb_o_gpa, cpu_nr);
> +	if (!vsie_page_n)
> +		return -EAGAIN;
> +	if (IS_ERR(vsie_page_n))
> +		return PTR_ERR(vsie_page_n);
> +
> +	rc = shadow_scb(vcpu, vsie_page_n);

[Severity: Critical]
Because IS_ERR(vsie_page_n) evaluates to false for (void *)1, does this allow
the invalid pointer 0x1 to be dereferenced in shadow_scb(), leading to a host
kernel panic?

[ ... ]
> @@ -1838,33 +2204,71 @@ int kvm_s390_handle_vsie(struct kvm_vcpu *vcpu)
[ ... ]
> +	} else {
> +		/* get the vsie_sca with pinned original sca */
> +		vsie_sca = get_vsie_sca(vcpu, scb_o);
> +		if (IS_ERR(vsie_sca)) {
> +			rc = PTR_ERR(vsie_sca);
> +			goto out_unpin;
> +		}

[Severity: High]
If get_vsie_sca() returned ERR_PTR(-EINVAL) due to validate_scao() failing
above, rc becomes -EINVAL here.

Does returning -EINVAL abort the KVM_RUN ioctl entirely, abruptly crashing
the VM instead of gracefully reflecting an error to the guest?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-vsie-sigpi-v5-0-cf471edb46b2@linux.ibm.com?part=16