Re: [PATCH v1 2/6] KVM: s390: Fix _gaccess_shadow_fault()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Accessing `sg->parent` outside of `children_lock` leads to a TOCTOU NULL pointer dereference if the shadow gmap is concurrently unshadowed.
--

--- Patch [2]: [PATCH v1 2/6] KVM: s390: Fix _gaccess_shadow_fault() ---
commit 64108760233a08b4a69c94be48f33b269ea23a85
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix _gaccess_shadow_fault()

This commit addresses an issue where nested guest memory is not present in
the parent guest gmap, causing dat_entry_walk() to return -ENOENT. It fixes
this by manually calling gmap_link() on the nested guest's memory if the
mapping is absent.

> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 36102b2727fbf..0d5d46d4c66fd 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1593,12 +1593,25 @@ static inline int ___gaccess_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg
>  	parent = READ_ONCE(sg->parent);
>  	if (!parent)
>  		return -EAGAIN;
> +retry:
>  	scoped_guard(spinlock, &parent->children_lock) {
>  		if (READ_ONCE(sg->parent) != parent)
>  			return -EAGAIN;
>  		sg->invalidated = false;
>  		rc = _gaccess_do_shadow(vcpu->arch.mc, sg, saddr, walk);
>  	}
> +	if (rc == -ENOENT) {
> +		struct kvm_memory_slot *slot;
> +		struct guest_fault *entries;
> +
> +		entries = get_entries(walk);
> +		slot = kvm_vcpu_gfn_to_memslot(vcpu, entries[LEVEL_MEM].gfn);
> +		if (!slot)
> +			return PGM_ADDRESSING;
> +		rc = gmap_link(vcpu->arch.mc, sg->parent, entries + LEVEL_MEM, slot);

[Severity: Critical]
Could accessing sg->parent here lead to a NULL pointer dereference?

The scoped_guard block drops parent->children_lock before this code is
reached. If another vCPU concurrently unshadows sg, sg->parent could be
set to NULL. Because this passes sg->parent directly instead of using the
cached parent variable, does this risk passing NULL into gmap_link() and
causing a host kernel panic?

> +		if (!rc)
> +			goto retry;
> +	}
>  	if (!rc)
>  		kvm_s390_release_faultin_array(vcpu->kvm, walk->raw_entries, false);
>  	return rc;

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