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

[Helge Deller]

* Helge Deller <deller@xxxxxx>:
> 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 <deller@xxxxxx>
> > 	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 <joern@xxxxxxxxx>
> > ---
> >  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