Skip to contentSkip to conversation

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

PATCH In review Reese Faucette 3 participants 4 messages Apr 30, 2015 – May 13, 2015

lore
Share & tools
lore

Reese Faucette

author
#0 lore raw

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

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.

====================================================

mmap.c +1 -1
@@ -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? */
#1 lore raw
Quoted 14 lines · On Wed, 29 Apr 2015 22:14:19 -0700 "Reese Faucette" <[email protected]>…

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

(quoted diff) +0 -2
-EOVERFLOW, because pgoff + (len >> PAGE_SHIFT) will be 0, which is < pgoff.
-reese

Reese Faucette
Cisco Systems, Inc.

====================================================

mmap.c line 1247

Quoted 8 lines
mmap.c +1 -1
@@ -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 [email protected] or some Cisco address, as people often prefer.

#2 lore raw
In reply to Reese Faucette
Quoted 6 lines · 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.

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

author
#3 lore raw

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

Quoted 26 lines

-----Original Message-----
From: Rasmus Villemoes [mailto:[email protected]]
Sent: Friday, May 08, 2015 2:46 AM
To: Reese Faucette
Cc: [email protected]; [email protected]
Subject: Re: [PATCH] overflow check calculation in mm/mmap.c is incorrect linux-3.12.38

Quoted 7 lines · 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.

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.

Quoted 3 lines

So it seems to be by design.

But I'm also a little confused, since pgoff should be in units of pages

(so a

Quoted 5 lines

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