Re: [PATCH v4 16/20] KVM: s390: vsie: Allow guest-3 cpu add and remove with ssca

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The main CPU's `vsie_page` is freed twice during `kvm_s390_vsie_destroy()`, leading to a fatal host double-free vulnerability.
- [High] Removed CPUs are not cleared from the shadow system control area (SSCA) during configuration changes, leaving stale active entries in hardware.
- [High] Failure to clear the VSIE_SCA_ESCA flag when transitioning from ESCA to BSCA causes memory corruption and infinite re-shadow loops.
- [High] Dynamically changing the SCA format from BSCA to ESCA fails to repin memory, triggering a guest-controllable host WARN_ON_ONCE.
--

--- Patch [16]: [PATCH v4 16/20] KVM: s390: vsie: Allow guest-3 cpu add and remove with ssca ---
commit 73a8d58ca743bc97299afb560aeb506eff16cf44
Author: Christoph Schlameuss <[email protected]>

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

As we are shadowing the SCA we need to add and remove the pointers to
the shadowed control blocks and sca entries whenever the mcn /
processors in the configuration changes.

It is not expected that the mcn changes frequently for an already
running guest-3 configuration. So we can simply fully re-init the ssca
whenever the mcn changes.

> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c

[ ... ]

> @@ -2071,18 +2108,16 @@ static int _shadow_sca(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
>  	struct vsie_page *vsie_page_n;
>  	hpa_t sca_o_entry_hpa;
>  	hva_t sca_o_entry_hva;
> -	unsigned long *mcn;
>  	gpa_t scb_o_gpa;
>  	int rc;
>  
>  	if (is_esca)
> -		mcn = phys_to_virt(sca_o_hpa(vsie_sca)) + offsetof(struct esca_block, mcn);
> -	else
> -		mcn = phys_to_virt(sca_o_hpa(vsie_sca)) + offsetof(struct bsca_block, mcn);
> +		__set_bit(VSIE_SCA_ESCA, &vsie_sca->flags);

[Severity: High]
If the guest switches its SCA format from ESCA to BSCA, is_esca will
be false, but does anything clear the VSIE_SCA_ESCA flag?

If the flag remains set, it seems subsequent functions like
sca_mcn_copy() and get_sca_entry_addr() will incorrectly parse the BSCA
using ESCA offsets. Could this lead to memory corruption or infinite
re-shadowing loops since config_changed() would always return true?

[Severity: High]
When dynamically switching from BSCA to ESCA format, does the system
need to repin memory for the additional SCA pages?

If a guest starts with a BSCA (which pins 1-2 pages) and dynamically
switches to ESCA while keeping the same SCA address, the VSIE_SCA_ESCA
flag is set here but the additional pages (up to 5) are not pinned.

Could this cause get_sca_entry_addr() to calculate a page index that
exceeds sca_o_nr_pages:

get_sca_entry_addr() {
    ...
    if (WARN_ON_ONCE(pn >= vsie_sca->sca_o_nr_pages))
        return -EINVAL;
    ...
}

tripping the guest-controllable host WARN_ON_ONCE at the bounds check?

> +	sca_mcn_copy(vsie_sca, phys_to_virt(sca_o_hpa(vsie_sca)));
>  
>  	/* 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 leave stale active entries in the shadow system control
area (SSCA) when CPUs are removed?

If an unprivileged guest modifies the MCN mask to remove CPUs, this loop
only iterates over the currently set bits in the new MCN mask.

Since the SSCA entries for the removed CPUs are not cleared, won't
hardware continue delivering SIGPIs to these stale entries because it
uses a non-zero ssda to identify active CPUs?

[Severity: Critical]
Does storing the main CPU's vsie_page during this loop lead to a double-free
during VM destruction?

When vsie_page->scb_o->icpua == cpu_nr, vsie_sca_update() is called with
the vsie_page. This vsie_page is already tracked in kvm->arch.vsie.pages[],
and vsie_sca_update() also adds it to vsie_sca->pages[cpu_nr].

When the VM is destroyed, kvm_s390_vsie_destroy() frees all pages in
kvm->arch.vsie.pages[], and then iterates over vsie_sca->pages[], freeing
those pages too. Will this blindly free the same main CPU's page twice?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16