mm: mmap system call does not return EOVERFLOW

PATCH In review Naotaka Hamaguchi 2 participants 5 messages Dec 22, 2011 – Dec 28, 2011

Subsystems (2)

Memory management Memory mapping

lore

Share & tools

lore

Naotaka Hamaguchi

author

2011-12-22 09:35 UTC #0 lore raw

PATCH 1/1 mm: mmap system call does not return EOVERFLOW

In the system call mmap(), if the value of "offset" plus "length" exceeds the offset maximum of "off_t", the error EOVERFLOW should be returned.

------------------------------------------------------------------------
void *mmap(void *addr, size_t length, int prot, int flags,
int fd, off_t offset)
------------------------------------------------------------------------

Here is the detail how EOVERFLOW is returned:

The argument "offset" is shifted right by PAGE_SHIFT bits in sys_mmap(mmap systemcall).

------------------------------------------------------------------------
sys_mmap(unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long off)

{
	error = sys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
}

------------------------------------------------------------------------

In sys_mmap_pgoff(addr, len, prot, flags, fd, pgoff), do_mmap_pgoff() is called as follows:

------------------------------------------------------------------------
sys_mmap_pgoff(unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flags, unsigned long fd, unsigned long pgoff)

{
	retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
}

------------------------------------------------------------------------

In do_mmap_pgoff(file, addr, len, prot, flags, pgoff), the code path which returns with the error EOVERFLOW exists already.

------------------------------------------------------------------------
do_mmap_pgoff(struct file *file, unsigned long addr,
unsigned long len, unsigned long prot, unsigned long flags, unsigned long pgoff)

{
	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
		return -EOVERFLOW;
}

------------------------------------------------------------------------

However, in this case, giving off=0xfffffffffffff000 and len=0xfffffffffffff000 on x86_64 arch, EOVERFLOW is not
returned. It is because the argument, "off" and "len" are shifted right by PAGE_SHIFT bits and thus the condition "(pgoff + (len >> PAGE_SHIFT)) < pgoff" never becomes true.

To fix this bug, it is necessary to compare "off" plus "len" with "off" by units of "off_t". The patch is here:

Signed-off-by Naotaka Hamaguchi <[email protected]>

---
 mm/mmap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

mm/mmap.c +2 -1

index eae90af..e74e736 100644
@@ -948,6 +948,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
        vm_flags_t vm_flags;
        int error;
        unsigned long reqprot = prot;
+       off_t off = pgoff << PAGE_SHIFT;
 
        /*
         * Does the application expect PROT_READ to imply PROT_EXEC?

@@ -971,7 +972,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
                return -ENOMEM;
 
        /* offset overflow? */
-       if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
+       if ((off + len) < off)
                return -EOVERFLOW;
 
        /* Too many mappings? */

Signature · 9 lines

1.7.7.4
---

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KOSAKI Motohiro

2011-12-22 16:58 UTC #1 lore raw

Quoted 21 lines

To fix this bug, it is necessary to compare "off" plus "len" with "off" by units of "off_t". The patch is here:

Signed-off-by Naotaka Hamaguchi <[email protected]>

---
mm/mmap.c | 3 ++-

 1 files changed, 2 insertions(+), 1 deletions(-)

mm/mmap.c +1 -0

index eae90af..e74e736 100644
@@ -948,6 +948,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
        vm_flags_t vm_flags;
        int error;
        unsigned long reqprot = prot;
+       off_t off = pgoff << PAGE_SHIFT;
 
        /*
         * Does the application expect PROT_READ to imply PROT_EXEC?

mm/mmap.c unsigned long do_mmap_pgoff(struct file *file, unsigned long addr, · line 976

Quoted 6 lines

mm/mmap.c +1 -1

@@ -971,7 +972,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
                return -ENOMEM;
 
        /* offset overflow? */
-       if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
+       if ((off + len) < off)
                return -EOVERFLOW;

Hmm...
pgoff doesn't make actual overflow. do_mmap_pgoff() can calculate big value. We have
no reason to make artificial limit. Why don't you meke a overflow check in sys_mmap()?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KOSAKI Motohiro

2011-12-22 17:41 UTC #2 lore raw

In reply to Naotaka Hamaguchi

Quoted 11 lines

The argument "offset" is shifted right by PAGE_SHIFT bits in sys_mmap(mmap systemcall).
------------------------------------------------------------------------
sys_mmap(unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long off)
{
	error = sys_mmap_pgoff(addr, len, prot, flags, fd, off>>  PAGE_SHIFT);
}
------------------------------------------------------------------------

