[PATCH] s390/mm: Add missing vm_start <= addr check to do_secure_storage_access()

[PATCH] s390/mm: Add missing vm_start <= addr check to do_secure_storage_access()

[Heiko Carstens]

do_secure_storage_access() uses find_vma() without verifying that the
faulting address is within the returned vma. Add this missing check by
converting to lock_mm_and_find_vma().

This is not a critical fix, since the worst that could happen is
WARN_ON_ONCE() in folio_walk_start().

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260717093904.E4A421F00A3E@smtp.kernel.org/
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/mm/fault.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 028aeb9c48d6..d6c58d78edc5 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -456,10 +456,9 @@ void do_secure_storage_access(struct pt_regs *regs)
 		if (faulthandler_disabled())
 			return handle_fault_error_nolock(regs, 0);
 		mm = current->mm;
-		mmap_read_lock(mm);
-		vma = find_vma(mm, addr);
+		vma = lock_mm_and_find_vma(mm, addr, regs);
 		if (!vma)
-			return handle_fault_error(regs, SEGV_MAPERR);
+			return handle_fault_error_nolock(regs, SEGV_MAPERR);
 		folio = folio_walk_start(&fw, vma, addr, 0);
 		if (!folio) {
 			mmap_read_unlock(mm);
-- 
2.53.0

Re: [PATCH] s390/mm: Add missing vm_start <= addr check to do_secure_storage_access()

[Alexander Gordeev]

On Fri, Jul 17, 2026 at 03:14:07PM +0200, Heiko Carstens wrote:
> do_secure_storage_access() uses find_vma() without verifying that the
> faulting address is within the returned vma. Add this missing check by
> converting to lock_mm_and_find_vma().
> 
> This is not a critical fix, since the worst that could happen is
> WARN_ON_ONCE() in folio_walk_start().
> 
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/20260717093904.E4A421F00A3E@smtp.kernel.org/
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/mm/fault.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 028aeb9c48d6..d6c58d78edc5 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -456,10 +456,9 @@ void do_secure_storage_access(struct pt_regs *regs)
>  		if (faulthandler_disabled())
>  			return handle_fault_error_nolock(regs, 0);
>  		mm = current->mm;
> -		mmap_read_lock(mm);
> -		vma = find_vma(mm, addr);
> +		vma = lock_mm_and_find_vma(mm, addr, regs);
>  		if (!vma)
> -			return handle_fault_error(regs, SEGV_MAPERR);
> +			return handle_fault_error_nolock(regs, SEGV_MAPERR);
>  		folio = folio_walk_start(&fw, vma, addr, 0);
>  		if (!folio) {
>  			mmap_read_unlock(mm);

The Sashiko feedback looks like a false positive to me.

Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>

Re: [PATCH] s390/mm: Add missing vm_start <= addr check to do_secure_storage_access()

[Heiko Carstens]

On Fri, Jul 24, 2026 at 02:09:32PM +0200, Alexander Gordeev wrote:
> On Fri, Jul 17, 2026 at 03:14:07PM +0200, Heiko Carstens wrote:
> > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > index 028aeb9c48d6..d6c58d78edc5 100644
> > --- a/arch/s390/mm/fault.c
> > +++ b/arch/s390/mm/fault.c
> > @@ -456,10 +456,9 @@ void do_secure_storage_access(struct pt_regs *regs)
> >  		if (faulthandler_disabled())
> >  			return handle_fault_error_nolock(regs, 0);
> >  		mm = current->mm;
> > -		mmap_read_lock(mm);
> > -		vma = find_vma(mm, addr);
> > +		vma = lock_mm_and_find_vma(mm, addr, regs);
> >  		if (!vma)
> > -			return handle_fault_error(regs, SEGV_MAPERR);
> > +			return handle_fault_error_nolock(regs, SEGV_MAPERR);
> >  		folio = folio_walk_start(&fw, vma, addr, 0);
> >  		if (!folio) {
> >  			mmap_read_unlock(mm);
> 
> The Sashiko feedback looks like a false positive to me.
> 
> Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>

This has been integrated into the "[PATCH v4 0/8] s390: Reintroduce
support for DCACHE_WORD_ACCESS" series. First I thought of keeping
things separate, but decided then to address all of Sashiko reports
and make the code look quite similar to do_exception().

So v4 is the only series that matters, everything else can be ignored.

What is still missing is kmemleak handling for which Christian posted
a patch, but that appears to be incomplete. Or least my thinking is
that there will be a subsequent patch.

Christian?

Re: [PATCH] s390/mm: Add missing vm_start <= addr check to do_secure_storage_access()

[Christian Borntraeger]

Am 24.07.26 um 14:48 schrieb Heiko Carstens:
> On Fri, Jul 24, 2026 at 02:09:32PM +0200, Alexander Gordeev wrote:
>> On Fri, Jul 17, 2026 at 03:14:07PM +0200, Heiko Carstens wrote:
>>> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
>>> index 028aeb9c48d6..d6c58d78edc5 100644
>>> --- a/arch/s390/mm/fault.c
>>> +++ b/arch/s390/mm/fault.c
>>> @@ -456,10 +456,9 @@ void do_secure_storage_access(struct pt_regs *regs)
>>>   		if (faulthandler_disabled())
>>>   			return handle_fault_error_nolock(regs, 0);
>>>   		mm = current->mm;
>>> -		mmap_read_lock(mm);
>>> -		vma = find_vma(mm, addr);
>>> +		vma = lock_mm_and_find_vma(mm, addr, regs);
>>>   		if (!vma)
>>> -			return handle_fault_error(regs, SEGV_MAPERR);
>>> +			return handle_fault_error_nolock(regs, SEGV_MAPERR);
>>>   		folio = folio_walk_start(&fw, vma, addr, 0);
>>>   		if (!folio) {
>>>   			mmap_read_unlock(mm);
>>
>> The Sashiko feedback looks like a false positive to me.
>>
>> Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>
> 
> This has been integrated into the "[PATCH v4 0/8] s390: Reintroduce
> support for DCACHE_WORD_ACCESS" series. First I thought of keeping
> things separate, but decided then to address all of Sashiko reports
> and make the code look quite similar to do_exception().
> 
> So v4 is the only series that matters, everything else can be ignored.
> 
> What is still missing is kmemleak handling for which Christian posted
> a patch, but that appears to be incomplete. Or least my thinking is
> that there will be a subsequent patch.
> 
> Christian?

Yes, its on my todo list to extend the initial patch to also take care of
the other storage areas. Still doing other things first.....