Re: [PATCH v2 6/6] KVM: s390: Fix potential races in dat skey functions

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

New issues:
- [High] The functions `dat_get_storage_key` and `dat_reset_reference_bit` loop infinitely when the requested guest address is unmapped, leading to a host lockup (Denial of Service).

Pre-existing issues:
- [Critical] Missing TOCTOU fix for concurrent large page split in `_destroy_pages_crste()` allows KVM to use out-of-bounds physical addresses, leading to host memory corruption or panic.
--

--- Patch [6]: [PATCH v2 6/6] KVM: s390: Fix potential races in dat skey functions ---
commit 7ad4916f2fab0f5e4f1cfe048d34867e81b79c1b
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix potential races in dat skey functions

This commit addresses a race window between dat_entry_walk() and
large_crste_to_phys() where a large page could be split concurrently.
It fixes this by using READ_ONCE on the crste pointer and retrying the
walk if a race is detected.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index 7e5dd5a1eb1ef..b4c318ebd91e9 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -620,17 +620,20 @@ int dat_get_storage_key(union asce asce, gfn_t gfn, union skey *skey)
>  	union pte *ptep;
>  	int rc;
>  
> +again:
>  	skey->skey = 0;
>  	rc = dat_entry_walk(NULL, gfn, asce, DAT_WALK_ANY, TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);
>  	if (rc)
>  		return rc;
>  
>  	if (!ptep) {
> -		union crste crste;
> +		union crste crste = READ_ONCE(*crstep);
>  
> -		crste = READ_ONCE(*crstep);
> -		if (!crste.h.fc || !crste.s.fc1.pr)
> +		if (!crste_leaf(crste))
> +			goto again;

[Severity: High]
If the guest address is unmapped, dat_entry_walk() with DAT_WALK_ANY can
return 0 with ptep == NULL and an unmapped directory entry (where crste.h.fc
is 0).

Since crste_leaf() returns false when fc is 0, won't this unconditionally
trigger the goto again path and create an infinite loop?

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

[ ... ]

> @@ -775,9 +792,12 @@ int dat_reset_reference_bit(union asce asce, gfn_t gfn, union skey *skey)
>  	if (!ptep) {
>  		union crste crste = READ_ONCE(*crstep);
>  
> -		if (!crste.h.fc || !crste.s.fc1.pr)
> +		/* A large page has been split concurrently, try again */
> +		if (!crste_leaf(crste))
> +			goto again;

[Severity: High]
Does this also suffer from the same infinite loop issue as
dat_get_storage_key() when processing an unmapped physical address?

> +		if (!crste.s.fc1.pr)
>  			return 0;
> -		skey->skey = page_reset_referenced(large_crste_to_phys(*crstep, gfn)) << 1;
> +		skey->skey = page_reset_referenced(large_crste_to_phys(crste, gfn)) << 1;
>  		return 0;
>  	}

[Severity: Critical]
This isn't a bug introduced by this patch, but while reviewing the TOCTOU
fixes here, I noticed a similar pre-existing issue in
arch/s390/kvm/gmap.c:_destroy_pages_crste():

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

	origin = crste_origin_large(*crstep);

The crstep pointer is dereferenced twice without READ_ONCE(). If a concurrent
large page split occurs between these reads, crste_origin_large() could return
-1, passing an out-of-bounds address to phys_to_page() and
__kvm_s390_pv_destroy_page().

Should this also be updated with READ_ONCE() to prevent potential host memory
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6