Hm.
Which version are you looking at? Current code seems to don't have sys_mmap().

Naotaka Hamaguchi

author

2011-12-27 06:21 UTC #3 lore raw

Hi, Kosaki-san

Quoted 2 lines

Which version are you looking at? Current code seems to don't have sys_mmap().

This sys_mmap() means the entrance of mmap system call for x86_64.

----------------------------------------------------------------------
arch/x86/kernel/sys_x86_64.c:
84 SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
85 unsigned long, prot, unsigned long, flags,
86 unsigned long, fd, unsigned long, off)
87 {

  88         long error;
  89         error = -EINVAL;
  90         if (off & ~PAGE_MASK)

91 goto out;
92
93 error = sys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
94 out:
95 return error;
96 }
----------------------------------------------------------------------

This function calls sys_mmap_pgoff, which has the argument "off >> PAGE_SHIFT". It means that sys_mmap_pgoff does not use off, which is the argument of sys_mmap, with no change, but uses the value obtained after off is shifted right by PAGE_SHIFT bits.

In mmap system call for x86, the following sys_mmap_pgoff is the entrance in kernel.

----------------------------------------------------------------------
arch/x86/kernel/syscall_table_32.S:
...
194 .long sys_mmap_pgoff
...

mm/mmap.c:
1080 SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
1081 unsigned long, prot, unsigned long, flags,
1082 unsigned long, fd, unsigned long, pgoff)
...

1111         down_write(&current->mm->mmap_sem);
1112         retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
1113         up_write(&current->mm->mmap_sem);

----------------------------------------------------------------------

Quoted 3 lines

value. We have
no reason to make artificial limit. Why don't you meke a overflow check in sys_mmap()?

I consider it is better to make an overflow check in do_mmap_pgoff. There are two reasons:

1. If we make an overflow check in the entrance of system call, we have to check in sys_mmap for x86_64 and in sys_mmap_pgoff for x86. It means that we have to check for each architecture individually. Therefore, it is more effective to make an overflow check in do_mmap_pgoff because both sys_mmap and sys_mmap_pgoff call do_mmap_pgoff.

2. Because the argument "offset" of sys_mmap is a multiple of the page size(otherwise, EINVAL is returned.), no information is lost after shifting right by PAGE_SHIFT bits. Therefore to make an overflow check in do_mmap_pgoff is equivalent to check in sys_mmap.

Best Regards,
Naotaka Hamaguchi

Quoted 12 lines · KOSAKI Motohiro

Quoted 11 lines
The argument "offset" is shifted right by PAGE_SHIFT bits in sys_mmap(mmap systemcall).
------------------------------------------------------------------------
sys_mmap(unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long off)
{
	error = sys_mmap_pgoff(addr, len, prot, flags, fd, off>>   PAGE_SHIFT);
}
------------------------------------------------------------------------

Quoted 8 lines

Hm.
Which version are you looking at? Current code seems to don't have sys_mmap().

KOSAKI Motohiro

2011-12-28 02:04 UTC #4 lore raw

Quoted 23 lines

----------------------------------------------------------------------
arch/x86/kernel/syscall_table_32.S:
...
194 .long sys_mmap_pgoff
...
mm/mmap.c:
1080 SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len, 1081 unsigned long, prot, unsigned long, flags, 1082 unsigned long, fd, unsigned long, pgoff)
...
1111 down_write(&current->mm->mmap_sem);
1112 retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff); 1113 up_write(&current->mm->mmap_sem);
----------------------------------------------------------------------
Quoted 3 lines
value. We have
no reason to make artificial limit. Why don't you meke a overflow check in sys_mmap()?
I consider it is better to make an overflow check in do_mmap_pgoff. There are two reasons:

Quoted 6 lines

1. If we make an overflow check in the entrance of system call, we have to check in sys_mmap for x86_64 and in sys_mmap_pgoff for x86. It means that we have to check for each architecture individually. Therefore, it is more effective to make an overflow check in do_mmap_pgoff because both sys_mmap and sys_mmap_pgoff call do_mmap_pgoff.

arch/x86/include/asm/posix_types_32.h
---------------------------------------------
typedef long __kernel_off_t;

So, your patch introduce 2GB limitation to 32bit arch. It makes no sense.

Quoted 5 lines

2. Because the argument "offset" of sys_mmap is a multiple of the page size(otherwise, EINVAL is returned.), no information is lost after shifting right by PAGE_SHIFT bits. Therefore to make an overflow check in do_mmap_pgoff is equivalent to check in sys_mmap.