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

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Commit message and code mismatch regarding 'large' vs 'special' page.

Pre-existing issues:
- [Critical] Missing checks for 'special' page table entries before resolving to folios, leading to host kernel crashes during unmap or VM teardown.
--

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

KVM: s390: Do not set special large pages dirty

This commit prevents setting special pages or folios dirty, adding a check
specifically for large pages in gmap_clear_young_crste().

> Add a missing check in gmap_clear_young_crste() to prevent setting the
> large page dirty if it is a large page.

[Severity: Low]
This isn't a bug, but there is a typo in the commit message. The code adds
a check for the special bit (!new.s.fc1.s), but the message says "if it is
a large page" instead of "if it is a special page". Should the commit
message be updated to match the code?

> 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 is a pre-existing issue, but since this patch fixes a missing special
bit check here, are there other places missing this exact same check that
could lead to a host kernel crash?

For example, in arch/s390/kvm/gmap.c, _gmap_unmap_crste() calls
phys_to_folio() on a large page without checking if it is a special page:

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

Similarly, _gmap_unmap_pte() calls pfn_folio() on a standard PTE without
checking the special bit (ptep->s.s):

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

_destroy_pages_crste() and _destroy_pages_pte() also appear to
unconditionally resolve PFNs to page structs without checking the special
bit before passing them to __kvm_s390_pv_destroy_page().

Could a Secure/PV VM mapping an IO memory region directly (like via VFIO)
cause a local denial of service when this memory is unmapped or the VM is
destroyed, because these IO-mapped PFNs lack a valid struct page?

Also, in ptep_get_and_clear() in arch/s390/include/asm/pgtable.h,
uv_convert_from_secure_pte() is called on any present PTE in a protected mm
without verifying !pte_special(res):

	if (mm_is_protected(mm) && pte_present(res))
		WARN_ON_ONCE(uv_convert_from_secure_pte(res));

This unconditionally calls pfn_folio() on the PFN derived from the host
PTE in arch/s390/kernel/uv.c:uv_convert_from_secure_pte(). Should these
paths also be updated to ensure they check for special page table entries
before resolving them to folios?

>  		new.s.fc1.d = 0;
>  		new.h.p = 1;

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