Re: [PATCH v4 08/20] KVM: s390: vsie: Create helpers to alloc and free vsie_pages

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

Pre-existing issues:
- [High] When KVM recycles a `vsie_page` for a nested guest, it only zeros the `scb_s` field, leaving the `crycb` struct retaining data from previous executions. If an L1 guest runs an L2 guest with a format 2 CRYCB, `setup_apcb11()` populates the full `apcb1` array. If the page is subsequently recycled for an L2 guest with a format 0 or 1 CRYCB, `setup_apcb10()` only overwrites the first elements (`apm[0]`, `aqm[0]`, `adm[0]`). Since KVM unconditionally sets `CRYCB_FORMAT2` for the shadow SCB, the hardware reads the full `apcb1` array, unintentionally granting the second nested guest AP (crypto) permissions from the first guest.
--

--- Patch [8]: [PATCH v4 08/20] KVM: s390: vsie: Create helpers to alloc and free vsie_pages ---
commit 14607902bebe42835f13bc9f13b042b6aeeaca0f
Author: Christoph Schlameuss <[email protected]>

KVM: s390: vsie: Create helpers to alloc and free vsie_pages

This commit encapsulates the allocation and freeing of vsie_page
structures into their own helper methods. This refactoring enables these
operations to be called from multiple code paths.

> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index c935a375f57ca..805d28f0e7058 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c

[ ... ]

> @@ -1558,9 +1576,9 @@ static struct vsie_page *get_vsie_page(struct kvm *kvm, unsigned long addr)
>  		if (vsie_page->scb_gpa != ULONG_MAX)
>  			radix_tree_delete(&kvm->arch.vsie.addr_to_page,
>  					  vsie_page->scb_gpa >> 9);
> +		/* Mark it as invalid until it resides in the tree. */
> +		vsie_page->scb_gpa = ULONG_MAX;
>  	}

[Severity: High]
This is a pre-existing issue, but when a vsie_page is recycled here in
get_vsie_page(), only the scb_s field is zeroed later in the function:

arch/s390/kvm/vsie.c:get_vsie_page() {
    ...
    memset(&vsie_page->scb_s, 0, sizeof(struct kvm_s390_sie_block));
    ...
}

The crycb struct at offset 0x06f8 is left uninitialized and retains data
from previous guests.

If an L1 guest runs an L2 guest with CRYCB_FORMAT2, setup_apcb11()
populates the full apcb1 array. If the page is subsequently recycled by
get_vsie_page() for an L2 guest with CRYCB_FORMAT1, setup_apcb10() only
overwrites the first elements:

arch/s390/kvm/vsie.c:setup_apcb10() {
    ...
    apcb_s->apm[0] = apcb_h->apm[0] & tmp.apm[0];
    apcb_s->aqm[0] = apcb_h->aqm[0] & tmp.aqm[0] & 0xffff000000000000UL;
    apcb_s->adm[0] = apcb_h->adm[0] & tmp.adm[0] & 0xffff000000000000UL;
    ...
}

Because shadow_crycb() sets CRYCB_FORMAT2 unconditionally in the shadow SCB:

arch/s390/kvm/vsie.c:shadow_crycb() {
    ...
    scb_s->crycbd = (u32)virt_to_phys(&vsie_page->crycb) | CRYCB_FORMAT2;
    ...
}

Will the hardware read the full apcb1 array, unintentionally granting the
second nested guest AP (crypto) permissions from the first guest?

> -	/* Mark it as invalid until it resides in the tree. */
> -	vsie_page->scb_gpa = ULONG_MAX;
>  
>  	/* Double use of the same address or allocation failure. */
>  	if (radix_tree_insert(&kvm->arch.vsie.addr_to_page, addr >> 9, vsie_page)) {

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