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

From: Helge Deller
Date: Fri Jun 30 2017 - 03:34:38 EST


* 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