Re: [PATCH] mm: Fix overflow check in expand_upwards()

[Helge Deller]

* Helge Deller <del...@gmx.de>:
> On 30.06.2017 01:02, Jörn Engel wrote:
> > I believe the overflow check was correct, then got subtly broken by
> >     commit bd726c90b6b8
> >     Author: Helge Deller <del...@gmx.de>
> >     Date:   Mon Jun 19 17:34:05 2017 +0200
> > 
> >         Allow stack to grow up to address space limit
> > 
> > The old overflow check may have been a bit subtle and I suppose Helge
> > missed its importance.
> > 
> >     if (!address)
> >             return -ENOMEM;
> > 
> > Functionally the my check is identical to the old one.  I just hope the
> > alternative form (and comment!) make it harder to miss and break things
> > in a future patch.
> > 
> > Signed-off-by: Joern Engel <jo...@logfs.org>
> > ---
> >  mm/mmap.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index a5e3dcd75e79..7366f62c31f4 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2232,7 +2232,8 @@ int expand_upwards(struct vm_area_struct *vma, 
> > unsigned long address)
> >  
> >     /* Guard against exceeding limits of the address space. */
> >     address &= PAGE_MASK;
> > -   if (address >= TASK_SIZE)
> > +   /* second check is for integer overflow */
> > +   if (address >= TASK_SIZE || address + PAGE_SIZE < address)
> >             return -ENOMEM;
> >     address += PAGE_SIZE;
> 
> That overflow check is still there.

I see there are some architectures which define TASK_SIZE not as
multiple of PAGE_SIZE and as 0xffffffff for which the (address >=
TASK_SIZE) check will not trigger:

arch/arm/include/asm/memory.h:#define TASK_SIZE         UL(0xffffffff)
arch/frv/include/asm/mem-layout.h:#define TASK_SIZE                     
__UL(0xFFFFFFFFUL)
arch/m68k/include/asm/processor.h:#define TASK_SIZE     (0xFFFFFFFFUL)
arch/blackfin/include/asm/processor.h:#define TASK_SIZE 0xFFFFFFFF
arch/h8300/include/asm/processor.h:#define TASK_SIZE    (0xFFFFFFFFUL)
arch/xtensa/include/asm/processor.h:#define TASK_SIZE   
__XTENSA_UL_CONST(0xffffffff)

None of those have an upwards growing stack and thus I believe we don't
run into issues, but nevertheless the checks could probably be changed
like this (untested patch):

diff --git a/mm/mmap.c b/mm/mmap.c
index a5e3dcd..224bdc2 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2224,15 +2224,17 @@ int expand_upwards(struct vm_area_struct *vma, unsigned 
long address)
 {
        struct mm_struct *mm = vma->vm_mm;
        struct vm_area_struct *next;
-       unsigned long gap_addr;
+       unsigned long gap_addr, max_task_size;
        int error = 0;
 
        if (!(vma->vm_flags & VM_GROWSUP))
                return -EFAULT;
 
+       max_task_size = TASK_SIZE & PAGE_MASK;
+
        /* Guard against exceeding limits of the address space. */
        address &= PAGE_MASK;
-       if (address >= TASK_SIZE)
+       if (address >= max_task_size)
                return -ENOMEM;
        address += PAGE_SIZE;
 
@@ -2240,8 +2242,8 @@ int expand_upwards(struct vm_area_struct *vma, unsigned 
long address)
        gap_addr = address + stack_guard_gap;
 
        /* Guard against overflow */
-       if (gap_addr < address || gap_addr > TASK_SIZE)
-               gap_addr = TASK_SIZE;
+       if (gap_addr < address || gap_addr > max_task_size)
+               gap_addr = max_task_size;
 
        next = vma->vm_next;
        if (next && next->vm_start < gap_addr) {

Helge