Re: [PATCH v8 06/13] KVM: s390: Fix leaking of PGM_ADDRESSING to userspace

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

Pre-existing issues:
- [Critical] Concurrent large page split causes `large_crste_to_phys()` to dereference an invalid address (`-1`), leading to a host kernel panic via addressing exception.
--

--- Patch [6]: [PATCH v8 06/13] KVM: s390: Fix leaking of PGM_ADDRESSING to userspace ---
commit 7bad2118b8960be854e59fc638949bdda1bbace0
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix leaking of PGM_ADDRESSING to userspace

This commit fixes several key and CMMA operations to return -EFAULT instead
of PGM_ADDRESSING when acting outside of a memslot.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index ed4259d176295..171b619599089 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -771,21 +773,23 @@ int dat_reset_reference_bit(union asce asce, gfn_t gfn)
>  
>  		if (!crste.h.fc || !crste.s.fc1.pr)
>  			return 0;
> -		return page_reset_referenced(large_crste_to_phys(*crstep, gfn));
> +		skey->skey = page_reset_referenced(large_crste_to_phys(*crstep, gfn)) << 1;

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential race condition with
the large page handling here.

Because kvm->mmu_lock is only held for reading during this operation, could
another vCPU concurrently split the large page under the read lock (via
dat_split_crste()), clearing the fc bit?

If dat_entry_walk() succeeds, but the page is split before the implicit read
of *crstep in large_crste_to_phys(), large_crste_to_phys() could observe the
cleared fc bit and return -1. 

Passing -1 to the assembly block inside page_reset_referenced() could
trigger an unhandled addressing exception and panic the host kernel since it
lacks exception tables.

Would it be safer to use the already validated snapshot (crste) instead of
dereferencing *crstep again?

skey->skey = page_reset_referenced(large_crste_to_phys(crste, gfn)) << 1;

This same pattern of passing *crstep directly to large_crste_to_phys()
appears to also exist in dat_set_storage_key() and
dat_cond_set_storage_key().

> +		return 0;
>  	}
>  	old = pgste_get_lock(ptep);
>  	pgste = old;

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