Re: [PATCH v4 7/8] s390/mm: Use goto statement in do_secure_storage_access()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Stale TLB / Premature Page Freeing in VM_SPARSE area teardown
- [High] Invalid physical address translation for module addresses
--

--- Patch [7]: [PATCH v4 7/8] s390/mm: Use goto statement in do_secure_storage_access() ---
commit e37b9a92584a0cc071d6b45d285a37b7eb5137cc
Author: Heiko Carstens <hca@linux.ibm.com>

s390/mm: Use goto statement in do_secure_storage_access()

This patch refactors do_secure_storage_access() to use a goto statement
for unlocking mmap_read_unlock. It removes an open-coded unlock to make
it look more like do_exception().

> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 2a9b9fb30d09c..666fe8e994801 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -453,10 +453,8 @@ void do_secure_storage_access(struct pt_regs *regs)
[ ... ]
>  		folio_walk_end(&fw, vma);
>  		if (rc)
>  			return handle_fault_error(regs, SEGV_ACCERR);
> +out:
>  		mmap_read_unlock(mm);
>  	}
>  }

While reviewing the changes in do_secure_storage_access(), I noticed a
potential regression earlier in the same function:

arch/s390/mm/fault.c:do_secure_storage_access() {
    ...
    if (is_vmalloc_addr((void *)addr))
        return handle_fault_error_nolock(regs, 0);
    rc = uv_convert_from_secure(__pa(addr));
    ...
}

[Severity: High]
Does this incomplete vmalloc check allow module addresses to fall through
to the invalid __pa() translation?

On s390, module memory resides outside the VMALLOC_START..VMALLOC_END
range, so the is_vmalloc_addr() check returns false for module addresses.
The code then falls through to uv_convert_from_secure(__pa(addr)).

Since module addresses are not direct-mapped, __pa(addr) generates a
garbage physical address.

If this garbage address aligns with a valid physical page currently donated
to another guest as secure memory, would the Ultravisor erroneously convert
it, destroying the unrelated guest's secure state?

Should this check use is_vmalloc_or_module_addr() instead?


Also, looking at the VM_SPARSE area teardown sequence:

arch/s390/kernel/uv.c:uv_free_stor_var() {
    ...
    apply_to_existing_page_range(&init_mm, addr, size, uv_free_range_cb, NULL);
    free_vm_area(area);
    ...
}

arch/s390/kernel/uv.c:uv_free_range_cb() {
    ...
    __free_page(pte_page(*pte));
    return 0;
    ...
}

[Severity: High]
Can this sequence cause a stale TLB or premature page freeing during
VM_SPARSE area teardown?

This sequence frees the physical pages to the buddy allocator while
intentionally leaving the kernel PTEs valid. The PTEs are only cleared
and the TLB flushed later by free_vm_area().

During this window, could any speculative or stray access to the vmalloc
address access the physical page through the valid PTE/TLB, even though
the page may have already been reallocated to another subsystem or guest,
corrupting memory?

Should the teardown sequence clear the PTEs and flush the TLB before
freeing the physical pages, as is normally done during standard vfree()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720085834.898025-1-hca@linux.ibm.com?part=7