Re: [PATCH v6 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] Unhandled ERR_PTR from get_vsie_sca_unused() leads to host kernel panic.
- [Critical] XArray collision due to SCA alignment shift causes shadow SCA state mixups.
- [High] KVM permanently caches the SCA address, ignoring L1 guest updates when SSCA is not used.
- [Medium] Error path in `get_vsie_sca` leaks memory pins and the `vsie_sca` refcount.
--

commit 6a2015f888275e12b50521a0390805284a9592cd
Author: Christoph Schlameuss <schlameuss@linux.ibm.com>

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

Restructure kvm_s390_handle_vsie() to create a guest-1 shadow of the SCA
if guest-2 attempts to enter SIE with an SCA. If the SCA is used the
vsie_pages are stored in a new vsie_sca struct instead of the arch vsie
struct.

> diff --git a/arch/s390/kvm/s390/vsie.c b/arch/s390/kvm/s390/vsie.c
> index 69334d4a32311..16273cf5cbff5 100644
> --- a/arch/s390/kvm/s390/vsie.c
> +++ b/arch/s390/kvm/s390/vsie.c

[ ... ]

> @@ -1031,6 +1031,16 @@ static struct vsie_sca *get_vsie_sca_existing(struct kvm *kvm, gpa_t sca_o_gpa, bool uses_esca)
>  {
>  	struct vsie_sca *vsie_sca = xa_load(&kvm->arch.vsie.osca_to_sca,
>  					    sca_o_gpa >> SCA_ALIGNMENT_SHIFT);
>  
>  	if (!vsie_sca)
>  		return NULL;
>  	if (uses_esca != test_bit(VSIE_SCA_ESCA, &vsie_sca->flags))
>  		return NULL;
>  	WARN_ON_ONCE(atomic_inc_return(&vsie_sca->ref_count) < 1);
>  	return vsie_sca;
>  }

[Severity: Critical]
Does this code allow an XArray collision if multiple SCAs share the same
64-byte block?

If a guest allocates two different SCAs (which is valid since BSCA only
requires 16-byte alignment), they will result in the same XArray index due
to SCA_ALIGNMENT_SHIFT. The returned shadow SCA is not verified to ensure
vsie_sca->sca_gpa == sca_o_gpa, which could cause L2 VCPUs to operate on the
wrong SCA context.

[ ... ]

> @@ -1124,6 +1124,10 @@ static int get_vsie_sca(struct kvm_vcpu *vcpu, struct kvm_s390_sie_block *scb_o,
>  	} 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;

[Severity: Critical]
If get_vsie_sca_unused() returns ERR_PTR(-EAGAIN) when the system exhausts
the maximum number of concurrent active shadow SCAs, doesn't this jump to the
out label without setting an error code in rc?

This would leak the ERR_PTR to the caller because rc remains 0.

[ ... ]

> @@ -1164,13 +1164,13 @@ static int get_vsie_sca(struct kvm_vcpu *vcpu, struct kvm_s390_sie_block *scb_o,
>  	rc = xa_insert(&kvm->arch.vsie.osca_to_sca, vsie_sca->sca_gpa >> SCA_ALIGNMENT_SHIFT,
>  		       vsie_sca, GFP_KERNEL_ACCOUNT);
>  	if (rc == -EBUSY)
>  		rc = 1;
>  
>  out:
>  	up_write(&kvm->arch.vsie.vsie_sca_lock);
>  	if (vsie_sca_new)
>  		free_vsie_sca(kvm, vsie_sca_new);
>  	if (vsie_sca)
>  		*vsie_sca_out = vsie_sca;
>  	return rc;
>  }

[Severity: Medium]
If xa_insert() fails (e.g., returning -ENOMEM), this error path returns the
error code but still populates *vsie_sca_out with the valid vsie_sca pointer.

Because the caller kvm_s390_handle_vsie() jumps to out_unpin on error and
skips the out_put_sca label, won't this leak the memory pinned by pin_sca()
and the vsie_sca refcount?

[ ... ]

> @@ -1245,13 +1245,12 @@ static void unpin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>  	hpa_t hpa;
>  
> -	hpa = (u64) scb_s->scaoh << 32 | scb_s->scaol;
> -	if (hpa) {
> -		unpin_guest_page(vcpu->kvm, vsie_page->sca_gpa, hpa);
> -		vsie_page->sca_gpa = 0;
> -		write_scao(scb_s, 0);
> -	}
> +	if (!vsie_page->vsie_sca) {
> +		hpa = (u64) scb_s->scaoh << 32 | scb_s->scaol;
> +		if (hpa) {
> +			unpin_guest_page(vcpu->kvm, vsie_page->sca_gpa, hpa);
> +			write_scao(scb_s, 0);
> +		}
>  	}

[ ... ]

> @@ -1300,8 +1300,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_s, gpa);
>  		if (rc)
>  			goto unpin;
> @@ -1310,7 +1310,6 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  			rc = set_validity_icpt(scb_s, 0x0034U);
>  			goto unpin;
>  		}
> -		vsie_page->sca_gpa = gpa;
>  		write_scao(scb_s, hpa);
>  	}

[Severity: High]
Does removing vsie_page->sca_gpa = 0; break the guest's ability to dynamically
update the SCA?

Because pin_blocks() now uses the one-time cached vsie_page->sca_gpa instead
of evaluating the active L1 guest SCB via read_scao(), KVM might silently
overwrite the scaol field with the old cached physical address when the L1
guest updates the SCB.

[ ... ]

> @@ -2043,15 +2043,18 @@ static int 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_out)
>  {
>  	struct vsie_page *vsie_page, *vsie_page_new = NULL;
>  	int rc;
>  
>  	vsie_page = vsie_sca->pages[cpu_nr];

[Severity: Critical]
If get_vsie_sca() successfully returned an ERR_PTR (due to the missing rc
update discussed earlier), won't this dereference the unhandled ERR_PTR
and cause a host kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-vsie-sigpi-v6-0-8020bb53be52@linux.ibm.com?part=16