Date: Fri, 30 Jun 2017 09:34:24 +0200
From: Helge Deller <deller@gmx.de>
To: =?iso-8859-15?Q?J=F6rn?= Engel <joern@purestorage.com>
Cc: Hugh Dickins <hughd@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm: Fix overflow check in expand_upwards()
Message-ID: <20170630073424.GA4800@ls3530>
References: <20170629230256.GB494@cork>
 <747944b6-ffb8-14db-d574-efc03e11f2a5@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-15
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <747944b6-ffb8-14db-d574-efc03e11f2a5@gmx.de>
User-Agent: Mutt/1.8.0 (2017-02-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3181
Lines: 92

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