Re: [GIT PULL v1 06/12] 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:

Pre-existing issues:
- [Critical] Missing validation for special pages in KVM GMAP unmap and destroy paths leads to host kernel crash on invalid folio pointer dereference.
- [Critical] Missing check for a valid `struct page` before folio conversion during PV guest secure page creation causes host crash.
--

commit 5f6958e6ac1a501221bda61b5fd8d27af20f157d
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.

> 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)));
>  		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 page check here, do we need similar checks in other places?

For instance, in _gmap_unmap_pte(), does this code safely handle special 
pages when calling pfn_folio() on the PTE's PFN?

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, in _gmap_unmap_crste(), does this code need a check for 
!old.s.fc1.s before calling phys_to_folio() on a large page origin?

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 in _destroy_pages_pte(), does it need a check for !ptep->s.s to avoid 
an invalid struct page dereference?

arch/s390/kvm/gmap.c:_destroy_pages_pte() {
    ...
	__kvm_s390_pv_destroy_page(phys_to_page(pte_origin(*ptep)));
    ...
}

Also in _destroy_pages_crste(), should there be a check for !crstep->s.fc1.s
before calling phys_to_page()?

arch/s390/kvm/gmap.c:_destroy_pages_crste() {
    ...
	for ( ; cur < end; cur += PAGE_SIZE)
		__kvm_s390_pv_destroy_page(phys_to_page(cur));
    ...
}

Can unprivileged userspace trigger a host kernel crash by mapping a memory
region backed by special PFNs without struct pages, converting it to a
protected VM, and then tearing it down or unmapping it?

[Severity: Critical]
This is also a pre-existing issue, but does _kvm_s390_pv_make_secure() 
need a check for a valid struct page before converting the folio and 
attempting to acquire its lock?

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

Could unprivileged userspace map PFNMAP memory without a backing struct 
page into a PV guest and cause a host crash when this path is triggered
during guest memory fault?

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