Re: [PATCH v4 19/20] KVM: s390: vsie: Create minimal scb shadows for not running g3 blocks

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

New issues:
- [High] Silent failure / Lost Validity Intercept on SCA Shadowing
- [High] Cross-VM Data Leak / Stale TLB due to missing ihcpu initialization
- [Medium] L2 Guest State Leak due to Uninitialized Shadow SCB

Pre-existing issues:
- [High] Guest State Corruption in unshadow_scb when prepare_cpuflags fails
--

--- Patch [19]: [PATCH v4 19/20] KVM: s390: vsie: Create minimal scb shadows for not running g3 blocks ---
commit 5e715443b75203daf2ad2801d214c07f27b51bf4
Author: Christoph Schlameuss <[email protected]>
Subject: KVM: s390: vsie: Create minimal scb shadows for not running g3 blocks

Instead of creating a full shadow for vSIE control blocks, this commit creates
a minimal shadow for SIGP purposes for currently not running guest-3 control
blocks. This reduces the overhead of shadowing the entire guest-3 configuration.
The shadow will be fully recreated later when a vSIE is entered with that SCB.

> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index a1b679b4aa361..3a7c58dddf22d 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -709,6 +708,39 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		scb_o->ihcpu = scb_s->ihcpu;
>  }
>  
> +static int shadow_scb_minimal(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> +{
> +	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
> +	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
> +	int rc;
> +
> +	/* make sure we don't have any leftovers when reusing the scb */
> +	scb_s->icptcode = 0;
> +	scb_s->eca = 0;
> +	scb_s->ecb = 0;
> +	scb_s->ecb2 = 0;
> +	scb_s->ecb3 = 0;
> +	scb_s->ecd = 0;
> +	scb_s->fac = 0;
> +	scb_s->fpf = 0;

[Severity: Medium]
Does this limited clearing leave stale L2 execution state in the shadow SCB?

When get_vsie_page_cpu_nr() recycles a vsie_page, it doesn't clear the shadow
SCB with memset().

If shadow_scb_minimal() only clears these specific fields, could uninitialized
data from a previous nested guest (such as ipa and ipb) leak back to the L1
hypervisor's memory when unshadow_scb() is called during an intercept?


[Severity: High]
Does this missing initialization also cause a stale TLB issue for the recycled
SCB?

Because get_vsie_page_cpu_nr() fails to set vsie_page->scb_s.ihcpu = 0xffffU,
and shadow_scb_minimal() doesn't clear it either, a recycled SCB could retain a
stale ihcpu value.

If the L1 guest provides an scb_o->ihcpu that matches the host CPU, could
hardware skip the TLB flush for the shadow SCB, leading to cross-VM memory
access?

[ ... ]

> @@ -728,17 +760,7 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	unsigned long new_mso = 0;
>  	int rc;
>  
> -	/* make sure we don't have any leftovers when reusing the scb */
> -	scb_s->icptcode = 0;
> -	scb_s->eca = 0;
> -	scb_s->ecb = 0;
> -	scb_s->ecb2 = 0;
> -	scb_s->ecb3 = 0;
> -	scb_s->ecd = 0;
> -	scb_s->fac = 0;
> -	scb_s->fpf = 0;
> -
> -	rc = prepare_cpuflags(vcpu, vsie_page);
> +	rc = shadow_scb_minimal(vcpu, vsie_page);
>  	if (rc)
>  		goto out;

[Severity: High]
This is a pre-existing issue, but does unshadow_scb() corrupt the L2 guest
state if prepare_cpuflags() fails?

In shadow_scb(), prepare_cpuflags() is evaluated (via shadow_scb_minimal)
before the L2 guest state is copied from scb_o to scb_s:

shadow_scb() {
    rc = shadow_scb_minimal(vcpu, vsie_page);
    if (rc)
        goto out;
    
    // guest state is copied to scb_s here
    ...
out:
    if (rc)
        unshadow_scb(vcpu, vsie_page);
}

If prepare_cpuflags() fails, shadow_scb() jumps to out and calls
unshadow_scb(), which unconditionally overwrites the L2 guest state in scb_o
with the data from scb_s.

Since scb_s hasn't been populated with the guest state yet, does this corrupt
the L2 guest's state variables (CPU timer, clock comparator, PSW, etc.) in the
L1 memory?

[ ... ]

> @@ -2164,7 +2184,7 @@ static int _shadow_sca(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
>  				rc = PTR_ERR(vsie_page_n);
>  			if (rc)
>  				goto err;
> -			rc = shadow_scb(vcpu, vsie_page_n);
> +			rc = shadow_scb_minimal(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)

[Severity: High]
Can this error path bypass the validity intercept delivery?

In _shadow_sca(), if shadow_scb_minimal() fails for a secondary vCPU, the
function propagates the error without unshadowing:

_shadow_sca() {
    ...
            rc = shadow_scb_minimal(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;
    ...
}

This error is returned to kvm_s390_handle_vsie(), which then jumps to out_put:

kvm_s390_handle_vsie() {
    ...
        rc = shadow_sca(vcpu, vsie_page, vsie_sca);
        if (rc)
            goto out_put;
    ...
out_put:
    put_vsie_page(vsie_page);
    ...
}

Because this bypasses out_unshadow for the primary vCPU, does this silently
drop the ICPT_VALIDITY intercept code, causing the L1 hypervisor's SIE
instruction to appear as if it completed successfully when it actually failed?

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