[PATCH] overflow check calculation in mm/mmap.c is incorrect linux-3.12.38

[PATCH] overflow check calculation in mm/mmap.c is incorrect linux-3.12.38

[Reese Faucette]

When checking for overflow, the code in mm/mmap.c compares the first byte
*after* the end of mapped region to the start of the region instead of the
last byte of the mapped region.  This prevents mapping a region which abuts
the end of physical space, as mmap() incorrectly rejects the region with
-EOVERFLOW, because pgoff + (len >> PAGE_SHIFT) will be 0, which is < pgoff.
-reese

Reese Faucette
Cisco Systems, Inc.

====================================================
--- mm/mmap.c
+++ mm/mmap.c
@@ -1241,7 +1241,7 @@
                return -ENOMEM;

        /* offset overflow? */
-       if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
+       if ((pgoff + (len >> PAGE_SHIFT) - 1) < pgoff)
                return -EOVERFLOW;

        /* Too many mappings? */

Re: [PATCH] overflow check calculation in mm/mmap.c is incorrect linux-3.12.38

[Andrew Morton]

On Wed, 29 Apr 2015 22:14:19 -0700 "Reese Faucette" <reesefaucette@gmail.com> wrote:

> When checking for overflow, the code in mm/mmap.c compares the first byte
> *after* the end of mapped region to the start of the region instead of the
> last byte of the mapped region.  This prevents mapping a region which abuts
> the end of physical space, as mmap() incorrectly rejects the region with
> -EOVERFLOW, because pgoff + (len >> PAGE_SHIFT) will be 0, which is < pgoff.
> -reese
> 
> Reese Faucette
> Cisco Systems, Inc.
> 
> ====================================================
> --- mm/mmap.c
> +++ mm/mmap.c
> @@ -1241,7 +1241,7 @@
>                 return -ENOMEM;
> 
>         /* offset overflow? */
> -       if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
> +       if ((pgoff + (len >> PAGE_SHIFT) - 1) < pgoff)
>                 return -EOVERFLOW;
> 
>         /* Too many mappings? */

Looks right.

Please send a Signed-off-by: for this patch, as per
Documentation/SubmittingPatches, section 11.  Please also let me know
whether the authorship should be you@gmail.com or some Cisco address,
as people often prefer.

Re: [PATCH] overflow check calculation in mm/mmap.c is incorrect linux-3.12.38

[Rasmus Villemoes]

On Thu, Apr 30 2015, "Reese Faucette" <reesefaucette@gmail.com> wrote:

> When checking for overflow, the code in mm/mmap.c compares the first byte
> *after* the end of mapped region to the start of the region instead of the
> last byte of the mapped region.  This prevents mapping a region which abuts
> the end of physical space, as mmap() incorrectly rejects the region with
> -EOVERFLOW, because pgoff + (len >> PAGE_SHIFT) will be 0, which is <
> pgoff.

Note this comment elsewhere in mmap.c:

 * We don't check here for the merged mmap wrapping around the end of pagecache
 * indices (16TB on ia32) because do_mmap_pgoff() does not permit mmap's which
 * wrap, nor mmaps which cover the final page at index -1UL.

So it seems to be by design.

But I'm also a little confused, since pgoff should be in units of pages (so a
20 bit number on 32bit), and I can't see how adding another 20 bit
number could ever make that overflow. Unless of course some magic power
ensures that pgoffs in the high half get sign-extended.

Rasmus

RE: [PATCH] overflow check calculation in mm/mmap.c is incorrect linux-3.12.38

[Reese Faucette]

Rasmus-
I think you're right - I was not paying close enough attention.  In this
case, I think the real culprit appears to be something like libc incorrectly
sign-extending the mmap() offset argument.  Calling mmap() with an offset of
0xf80000000 is resulting in a pg_off of 0xffff8000 when the syscall wrappers
convert the mmap() call to mmap_pgoff().  This is on MIPS linux - I'll dig
further on the user side to see why the offset is being sign extended.  The
proper casts *seem* to be in place in libc, but its plainly not doing what
it's supposed to.
-reese

> -----Original Message-----
> From: Rasmus Villemoes [mailto:linux@rasmusvillemoes.dk]
> Sent: Friday, May 08, 2015 2:46 AM
> To: Reese Faucette
> Cc: linux-kernel@vger.kernel.org; alan@lxorguk.ukuu.org.uk
> Subject: Re: [PATCH] overflow check calculation in mm/mmap.c is incorrect
> linux-3.12.38
> 
> On Thu, Apr 30 2015, "Reese Faucette" <reesefaucette@gmail.com> wrote:
> 
> > When checking for overflow, the code in mm/mmap.c compares the first
> > byte
> > *after* the end of mapped region to the start of the region instead of
> > the last byte of the mapped region.  This prevents mapping a region
> > which abuts the end of physical space, as mmap() incorrectly rejects
> > the region with -EOVERFLOW, because pgoff + (len >> PAGE_SHIFT) will
> > be 0, which is < pgoff.
> 
> Note this comment elsewhere in mmap.c:
> 
>  * We don't check here for the merged mmap wrapping around the end of
> pagecache
>  * indices (16TB on ia32) because do_mmap_pgoff() does not permit mmap's
> which
>  * wrap, nor mmaps which cover the final page at index -1UL.
> 
> So it seems to be by design.
> 
> But I'm also a little confused, since pgoff should be in units of pages
(so a
> 20 bit number on 32bit), and I can't see how adding another 20 bit number
> could ever make that overflow. Unless of course some magic power ensures
> that pgoffs in the high half get sign-extended.
> 
> Rasmus