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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] Incorrect pointer arithmetic in `get_sca_entry_addr()` computes wildly out-of-bounds host and guest physical addresses.
- [Critical] Incomplete error rollback in `get_vsie_page_cpu_nr()` leads to host NULL pointer dereference and XArray corruption.
- [Critical] Guest-controlled `icpua` field from the SIE control block is used directly as an array index without bounds checking, leading to arbitrary OOB reads and writes.
- [Critical] Concurrent `memset` of a shared `vsie_page` during SCA reuse causes data corruption and potential Host DoS.
- [Critical] Shadow SCBs are left unprotected during guest execution because `VSIE_PAGE_IN_USE` is prematurely cleared.
- [High] Unvalidated guest-provided `sda` in the SCA allows out-of-bounds reads of host physical memory.
- [High] Failure in `pin_sca()` permanently leaks the `vsie_sca` reference, allowing a malicious guest to exhaust the VSIE SCA pool.
- [High] Stale `vsie_sca` pointers in cached `vsie_page`s break SCB execution when a guest toggles SIGPIF off, and enable Use-After-Free.
- [High] KVM caches the guest's SCA address in `vsie_page->sca_gpa` and ignores dynamic updates to the SCB's SCAO field.
- [High] Mismatched locking causes a data race on the `vsie_sca->pages[]` array.
- [Medium] Multiple guest-triggerable `WARN_ON_ONCE` assertions enable host syslog flooding and Denial of Service.
- [Medium] XArray internal nodes and elements are permanently leaked on VM destruction and SCA reuse due to missing `xa_destroy()` and incomplete `xa_erase()` cleanup.
--

--- Patch [16]: [PATCH 16/21] KVM: s390: vsie: Shadow VSIE SCA in guest-1 ---
commit 9e60c38b165cf3838037fe34e3e571d246e77218
Author: Christoph Schlameuss <schlameuss@linux.ibm.com>

KVM: s390: vsie: Shadow VSIE SCA in guest-1

This commit restructures the vsie handler to create a guest-1 shadow of the
SCA if guest-2 attempts to enter SIE with an SCA. It adds the vsie_sca
struct and manages its lifecycle and lookup via a map in the arch vsie
struct.

> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 4b492af155429..74455c689c51b 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
[ ... ]
> @@ -694,6 +694,22 @@ static int shadow_scb_minimal(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
[ ... ]
>  	scb_s->ecb2 |= scb_o->ecb2 & ECB2_ESCA;
>  
>  	if (vsie_page->vsie_sca) {
>  		scb_s->eca |= scb_o->eca & ECA_SIGPI;
>  		scb_s->ecb |= scb_o->ecb & ECB_SRSI;
>  		write_scao(scb_s, virt_to_phys(&vsie_page->vsie_sca->ssca));
>  		scb_s->osda = virt_to_phys(scb_o);
>  	}

[Severity: High]
Does this code use a stale vsie_sca pointer if the guest executes the same
SCB with SIGPIF disabled?

When get_vsie_page() retrieves a cached vsie_page, it does not clear
vsie_page->vsie_sca. If the guest previously executed this SCB with SIGPIF
enabled, but now executes it with SIGPIF disabled, will shadow_scb_minimal()
forcefully inject SIGPIF configuration using a stale pointer? Since vsie_sca
can be recycled, could this cause cross-guest isolation breaches?

[ ... ]
> @@ -927,6 +927,21 @@ static int pin_sca(struct kvm *kvm, struct vsie_sca *vsie_sca)
[ ... ]
>  	vsie_sca->sca_o_nr_pages = pin_guest_pages(kvm, vsie_sca->sca_gpa, nr_pages,
>  						   vsie_sca->sca_o_pages);
>  	if (WARN_ON_ONCE(vsie_sca->sca_o_nr_pages != nr_pages))
>  		return -EIO;

[Severity: Medium]
Is this WARN_ON_ONCE triggerable by a malicious guest?

If the guest provides an unmapped sca_gpa, pin_guest_pages() will return
-EFAULT. This causes an underflow when compared to nr_pages, triggering the
warning. Should guest-driven errors return directly without asserting to
prevent syslog flooding?

[ ... ]
> +static int get_sca_entry_addr(struct kvm *kvm, struct vsie_sca *vsie_sca, u16 cpu_nr, gpa_t *gpa,
> +			      hpa_t *hpa)
> +{
> +	hpa_t offset;
> +	int pn;
> +
> +	/*
> +	 * We cannot simply access the hva since the esca_block has typically
> +	 * 4 pages (arch max 5 pages) that might not be continuous in g1 memory.
> +	 * The bsca_block may also be stretched over two pages. Only the header
> +	 * is guaranteed to be on the same page.
> +	 */
> +	if (test_bit(VSIE_SCA_ESCA, &vsie_sca->flags))
> +		offset = offsetof(struct esca_block, cpu[cpu_nr]);
> +	else
> +		offset = offsetof(struct bsca_block, cpu[cpu_nr]);
> +	pn = ((vsie_sca->sca_gpa & ~PAGE_MASK) + offset) >> PAGE_SHIFT;
> +	if (WARN_ON_ONCE(pn >= vsie_sca->sca_o_nr_pages))
> +		return -EINVAL;
> +
> +	if (gpa)
> +		*gpa = vsie_sca->sca_o_pages[pn].gpa + offset;
> +	if (hpa)
> +		*hpa = vsie_sca->sca_o_pages[pn].hpa + offset;
> +	return 0;
> +}

[Severity: Critical]
Does this address arithmetic overshoot the pinned page boundaries?

The offset calculated here is the full structural offset from the start of
the SCA (which can be up to 5 * PAGE_SIZE). When calculating the hpa, this
full offset is added to the base physical address of the pn-th page, rather
than just the offset within that specific page. Can this instruct the
hardware to access unpinned, arbitrary host physical memory?

[ ... ]
> +static struct vsie_sca *get_vsie_sca(struct kvm_vcpu *vcpu, struct kvm_s390_sie_block *scb_o,
> +				     gpa_t sca_gpa)
> +{
[ ... ]
> +	} else {
> +		/* reuse previously created vsie_sca allocation for different osca */
> +		vsie_sca = get_reuseable_vsie_sca(kvm);
> +		/* with nr_vcpus scas one must be reusable */
> +		if (IS_ERR(vsie_sca))
> +			goto out;
> +		WARN_ON_ONCE(atomic_read(&vsie_sca->ref_count) != 1);
> +
> +		xa_erase(&kvm->arch.vsie.osca_to_sca, vsie_sca->sca_gpa >> 6);
> +		for (cpu_nr = 0; cpu_nr < KVM_S390_MAX_VSIE_VCPUS; cpu_nr++) {
> +			vsie_page_n = vsie_sca->pages[cpu_nr];
> +			if (!vsie_page_n)
> +				continue;
> +
> +			/* unpin but keep the vsie_page for reuse */
> +			unpin_scb(kvm, vsie_page_n);
> +			release_gmap_shadow_safe(kvm, vsie_page_n);
> +			memset(vsie_page_n, 0, sizeof(struct vsie_page));

[Severity: Critical]
Is it safe to memset the vsie_page while it is still in the addr_to_page
XArray?

The vsie_page is zeroed out here, clearing its VSIE_PAGE_IN_USE flag, but it
is never removed from the addr_to_page XArray. A concurrent VCPU executing
get_vsie_page() can perform a lockless xa_load(), observe the in-use flag as
0, and claim the page. Could this cause the first thread to clear scb_o
while the second thread actively executes shadow_scb() using the now-NULL
pointer?

> +			vsie_page_n->scb_gpa = ULONG_MAX;

[Severity: Medium]
Does this code leak XArray entries?

Because scb_gpa is set to ULONG_MAX without calling xa_erase(), does this
leave dangling pointers and leak XArray internal nodes?

> +		}
> +		unpin_sca(kvm, vsie_sca);
> +		clear_vsie_sca(vsie_sca);
> +	}
> +
> +	if (sie_uses_esca(scb_o))
> +		__set_bit(VSIE_SCA_ESCA, &vsie_sca->flags);
> +	vsie_sca->sca_gpa = sca_gpa;
> +
> +	/*
> +	 * The pinned original sca will only be unpinned lazily to limit the
> +	 * required amount of pins/unpins on each vsie entry/exit.
> +	 * The unpin is done in the reuse vsie_sca allocation path above and
> +	 * kvm_s390_vsie_destroy().
> +	 */
> +	rc = pin_sca(kvm, vsie_sca);
> +	if (rc) {
> +		vsie_sca = ERR_PTR(rc);
> +		goto out;
> +	}

[Severity: High]
Does this error path leak the vsie_sca reference?

The ref_count is initialized to 1 earlier. If pin_sca() fails (e.g., due to
an unmapped guest sca_gpa), the function returns ERR_PTR(rc) and the caller
bypasses put_vsie_sca(). Since get_reuseable_vsie_sca() expects a ref_count
of 0 for reuse, will these stranded objects permanently exhaust the finite
pool of vsie_sca structures?

[ ... ]
> @@ -1032,8 +1242,8 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	gpa_t gpa;
>  	int rc = 0;
>  
> -	gpa = read_scao(vcpu->kvm, scb_o);
> -	if (gpa) {
> +	gpa = vsie_page->sca_gpa;
> +	if (gpa && !vsie_page->vsie_sca) {
>  		rc = validate_scao(vcpu, scb_o, gpa);

[Severity: High]
Will this ignore dynamic updates to the guest's SCAO field?

By reading the cached vsie_page->sca_gpa instead of dynamically calling
read_scao(), KVM seems to lock the SCA address permanently after it is first
set in vsie_page_init(). If a guest updates the SCAO field for a reused SCB,
will KVM silently continue pinning and shadowing the old address?

[ ... ]
> +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)
> +{
> +	struct vsie_page *vsie_page, *vsie_page_new = NULL;
> +	int rc;
> +
> +	vsie_page = vsie_sca->pages[cpu_nr];

[Severity: Critical]
Is the cpu_nr index validated against the bounds of the pages array?

The cpu_nr parameter comes directly from the guest-controlled icpua field.
Since it is not bounds-checked against KVM_S390_MAX_VSIE_VCPUS, can a
malicious guest provide an out-of-bounds or negative index, causing KVM to
read from and later overwrite arbitrary host memory with a newly allocated
page pointer?

> +	if (!vsie_page) {
> +		vsie_page_new = alloc_vsie_page(vcpu->kvm);
> +		if (!vsie_page_new)
> +			return ERR_PTR(-ENOMEM);
> +		vsie_page_new->vsie_sca = vsie_sca;
> +		__set_bit(VSIE_PAGE_IN_USE, &vsie_page_new->flags);
> +
> +		/* be careful to not loose a page here if we raced */
> +		scoped_guard(mutex, &vsie_sca->mutex) {
> +			vsie_page = vsie_sca->pages[cpu_nr];
> +			if (!vsie_page) {
> +				WRITE_ONCE(vsie_sca->pages[cpu_nr], vsie_page_new);

[Severity: High]
Does this create a data race on the vsie_sca->pages array?

This code writes to the array while holding vsie_sca->mutex. However,
vsie_sca_update() also writes to this array but holds vsie_sca_lock instead
of vsie_sca->mutex. Could this lock mismatch corrupt array pointers?

> +				vsie_page = vsie_page_new;
> +			}
> +		}
> +	}
[ ... ]
> +	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 = vsie_page_init(vcpu, vsie_page, scb_gpa);
> +		}
> +		if (WARN_ON_ONCE(rc)) {
> +			put_vsie_page(vsie_page);
> +			return ERR_PTR(rc);
> +		}
> +	}

[Severity: Critical]
Does this error path leave vsie_page->scb_gpa with a stale value?

If vsie_page_init() fails to pin the SCB, it returns an error but leaves
vsie_page->scb_gpa updated to the unpinnable address. By returning ERR_PTR
here without rolling back scb_gpa to ULONG_MAX, if this page is encountered
again, vsie_page_init() will see the stale value and mistakenly call
xa_erase(). Can this arbitrarily delete a valid entry belonging to another
VCPU's SCB?

Also, since the error is driven by guest-controlled input (an unpinnable
SCB address), does WARN_ON_ONCE(rc) allow a guest to spam the host syslog?

[ ... ]
> +static int _shadow_sca(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
> +		       struct vsie_sca *vsie_sca)
> +{
[ ... ]
> +	for_each_set_bit_inv(cpu_nr, mcn, cpu_slots) {
> +		rc = get_sca_entry_addr(vcpu->kvm, vsie_sca, cpu_nr, NULL, &sca_o_entry_hpa);
> +		if (rc)
> +			goto err;
> +
> +		if (vsie_page->scb_o->icpua == cpu_nr) {
> +			vsie_sca_update(vsie_sca, cpu_nr, vsie_page, sca_o_entry_hpa);
> +		} else {
> +			sca_o_entry_hva = (hva_t)phys_to_virt(sca_o_entry_hpa);
> +			if (is_esca)
> +				scb_o_gpa = ((struct esca_entry *)sca_o_entry_hva)->sda;
> +			else
> +				scb_o_gpa = ((struct bsca_entry *)sca_o_entry_hva)->sda;

[Severity: High]
Are alignment checks missing when reading the guest-provided sda?

The guest-provided sda is read from the SCA and passed to
get_vsie_page_cpu_nr(), which maps a single 4096-byte page. If KVM does not
enforce 512-byte alignment on sda, could a malicious guest provide an address
near the end of the page? This would cause the 512-byte shadow_scb() read to
cross the boundary of the pinned page into adjacent, unpinned host physical
memory.

> +			vsie_page_n = get_vsie_page_cpu_nr(vcpu, vsie_sca, scb_o_gpa, cpu_nr);
> +			if (!vsie_page_n)
> +				rc = -EAGAIN;
> +			if (IS_ERR(vsie_page_n))
> +				rc = PTR_ERR(vsie_page_n);
> +			if (rc)
> +				goto err;
> +			rc = shadow_scb(vcpu, vsie_page_n);
> +			vsie_sca_update(vsie_sca, cpu_nr, vsie_page_n, sca_o_entry_hpa);
> +			put_vsie_page(vsie_page_n);
> +			if (rc)
> +				goto err;
> +		}
> +	}

[Severity: Critical]
Does calling put_vsie_page() prematurely expose the shadow SCB?

put_vsie_page() clears the VSIE_PAGE_IN_USE flag. Because these pages reside
in the global addr_to_page pool, another host VCPU looking for a free page
can observe the in-use flag as 0 and claim it. Can this concurrent VCPU call
vsie_page_init() and overwrite the memory of the shadow SCB while the
hardware is actively using it for nested execution?

[ ... ]
>  void kvm_s390_vsie_destroy(struct kvm *kvm)
>  {
>  	struct vsie_page *vsie_page;
> -	int i;
> +	struct vsie_sca *vsie_sca;
> +	int i, cpu_nr;
> +
> +	guard(mutex)(&kvm->arch.vsie.mutex);
[ ... ]
> +	for (i = 0; i < kvm->arch.vsie.sca_count; i++) {
> +		vsie_sca = kvm->arch.vsie.scas[i];
> +		kvm->arch.vsie.scas[i] = NULL;
> +
> +		for (cpu_nr = 0; cpu_nr < KVM_S390_MAX_VSIE_VCPUS; cpu_nr++) {
> +			vsie_page = vsie_sca->pages[cpu_nr];
> +			vsie_sca->pages[cpu_nr] = NULL;
> +			if (!vsie_page)
> +				continue;
> +			unpin_scb(kvm, vsie_page);
> +			kvm_s390_vsie_destroy_page(kvm, vsie_page);
> +		}
> +
> +		unpin_sca(kvm, vsie_sca);
> +		free_vsie_sca(kvm, vsie_sca);
> +	}
> +	kvm->arch.vsie.sca_count = 0;
>  }

[Severity: Medium]
Does this teardown leak XArray entries?

When freeing vsie_sca objects, there are no calls to xa_erase() to remove
them from the osca_to_sca XArray. Furthermore, xa_destroy() is missing for
both addr_to_page and osca_to_sca. Will this permanently leak dynamically
allocated XArray internal nodes every time a VM is destroyed?

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