Re: [PATCH 17/21] KVM: s390: vsie: Allow guest-3 cpu add and remove with vsie sigpif

[sashiko-bot@kernel.org]

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

New issues:
- [High] If `_shadow_sca()` encounters a transient error, it prematurely updates the cached MCN but zeroes out the shadow SCA, permanently breaking VSIE SIGP handling.
- [High] Dynamic MCN updates fail to clear shadow SCA entries for removed vCPUs, leaving stale pointers that the hardware may access.
- [High] Mismatch between cached `vsie_sca->flags` and actual guest SCA format leads to copying garbage MCN data from wrong offsets.
- [Low] Incorrect pointer type `struct kvm_s390_sie_block *` is used for the guest SCA page.

Pre-existing issues:
- [Critical] get_sca_entry_addr() incorrectly calculates the host physical address (HPA) of SCA entries located past the first page, leading to arbitrary host physical memory access.
--

--- Patch [17]: [PATCH 17/21] KVM: s390: vsie: Allow guest-3 cpu add and remove with vsie sigpif ---
commit c4d503d35381889298738f17763bcbfdd845d493
Author: Christoph Schlameuss <schlameuss@linux.ibm.com>

KVM: s390: vsie: Allow guest-3 cpu add and remove with vsie sigpif

This commit enables dynamically adding and removing guest-3 CPUs in KVM
by shadowing the SCA and re-initializing the shadow SCA whenever the guest's
multi-cpu configuration (MCN) changes. It introduces caching and comparison
of the expected MCN to detect when the layout changes.

> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 74455c689c51b..de8931e714ca2 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
[ ... ]
> +static void sca_mcn_copy(struct vsie_sca *vsie_sca, void *sca)
> +{
> +	int offset = offsetof(struct bsca_block, mcn);
> +	int size = BYTES_PER_LONG;
> +
> +	if (test_bit(VSIE_SCA_ESCA, &vsie_sca->flags)) {

[Severity: High]
If a guest switches a control block from BSCA to ESCA (or vice versa) while
keeping the same physical sca_gpa, will the cached VSIE_SCA_ESCA flag in
vsie_sca->flags be stale?

Later in _shadow_sca(), the layout is determined dynamically using
sie_uses_esca(vsie_page->scb_o). If these disagree, could sca_mcn_copy() read
garbage memory from the wrong MCN offset?

> +		offset = offsetof(struct esca_block, mcn);
> +		size = BYTES_PER_LONG * 4;
> +	}
> +	memcpy(&vsie_sca->mcn, sca + offset, size);
> +}
[ ... ]
>  static int _shadow_sca(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
>  		       struct vsie_sca *vsie_sca)
>  {
[ ... ]
> -	if (is_esca)
> -		mcn = phys_to_virt(sca_o_hpa) + offsetof(struct esca_block, mcn);
> -	else
> -		mcn = phys_to_virt(sca_o_hpa) + offsetof(struct bsca_block, mcn);
> +	sca_mcn_copy(vsie_sca, phys_to_virt(sca_o_hpa));

[Severity: High]
If _shadow_sca() encounters an error later in this function, could this early
copy of the MCN cause permanent failures?

If _shadow_sca() returns an error, vsie_sca->mcn will already be updated to
the new value, while the error path zeroes out the shadow SCA but leaves
vsie_sca->ssca.osca unmodified.

On the next VSIE entry retry, shadow_sca() will see that sca_mcn_equals() is
true, and skip re-initializing the shadow SCA. Does this leave the guest
with a broken SIGP configuration permanently?

>  
>  	/* pin and make shadow for ALL scb in the sca */
>  	cpu_slots = is_esca ? KVM_S390_MAX_VSIE_VCPUS : KVM_S390_BSCA_CPU_SLOTS;
> -	for_each_set_bit_inv(cpu_nr, mcn, cpu_slots) {
> +	for_each_set_bit_inv(cpu_nr, (unsigned long *)&vsie_sca->mcn, cpu_slots) {

[Severity: High]
Does this loop exclusively iterate over the bits set in the new MCN?

If a vCPU was removed by the guest (its MCN bit transitions from 1 to 0), the
loop skips that CPU index entirely. Does this mean the ssda and ossea
pointers for the removed CPU are never cleared, leaving stale pointers that
the hardware might still access?

>  		rc = get_sca_entry_addr(vcpu->kvm, vsie_sca, cpu_nr, NULL, &sca_o_entry_hpa);

[Severity: Critical]
This is a pre-existing issue, but looking at get_sca_entry_addr() called
here, is there a problem with how it calculates the host physical address
for SCA entries past the first page?

arch/s390/kvm/vsie.c:get_sca_entry_addr() {
    ...
    if (hpa)
        *hpa = vsie_sca->sca_o_pages[pn].hpa + offset;
    ...
}

Because offset is the total byte offset from the start of the SCA block, and
sca_o_pages[pn].hpa is already the physical address of the specific pn-th
page, does adding offset again push the resulting HPA massively out of
bounds?

>  		if (rc)
>  			goto err;
[ ... ]
>  /* Shadow or reshadow the SCA on VSIE enter. */
>  static int shadow_sca(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page, struct vsie_sca *vsie_sca)
>  {
> -	int rc = 0;
> +	struct kvm_s390_sie_block *sca_o = phys_to_virt(vsie_sca->sca_o_pages[0].hpa);

[Severity: Low]
This isn't a bug, but sca_o is typed as a State Control Block
(struct kvm_s390_sie_block *) when the memory appears to actually be a System
Control Area (struct bsca_block or struct esca_block). Could this static
typing make the code confusing to read?

> +	bool do_init_ssca;
>  
> -	guard(rwsem_write)(&vcpu->kvm->arch.vsie.vsie_sca_lock);
> -	if (!vsie_sca->ssca.osca)
> -		rc = _shadow_sca(vcpu, vsie_page, vsie_sca);
> +	scoped_guard(rwsem_read, &vcpu->kvm->arch.vsie.vsie_sca_lock) {
> +		do_init_ssca = !vsie_sca->ssca.osca;
> +		do_init_ssca = do_init_ssca || !sca_mcn_equals(vsie_sca, sca_o);
> +		if (!do_init_ssca)
> +			return 0;
> +	}

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