[PATCH] mm: fix mmap overflow checking

[PATCH] mm: fix mmap overflow checking

[Wanlong Gao]

POSIX said that if the file is a regular file and the value of "off"
plus "len" exceeds the offset maximum established in the open file
description associated with fildes, mmap should return EOVERFLOW.

The following test from LTP can reproduce this bug.

	char tmpfname[256];
	void *pa = NULL;
	void *addr = NULL;
	size_t len;
	int flag;
	int fd;
	off_t off = 0;
	int prot;

	long page_size = sysconf(_SC_PAGE_SIZE);

	snprintf(tmpfname, sizeof(tmpfname), "/tmp/mmap_test_%d", getpid());
	unlink(tmpfname);
	fd = open(tmpfname, O_CREAT | O_RDWR | O_EXCL, S_IRUSR | S_IWUSR);
	if (fd == -1) {
		printf(" Error at open(): %s\n", strerror(errno));
		return 1;
	}
	unlink(tmpfname);

	flag = MAP_SHARED;
	prot = PROT_READ | PROT_WRITE;

	/* len + off > maximum offset */

	len = ULONG_MAX;
	if (len % page_size) {
		/* Lower boundary */
		len &= ~(page_size - 1);
	}

	off = ULONG_MAX;
	if (off % page_size) {
		/* Lower boundary */
		off &= ~(page_size - 1);
	}

	printf("off: %lx, len: %lx\n", (unsigned long)off, (unsigned long)len);
	pa = mmap(addr, len, prot, flag, fd, off);
	if (pa == MAP_FAILED && errno == EOVERFLOW) {
		printf("Test Pass: Error at mmap: %s\n", strerror(errno));
		return 0;
	}

	if (pa == MAP_FAILED)
		perror("Test FAIL: expect EOVERFLOW but get other error");
	else
		printf("Test FAIL : Expect EOVERFLOW but got no error\n");

	close(fd);
	munmap(pa, len);
	return 1;

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: linux-mm@kvack.org (open list:MEMORY MANAGEMENT)
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 mm/mmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index ae18a48..5380764 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -980,6 +980,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	struct mm_struct * mm = current->mm;
 	struct inode *inode;
 	vm_flags_t vm_flags;
+	off_t off = pgoff << PAGE_SHIFT;
 
 	/*
 	 * Does the application expect PROT_READ to imply PROT_EXEC?
@@ -1003,8 +1004,8 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 		return -ENOMEM;
 
 	/* offset overflow? */
-	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
-               return -EOVERFLOW;
+	if (off + len < off)
+		return -EOVERFLOW;
 
 	/* Too many mappings? */
 	if (mm->map_count > sysctl_max_map_count)
-- 
1.7.12

Re: [PATCH] mm: fix mmap overflow checking

[Andrew Morton]

On Tue, 4 Sep 2012 17:23:00 +0800
Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:

> POSIX said that if the file is a regular file and the value of "off"
> plus "len" exceeds the offset maximum established in the open file
> description associated with fildes, mmap should return EOVERFLOW.

That's what POSIX says, but what does Linux do?  It is important that
we precisely describe and understand the behaviour change, as there is
potential here to break existing applications.

I'm assuming that Linux presently permits the mmap() and then generates
SIGBUS if an access is attempted beyond the max file size?

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

Well, this treats sizeof(off_t) as the "offset maximum established in
the open file".  But from my reading of the above excerpt, we should in
fact be checking against the underlying fs's s_maxbytes?

Re: [PATCH] mm: fix mmap overflow checking

[KOSAKI Motohiro]

On Tue, Sep 4, 2012 at 5:23 AM, Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
> POSIX said that if the file is a regular file and the value of "off"
> plus "len" exceeds the offset maximum established in the open file
> description associated with fildes, mmap should return EOVERFLOW.
>
> The following test from LTP can reproduce this bug.
>
>         char tmpfname[256];
>         void *pa = NULL;
>         void *addr = NULL;
>         size_t len;
>         int flag;
>         int fd;
>         off_t off = 0;
>         int prot;
>
>         long page_size = sysconf(_SC_PAGE_SIZE);
>
>         snprintf(tmpfname, sizeof(tmpfname), "/tmp/mmap_test_%d", getpid());
>         unlink(tmpfname);
>         fd = open(tmpfname, O_CREAT | O_RDWR | O_EXCL, S_IRUSR | S_IWUSR);
>         if (fd == -1) {
>                 printf(" Error at open(): %s\n", strerror(errno));
>                 return 1;
>         }
>         unlink(tmpfname);
>
>         flag = MAP_SHARED;
>         prot = PROT_READ | PROT_WRITE;
>
>         /* len + off > maximum offset */
>
>         len = ULONG_MAX;
>         if (len % page_size) {
>                 /* Lower boundary */
>                 len &= ~(page_size - 1);
>         }
>
>         off = ULONG_MAX;
>         if (off % page_size) {
>                 /* Lower boundary */
>                 off &= ~(page_size - 1);
>         }
>
>         printf("off: %lx, len: %lx\n", (unsigned long)off, (unsigned long)len);
>         pa = mmap(addr, len, prot, flag, fd, off);
>         if (pa == MAP_FAILED && errno == EOVERFLOW) {
>                 printf("Test Pass: Error at mmap: %s\n", strerror(errno));
>                 return 0;
>         }
>
>         if (pa == MAP_FAILED)
>                 perror("Test FAIL: expect EOVERFLOW but get other error");
>         else
>                 printf("Test FAIL : Expect EOVERFLOW but got no error\n");
>
>         close(fd);
>         munmap(pa, len);
>         return 1;
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: linux-mm@kvack.org (open list:MEMORY MANAGEMENT)
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  mm/mmap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index ae18a48..5380764 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -980,6 +980,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
>         struct mm_struct * mm = current->mm;
>         struct inode *inode;
>         vm_flags_t vm_flags;
> +       off_t off = pgoff << PAGE_SHIFT;

I've seen the exactly same patch from another fujitsu guys several
month ago. and as I pointed
out at that time, this line don't work when 32bit kernel + mmap2 syscall case.

Please don't think do_mmap_pgoff() is for mmap(2) specific and read a
past thread before resend
a patch.

Re: [PATCH] mm: fix mmap overflow checking

[KOSAKI Motohiro]

>> I've seen the exactly same patch from another fujitsu guys several
>> month ago. and as I pointed
>> out at that time, this line don't work when 32bit kernel + mmap2 syscall case.
>>
>> Please don't think do_mmap_pgoff() is for mmap(2) specific and read a
>> past thread before resend
>> a patch.
>
> So, what's your opinion about this bug? How to fix it in your mind?

Fix glibc instead of kernel.

Re: [PATCH] mm: fix mmap overflow checking

[Wanlong Gao]

On 09/08/2012 09:58 AM, KOSAKI Motohiro wrote:
>>> I've seen the exactly same patch from another fujitsu guys several
>>> month ago. and as I pointed
>>> out at that time, this line don't work when 32bit kernel + mmap2 syscall case.
>>>
>>> Please don't think do_mmap_pgoff() is for mmap(2) specific and read a
>>> past thread before resend
>>> a patch.
>>
>> So, what's your opinion about this bug? How to fix it in your mind?
> 
> Fix glibc instead of kernel.
> 

OK, I will send a patch to glibc community. Thank you.

Wanlong Gao