Re: [PATCH v4 2/8] s390/mm: Add missing mm check to do_secure_storage_access()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing VMA boundary check in user space fault path can trigger a kernel WARN_ON_ONCE due to a race condition with concurrent munmap.
- [High] Using virt_to_folio() on arbitrary kernel fault addresses can cause a kernel panic if the address is in vmalloc space.
- [High] Failing to run exception fixups for kernel-mode faults in `do_secure_storage_access()` causes an infinite loop.
--

--- Patch [2]: [PATCH v4 2/8] s390/mm: Add missing mm check to do_secure_storage_access() ---
commit 61b1851bdfb6749b6398a064bf9e76283ed6b1e3
Author: Heiko Carstens <hca@linux.ibm.com>

s390/mm: Add missing mm check to do_secure_storage_access()

Similar like the normal exception handler the secure storage access
exception handler needs to verify if mm is not NULL, which may happen
with kernel threads.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260717132345.539A11F00A3A@smtp.kernel.org/

> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 028aeb9c48d6f..2839d7a64401c 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -409,9 +409,9 @@ void do_secure_storage_access(struct pt_regs *regs)
>  {
>  	union teid teid = { .val = regs->int_parm_long };
>  	unsigned long addr = get_fault_address(regs);
> +	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma;
>  	struct folio_walk fw;
> -	struct mm_struct *mm;
>  	struct folio *folio;
>  	int rc;
>  
[ ... ]
>  		panic("Unexpected PGM 0x3d with TEID bit 61=0");
>  	}
>  	if (is_kernel_fault(regs)) {
>  		folio = virt_to_folio((void *)addr);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does using virt_to_folio()
unconditionally here cause a kernel panic if the fault address is in vmalloc
space?

If the kernel accesses donated secure memory that was allocated via vmalloc,
virt_to_folio() on s390 uses __pa(), which only works for direct-mapped
addresses. This would produce a garbage physical address, leading to an
out-of-bounds pointer dereference in the subsequent folio_try_get() check.

>  		if (unlikely(!folio_try_get(folio)))
>  			return;
>  		rc = uv_convert_from_secure(folio_to_phys(folio));
[ ... ]
>  		if (rc)
>  			return handle_fault_error_nolock(regs, 0);
>  	} else {
> -		if (faulthandler_disabled())
> +		if (faulthandler_disabled() || !mm)
>  			return handle_fault_error_nolock(regs, 0);
> -		mm = current->mm;
>  		mmap_read_lock(mm);
>  		vma = find_vma(mm, addr);
                ^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does find_vma() properly check that the
address is within the VMA's boundaries before we call folio_walk_start()?

find_vma() returns the first VMA with vm_end greater than addr, but its
vm_start could also be greater than addr if the faulting address was just
unmapped by a concurrent munmap(). If this happens, we would pass a VMA
to folio_walk_start() that doesn't actually contain the address, which
triggers its strict WARN_ON_ONCE assertion. Should this be using
vma_lookup() instead?

>  		if (!vma)
>  			return handle_fault_error(regs, SEGV_MAPERR);
>  		folio = folio_walk_start(&fw, vma, addr, 0);
>  		if (!folio) {
>  			mmap_read_unlock(mm);
>  			return;
>  		}
>  		/* arch_make_folio_accessible() needs a raised refcount. */
>  		folio_get(folio);
>  		rc = arch_make_folio_accessible(folio);
>  		folio_put(folio);
>  		folio_walk_end(&fw, vma);
>  		if (rc)
>  			send_sig(SIGSEGV, current, 0);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does this error path cause an
infinite loop if the fault was triggered by a kernel-mode access like
copy_to_user()?

For kernel faults that happen in the user address space, is_kernel_fault()
is false. If arch_make_folio_accessible() fails here, we send a SIGSEGV
and return without calling fixup_exception(). Because fixup_exception() is
not run, the kernel will return to the faulting instruction rather than an
error path, causing the instruction to endlessly re-execute and lock up
the CPU.

>  		mmap_read_unlock(mm);
>  	}
>  }

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