Re: [PATCH v3 7/9] KVM: s390: vsie: Fix unshadowing while shadowing

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

Pre-existing issues:
- [Critical] Missing unshadowing check in `_do_shadow_pte` and `_do_shadow_crste` leads to orphaned shadow mappings and use-after-free of host memory.
--

--- Patch [7]: [PATCH v3 7/9] KVM: s390: vsie: Fix unshadowing while shadowing ---
commit d5030d28123280edd38c0f9ad86e8a0833ab9f12
Author: Claudio Imbrenda <[email protected]>

KVM: s390: vsie: Fix unshadowing while shadowing

If shadowing causes the shadow gmap to get unshadowed, exit early to
prevent an attempt to dereference the parent pointer, which at this
point is NULL.

> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 70703bf866dbe..0ac2d775d4c05 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1472,6 +1472,9 @@ static int _do_shadow_crste(struct gmap *sg, gpa_t raddr, union crste *host, uni
>  		return rc;
>  
>  	do {
> +		/* _gmap_crstep_xchg_atomic() could have unshadowed this shadow gmap */
> +		if (!sg->parent)
> +			return -EAGAIN;
>  		oldcrste = READ_ONCE(*host);
>  		newcrste = _crste_fc1(f->pfn, oldcrste.h.tt, f->writable, !p);

[Severity: Critical]
This is a pre-existing issue, but since we are checking for unshadowing here,
could _gmap_crstep_xchg_atomic() unshadow the shadow gmap on the exact 
iteration where it succeeds?

If _gmap_crstep_xchg_atomic() succeeds but unshadows sg, the loop exits and
bypasses this check on the next iteration. It would then unconditionally install
the shadow entry just below the loop:

_do_shadow_crste() {
    ...
	} while (!_gmap_crstep_xchg_atomic(sg->parent, host, oldcrste, newcrste, f->gfn, false));

	newcrste = _crste_fc1(f->pfn, oldcrste.h.tt, 0, !p);
	gfn = gpa_to_gfn(raddr);
	while (!dat_crstep_xchg_atomic(table, READ_ONCE(*table), newcrste, gfn, sg->asce))
		;
    ...
}

Could this leave the guest with orphaned shadow mappings? If the host later
unmaps the page, it iterates over parent->children to invalidate shadow page
tables. Since sg was removed during unshadowing, it would fail to find and 
invalidate it, which seems like it might allow a use-after-free of host memory.

Would we need a similar check after the loop exits here, and similarly in
_do_shadow_pte() after _gmap_ptep_xchg() returns?

[ ... ]

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