[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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix mmap overflow checking

[Wanlong Gao]

On 09/05/2012 04:59 AM, Andrew Morton wrote:
> 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

Current Linux checks whether the shifted off+len exceed ULONG_MAX, it seems
never happen.

> 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?

What I saw is ENOMEM because the "len" here is too large.
 
> 
>> 	/* 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?

More reasonable, how about following?

---
 mm/mmap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index ae18a48..4d7bc64 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -980,6 +980,10 @@ 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;
+	loff_t off = pgoff << PAGE_SHIFT;
+	loff_t maxbytes = -1;
+	if (file)
+		maxbytes = file->f_mapping->host->i_sb->s_maxbytes;
 
 	/*
 	 * Does the application expect PROT_READ to imply PROT_EXEC?
@@ -1003,8 +1007,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 > maxbytes)
+		return -EOVERFLOW;
 
 	/* Too many mappings? */
 	if (mm->map_count > sysctl_max_map_count)
-- 

Thanks,
Wanlong Gao

> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix mmap overflow checking

[Andrew Morton]

On Wed, 05 Sep 2012 11:20:07 +0800
Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:

> On 09/05/2012 04:59 AM, Andrew Morton wrote:
> > 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
> 
> Current Linux checks whether the shifted off+len exceed ULONG_MAX, it seems
> never happen.
> 
> > 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?
> 
> What I saw is ENOMEM because the "len" here is too large.

I don't think I understand this.  You're saying that without your patch
applied, the mmap() attempt returns -ENOMEM?  If so, where in the code
does that occur?

In the current upstream kernel is there some combination of mmap()
arguments which will permit the mmap() to succeed, even though it
refers to a section of the file which lies beyond the file's maximum
offset?

> > 
> >> 	/* 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?
> 
> More reasonable, how about following?

Well I don't know.  Again, the concern here is the risk of breaking
existing applications.  So before proceeding, we need a very complete
and accurate understanding of the kernel's behaviour both before and
after this patch.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix mmap overflow checking

[Wanlong Gao]

On 09/08/2012 06:38 AM, KOSAKI Motohiro wrote:
> 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.

So, what's your opinion about this bug? How to fix it in your mind?

Thanks,
Wanlong Gao

> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>