Re: [PATCH v5 3/9] KVM: s390: Do not set special large pages dirty

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing special page checks (`.s` flag) in `gmap_unmap` and `destroy_pages` paths lead to a host kernel panic when mapping I/O memory into Secure Execution VMs.
- [Critical] Missing special page checks in storage key and reference bit handlers allow an unprivileged guest to crash the host via addressing exceptions.
- [Critical] In `_kvm_s390_pv_make_secure()`, KVM unconditionally calls `pfn_folio()` on the faulting PFN and attempts to lock it without verifying if the PFN is backed by a valid `struct page`.
--

commit 9b9c8471e051ee12971f206fa564520a1f1fea5b
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>
Subject: KVM: s390: Do not set special large pages dirty

This commit updates gmap_clear_young_crste() to avoid setting special large
pages dirty. It adds a check for the special bit before calling
folio_set_dirty().

> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> index 52d55ddea8d4f..3192f610f6962 100644
> --- a/arch/s390/kvm/gmap.c
> +++ b/arch/s390/kvm/gmap.c
> @@ -327,7 +327,7 @@ static long gmap_clear_young_crste(union crste *crstep, gfn_t gfn, gfn_t end, st
>  		new.h.i = 1;
>  		new.s.fc1.y = 0;
>  		new.s.fc1.prefix_notif = 0;
> -		if (new.s.fc1.d || !new.h.p)
> +		if ((new.s.fc1.d || !new.h.p) && !new.s.fc1.s)
>  			folio_set_dirty(phys_to_folio(crste_origin_large(crste)));

[Severity: Critical]
This isn't a bug introduced by this patch, but while looking at the special bit
check added here, I noticed similar missing checks in other paths.

In arch/s390/kvm/gmap.c:_gmap_unmap_crste():

    if (old.s.fc1.pr && test_bit(GMAP_FLAG_EXPORT_ON_UNMAP, &priv->gmap->flags))
        folio = phys_to_folio(crste_origin_large(old));

And _gmap_unmap_pte():

    if (ptep->s.pr && test_bit(GMAP_FLAG_EXPORT_ON_UNMAP, &priv->gmap->flags))
        folio = pfn_folio(ptep->h.pfra);

Are we missing a check for the special bit here before converting the physical
address to a folio? If a Secure Execution VM uses device memory (like
VM_PFNMAP) without a backing struct page, could extracting the PFN and
calling phys_to_folio() / pfn_folio() return a garbage pointer and cause a
host panic when accessed?

Similarly, in _destroy_pages_crste():

    if (!crstep->h.fc || !crstep->s.fc1.pr)
        return 0;
    origin = crste_origin_large(*crstep);

And _destroy_pages_pte():

    if (!ptep->s.pr)
        return 0;
    __kvm_s390_pv_destroy_page(phys_to_page(pte_origin(*ptep)));

Could these paths also trigger a host panic by unconditionally calling
phys_to_page() on special pages?

[Severity: Critical]
This is a pre-existing issue, but I see a similar pattern in dat.c.

In arch/s390/kvm/dat.c:dat_reset_skeys_crste():

    if (!crstep->h.fc || !crstep->s.fc1.pr)
        return 0;
    // ...
        addr = sske_frame(addr, PAGE_DEFAULT_KEY);

In dat_reset_reference_bit():

    if (!crste.h.fc || !crste.s.fc1.pr)
        return 0;
    return page_reset_referenced(large_crste_to_phys(*crstep, gfn));

In dat_get_storage_key():

    if (!crste.h.fc || !crste.s.fc1.pr)
        return 0;
    skey->skey = page_get_storage_key(large_crste_to_phys(crste, gfn));

Does this code fail to check the special bit before executing inline
assemblies (sske, rrbe, iske) on the origin physical address?

If a guest maps I/O memory and executes these instructions, would KVM execute
them on the host side using the I/O physical address, potentially causing an
addressing exception and host panic since I/O memory does not support storage
keys?

[Severity: Critical]
This is also a pre-existing issue, but I noticed another unprotected
pfn_folio() call in pv.c.

In arch/s390/kvm/pv.c:_kvm_s390_pv_make_secure():

    folio = pfn_folio(f->pfn);
    priv->rc = -EAGAIN;
    // ...
    if (folio_trylock(folio)) {
        priv->rc = __kvm_s390_pv_make_secure(f, folio);

Is it possible for f->pfn to be a raw device PFN (where f->page is NULL) if
the fault resolves to a special device mapping like VM_PFNMAP?

If so, would calling pfn_folio() and folio_trylock() on an invalid folio
pointer lead to a host kernel panic?

>  		new.s.fc1.d = 0;
>  		new.h.p = 1;
>  	} while (!dat_crstep_xchg_atomic(crstep, crste, new, gfn, walk->asce));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622160739.202044-1-imbrenda@linux.ibm.com?part=3