Re: [GIT PULL v1 08/11] KVM: s390: Fix race in _destroy_pages_crste()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Time-of-check to time-of-use (TOCTOU) vulnerability in `_destroy_pages_pte()` allows arbitrary physical page destruction.
- [High] _destroy_pages_crste loops over up to 524,288 pages for a 2GB large page without yielding, causing massive latency and soft lockups.
- [High] Stale PTE value used to construct new PTE in `gmap_clear_young_pte()` overwrites concurrent changes.
--

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

KVM: s390: Fix race in _destroy_pages_crste()
    
This commit uses READ_ONCE() in _destroy_pages_crste() to read the crste,
avoiding dereferencing the pointer multiple times.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index 4968330e9553b..3f3fa864cc36b 100644
> --- a/arch/s390/kvm/gmap/gmap.c
> +++ b/arch/s390/kvm/gmap/gmap.c
> @@ -994,11 +994,13 @@ static long _destroy_pages_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct da
>  static long _destroy_pages_crste(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
>  {
>  	phys_addr_t origin, cur, end;
> +	union crste crste;
>  
> -	if (!crstep->h.fc || !crstep->s.fc1.pr)
> +	crste = READ_ONCE(*crstep);
> +	if (!crste.h.fc || !crste.s.fc1.pr)
>  		return 0;
>  
> -	origin = crste_origin_large(*crstep);
> +	origin = crste_origin_large(crste);
>  	cur = ((max(gfn, walk->start) - gfn) << PAGE_SHIFT) + origin;
>  	end = ((min(next, walk->end) - gfn) << PAGE_SHIFT) + origin;
>  	for ( ; cur < end; cur += PAGE_SIZE)

[Severity: High]
This is a pre-existing issue, but does this loop process the entirety of the
large page before checking if preemption is needed?

If a PV guest is backed by 2GB large pages (Region 3 / PUD), could the loop
execute up to 524,288 Ultravisor calls without yielding, potentially causing
massive latency or a host soft lockup?

[Severity: Critical]
While looking at this file, I also noticed a pre-existing potential TOCTOU
vulnerability in _destroy_pages_pte():

arch/s390/kvm/gmap/gmap.c:_destroy_pages_pte() {
	if (!ptep->s.pr)
		return 0;
	__kvm_s390_pv_destroy_page(phys_to_page(pte_origin(*ptep)));
}

Can a concurrent modification to the PTE (e.g., by another thread handling a
page fault or unmapping memory) occur between the lockless ptep->s.pr check
and the subsequent pte_origin(*ptep) fetch?

If the PTE is cleared concurrently, could pte_origin evaluate to 0, causing
phys_to_page(0) to be targeted by the Ultravisor hypercall? Could this allow
an untrusted guest vCPU to cause destruction of an arbitrary host physical
page?

[Severity: High]
Additionally, there is a pre-existing issue where a stale PTE value appears
to be used to construct a new PTE in gmap_clear_young_pte(), which could
overwrite concurrent changes:

arch/s390/kvm/gmap/gmap.c:gmap_clear_young_pte() {
	pte = READ_ONCE(*ptep);

	if (!pte.s.pr || (!pte.s.y && pte.h.i))
		return 0;

	pgste = pgste_get_lock(ptep);
	if (!pgste.prefix_notif || gmap_mkold_prefix(p->gmap, gfn, end)) {
		new = pte;
		new.h.i = 1;
		new.s.y = 0;
...
		pgste = __dat_ptep_xchg(ptep, pgste, new, gfn, walk->asce,
					uses_skeys(p->gmap));
...
}

Since kvm_age_gfn() takes the KVM MMU lock for read, concurrent KVM fault
handlers can also modify the PTE. If a concurrent fault modifies the PTE
after gmap_clear_young_pte locklessly reads it, but before it acquires
pgste_get_lock, wouldn't the concurrent modification (such as setting the
dirty bit) be silently overwritten because the stale read value is used to
construct the new PTE?

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