Re: [PATCH v3 8/9] KVM: s390: vsie: Fix guest page tables protection

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Uninitialized variable 'gl' used in condition, leading to skipped allocation and host kernel NULL pointer dereference.
--

--- Patch [8]: [PATCH v3 8/9] KVM: s390: vsie: Fix guest page tables protection ---
commit ff58b599b812f86d2edff5eab1c005a2ef716cc7
Author: Claudio Imbrenda <[email protected]>

KVM: s390: vsie: Fix guest page tables protection

This patch adjusts the levels of guest page tables that are write-protected
when shadowing, and avoids double protecting the last level. It also skips
attempting to shadow the DAT tables when the nested guest is running in a
real address space.

> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 0ac2d775d4c05..93a757749a6ee 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1518,6 +1518,13 @@ static int _gaccess_do_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *sg,
>  	    (!ptep && crste_leaf(*table) && !table->h.i && table->h.p == w->p))
>  		return 0;
>  
> +	/* In case of a real address space */
> +	if (w->level <= LEVEL_MEM) {
> +		l = TABLE_TYPE_PAGE_TABLE;
> +		hl = TABLE_TYPE_REGION1;
> +		goto real_address_space;
> +	}
> +
>  	gl = get_level(table, ptep);

[ ... ]

> @@ -1542,6 +1549,7 @@ static int _gaccess_do_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *sg,
>  	/* Get the smallest granularity */
>  	l = min3(gl, hl, w->level);
>  
> +real_address_space:
>  	flags = DAT_WALK_SPLIT_ALLOC | (uses_skeys(sg->parent) ? DAT_WALK_USES_SKEYS : 0);
>  	/* If necessary, create the shadow mapping */
>  	if (l < gl) {
                  ^^
[Severity: Critical]
Does this leave the variable gl uninitialized when taking the goto?

When a nested guest is running in a real address space, the new goto
statement bypasses the initialization of gl:

_gaccess_do_shadow() {
    ...
    if (w->level <= LEVEL_MEM) {
        l = TABLE_TYPE_PAGE_TABLE;
        hl = TABLE_TYPE_REGION1;
        goto real_address_space;
    }
    
    gl = get_level(table, ptep);
    ...
real_address_space:
    ...
    if (l < gl) {

If the uninitialized stack value for gl causes the if (l < gl) condition
to evaluate to false, the dat_entry_walk() call is skipped. 

Since ptep was initialized to NULL at the start of _gaccess_do_shadow()
and the initial dat_entry_walk() may have stopped early without setting it,
ptep can remain NULL. 

Later in the function, _do_shadow_pte() is called with this NULL ptep,
which could lead to a NULL pointer dereference in pgste_get_trylock() and
a host kernel panic.
        
Is there a way to ensure gl is initialized before the jump, or provide a
safe default when running in a real address space?

>  		rc = dat_entry_walk(mc, gpa_to_gfn(saddr), sg->asce, flags, l, &table, &ptep);
>  		if (rc)
>  			return rc;
>  	}

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