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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing VMA Bounds Check allows Denial of Service via WARN_ON_ONCE.
- [High] Invalid Memory Access / Kernel Crash via virt_to_folio() on non-direct-mapped addresses.
--

--- Patch [2]: [PATCH v3 2/6] s390/mm: Add missing mm check to do_secure_storage_access() ---
commit 55ed80680943e84fffd67209605311fedc0e28ae
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;
>  

[ ... ]

> @@ -453,9 +453,8 @@ void do_secure_storage_access(struct pt_regs *regs)
>  		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);
>  		if (!vma)

[Severity: High]
This isn't a bug introduced by this patch, but can this cause memory
corruption?

Earlier in this same function:

arch/s390/mm/fault.c:do_secure_storage_access() {
    ...
	if (is_kernel_fault(regs)) {
		folio = virt_to_folio((void *)addr);
    ...
}

When the kernel accesses a secure guest page through a vmalloc mapping,
is_kernel_fault(regs) evaluates to true. Then virt_to_folio() is
unconditionally applied to the faulting kernel address without validating that
it is a direct-mapped address.

Since virt_to_folio() subtracts the direct map offset, wouldn't it produce a
bogus PFN for vmalloc addresses, leading to dereferencing a garbage struct page
pointer?

[Severity: High]
This is also a pre-existing issue, but could this lead to a denial of service
via WARN_ON_ONCE?

Looking at do_secure_storage_access() in arch/s390/mm/fault.c, find_vma()
returns a VMA that may start after the faulting address, which is not verified
before use.

If a concurrent thread unmaps the VMA between the time the hardware generates
the secure storage exception and the handler acquires the mmap lock, could
find_vma() return the next adjacent VMA?

If so, passing this potentially unrelated VMA to folio_walk_start():

arch/s390/mm/fault.c:do_secure_storage_access() {
    ...
		vma = find_vma(mm, addr);
		if (!vma)
			return handle_fault_error(regs, SEGV_MAPERR);
		folio = folio_walk_start(&fw, vma, addr, 0);
    ...
}

would trigger the WARN_ON_ONCE(addr < vma->vm_start || addr >= vma->vm_end)
safety check. Could a malicious user-space program exploit this race by
repeatedly triggering secure storage accesses while concurrently unmapping
regions to crash the system if panic_on_warn is enabled?

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