Re: [PATCH v1 6/6] KVM: s390: Fix potential race in dat_cond_set_storage_key()

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

Pre-existing issues:
- [Critical] Identical TOCTOU vulnerabilities remain in `dat_set_storage_key()`, `dat_reset_reference_bit()`, and `dat_get_storage_key()`.
--

--- Patch [6]: [PATCH v1 6/6] KVM: s390: Fix potential race in dat_cond_set_storage_key() ---
commit 5e22031091ce720a5918edc8a96db1b780f83398
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix potential race in dat_cond_set_storage_key()

This commit addresses a race window in dat_cond_set_storage_key() where a
large page could be concurrently split between the page table walk and the
dereferencing of the crste pointer. By reading the entry exactly once and
checking if it is still a leaf, it prevents passing an invalid physical
address to the storage key functions.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index 7e5dd5a1eb1ef..b467b95689b21 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
[ ... ]
> @@ -725,7 +727,11 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
>  	if (!ptep) {
>  		if (!oldkey)
>  			oldkey = &prev;
> -		return page_cond_set_storage_key(large_crste_to_phys(*crstep, gfn), skey, oldkey,
> +		crste = READ_ONCE(*crstep);
> +		/* A large page has been split concurrently, try again */
> +		if (!crste_leaf(crste))
> +			goto again;
> +		return page_cond_set_storage_key(large_crste_to_phys(crste, gfn), skey, oldkey,
>  						 nq, mr, mc);
>  	}

[Severity: Critical]
This is a pre-existing issue, but do other functions in this file suffer from
the exact same TOCTOU race that this patch fixes?

Looking at dat_set_storage_key() in the same file, the pointer is dereferenced
directly without a READ_ONCE() and leaf check:

arch/s390/kvm/dat.c:dat_set_storage_key() {
    ...
    page_set_storage_key(large_crste_to_phys(*crstep, gfn), skey.skey, !nq);
    ...
}

If a concurrent split occurs here, large_crste_to_phys() will return -1, which
can result in an invalid physical address access.

Similarly, in dat_reset_reference_bit(), *crstep is dereferenced a second time:

arch/s390/kvm/dat.c:dat_reset_reference_bit() {
    ...
    skey->skey = page_reset_referenced(large_crste_to_phys(*crstep, gfn)) << 1;
    ...
}

Does this TOCTOU window also pass -1 to page_reset_referenced() if a concurrent
split happens between the read and the usage?

Finally, in dat_get_storage_key():

arch/s390/kvm/dat.c:dat_get_storage_key() {
    ...
    crste = READ_ONCE(*crstep);
    if (!crste.h.fc || !crste.s.fc1.pr)
        return 0;
    ...
}

Instead of retrying the walk via a goto loop like the fix in
dat_cond_set_storage_key(), it returns 0. Could this silently give a false
storage key to the guest if a race occurred?

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