Re: [PATCH v5 13/21] KVM: s390: vsie: Lazily keep original scb pinned after vsie exit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Removing the old SCB address from the XArray when recycling a vsie_page causes an unbounded host memory leak.
- [High] Unconditional `xa_store()` silently overwrites concurrent entries, bypassing KVM's protection against concurrent nested SCB execution.
- [High] Misrouting of addressing exceptions to the 'double use' path causes improper guest PSW rewinding.
--

commit 5c64511a5db61f1b968b748560aa62a1a39b08a9
Author: Christoph Schlameuss <schlameuss@linux.ibm.com>

KVM: s390: vsie: Lazily keep original scb pinned after vsie exit

Keep the original SIE control block (SCB) pinned and only lazily unpin
it on reuse of the vsie_page for a different SCB. Track pinned pages and
ensure stale pins are released when needed.

> diff --git a/arch/s390/kvm/s390/vsie.c b/arch/s390/kvm/s390/vsie.c
> index 988370f491bf5..24af48af89cd9 100644
> --- a/arch/s390/kvm/s390/vsie.c
> +++ b/arch/s390/kvm/s390/vsie.c
[ ... ]
> @@ -1575,47 +1606,65 @@ static struct vsie_page *get_vsie_page(struct kvm *kvm, unsigned long addr)
[ ... ]
>  	} else {
>  		/* reuse an existing entry that belongs to nobody */
>  		while (true) {
> -			vsie_page = kvm->arch.vsie.pages[kvm->arch.vsie.next];
> +			pages_idx = kvm->arch.vsie.next;
> +			kvm->arch.vsie.next++;
> +			kvm->arch.vsie.next %= kvm->arch.vsie.page_count;
> +			vsie_page = kvm->arch.vsie.pages[pages_idx];
>  			if (try_get_vsie_page(vsie_page))
>  				break;
> -			kvm->arch.vsie.next++;
> -			kvm->arch.vsie.next %= nr_vcpus;
>  		}
> -		if (vsie_page->scb_gpa != ULONG_MAX)
> -			xa_erase(&kvm->arch.vsie.addr_to_page,
> -				 vsie_page->scb_gpa >> SCB_ALIGNMENT_SHIFT);
> -		/* Mark it as invalid until it resides in the tree. */
> -		vsie_page->scb_gpa = ULONG_MAX;
> +
> +		unpin_scb(kvm, vsie_page);
>  	}

[Severity: Critical]
Why is xa_erase() removed here? Leaving the old guest SCB address mapping in
the XArray when recycling a vsie_page allows a malicious guest to repeatedly
execute vSIE with unique SCB addresses, continuously consuming host kernel
memory by allocating new XArray entries. Could this lead to an unbounded
host memory leak?

>  
> -	/* Double use of the same address or allocation failure. */
> -	if (xa_insert(&kvm->arch.vsie.addr_to_page, addr >> SCB_ALIGNMENT_SHIFT, vsie_page,
> -		      GFP_KERNEL_ACCOUNT)) {
> +	rc = init_vsie_page(vcpu, vsie_page, addr);
> +	if (!rc)
> +		xa_store(&kvm->arch.vsie.addr_to_page, addr >> SCB_ALIGNMENT_SHIFT, vsie_page,
> +			 GFP_KERNEL_ACCOUNT);

[Severity: High]
Does xa_store() here bypass the concurrent double-use protection?

The baseline code used xa_insert() which gracefully returns an error if an
entry already exists, preventing two vCPUs from executing the same nested SCB
concurrently.

By replacing this with an unconditional xa_store(), the second vCPU will
silently overwrite the XArray entry. Both vCPUs will enter nested SIE using
different shadow structures for the same physical guest SCB, and when they exit,
both will concurrently write back to scb_o in unshadow_scb().

Could this cause severe nested guest state corruption?

> +
> +	mutex_unlock(&kvm->arch.vsie.mutex);
> +	if (vsie_page_new)
> +		free_vsie_page(vsie_page_new);
> +	if (rc) {
> +		vsie_page->scb_gpa = ULONG_MAX;
>  		put_vsie_page(vsie_page);
> -		mutex_unlock(&kvm->arch.vsie.mutex);
> -		return NULL;
> +		return rc < 0 ? ERR_PTR(rc) : NULL;
>  	}

[Severity: High]
If a guest provides an invalid SCB address, pin_scb() injects a PGM_ADDRESSING
program exception and returns 1. Here, rc < 0 ? ERR_PTR(rc) : NULL will convert
this 1 into a NULL pointer return.

The caller kvm_s390_handle_vsie() interprets a NULL return strictly as a 'double
use' scenario and executes kvm_s390_rewind_psw(vcpu, 4). Doesn't this
incorrectly rewind the guest PSW after a program exception has already been
injected?

Furthermore, if follow-up patches in the series return ERR_PTR(rc) directly
(evaluating to ERR_PTR(1)), an IS_ERR check in the caller will fail to catch
it because 1 is a valid pointer address, leading to a host kernel panic when
dereferenced.

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