Re: [PATCH v6 03/10] 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 check for special pages (`!s.s` / `!old.s.fc1.s`) in KVM gmap unmap and destroy paths causes operations on garbage folio/page pointers.
- [Critical] The UVC handler `_kvm_s390_pv_make_secure()` unconditionally converts the faulting PFN to a folio, enabling a malicious PV guest to crash the host by attempting to secure an MMIO region.
- [High] The `s` (Special) bit is lost when a large page (PMD) is split into PTEs, leading to illegal `folio_set_dirty()` calls on special pages.
--

commit c8813ca7d60b80b32bbd7cf3d235de6086bea6b4
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Do not set special large pages dirty

Special pages / folios should not be set dirty. This also applies to
large pages. Add a missing check in gmap_clear_young_crste() to prevent
setting the large page dirty if it is a special page.

> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> --- 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)));
>  		new.s.fc1.d = 0;
>  		new.h.p = 1;

[Severity: Critical]
This isn't a bug introduced by this patch, but since you are fixing the
special bit check here, I noticed other unmap and destroy paths might be
missing similar checks.

In _gmap_unmap_pte() and _gmap_unmap_crste(), the folio is extracted and
converted without verifying if the special bit is clear:

arch/s390/kvm/gmap.c:_gmap_unmap_pte() {
    ...
    if (ptep->s.pr && test_bit(GMAP_FLAG_EXPORT_ON_UNMAP, &priv->gmap->flags))
        folio = pfn_folio(ptep->h.pfra);
    ...
}

Similarly, _destroy_pages_pte() and _destroy_pages_crste() check for presence
but seem to skip checking the special bit before calling phys_to_page().

Can this lead to host kernel panics? If a protected VM has MMIO regions
mapped, could unmapping or destroying the VM cause these functions to extract
a garbage pointer from the special PFN and crash the host upon dereference?

[Severity: Critical]
This is a pre-existing issue, but while looking at PFN conversions, does
the UVC handler correctly validate the faulting PFN before converting it?

arch/s390/kvm/pv.c:_kvm_s390_pv_make_secure() {
    ...
    folio = pfn_folio(f->pfn);
    priv->rc = -EAGAIN;
    ...
    if (folio_trylock(folio)) {
    ...
}

Could a malicious PV guest issue a secure storage conversion request on a
guest physical address corresponding to an MMIO region? If KVM's fault-in
logic resolves the PFN without a backing struct page, would the unconditional
pfn_folio() conversion and subsequent trylock result in a host panic?

[Severity: High]
This is a pre-existing issue, but I noticed the special bit might be getting
lost during page table splitting.

When dat_split_ste() copies properties from the old PMD to the new PTEs,
it appears to omit the special bit:

arch/s390/kvm/dat.c:dat_split_ste() {
    ...
    init.val = pmd_origin_large(old);
    init.h.p = old.h.p;
    init.h.i = old.h.i;
    init.s.d = old.s.fc1.d;
    init.s.w = old.s.fc1.w;
    init.s.y = old.s.fc1.y;
    init.s.sd = old.s.fc1.sd;
    init.s.pr = old.s.fc1.pr;
    ...
}

If a special large page is split, will the resulting PTEs permanently lose
their special designation? Could this cause aging paths to mistakenly skip
the special check and attempt to call folio_set_dirty() on an invalid
pointer later?

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