mm: mmap() sometimes succeeds even if the region to map is invalid.

PATCH In review Naotaka Hamaguchi 3 participants 5 messages Feb 17, 2012 – Feb 21, 2012

Subsystems (3)

Memory management x86 Memory mapping

lore

Share & tools

lore

Naotaka Hamaguchi

author

2012-02-17 08:42 UTC #0 lore raw

PATCH 1/1 mm: mmap() sometimes succeeds even if the region to map is invalid.

This patch fixes two bugs of mmap():
1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case. Currently I have only checked it on x86_64 because (a) x86 seems to OK to accept a negative offset for mapping 2GB-4GB regions, and (b) I don't know about other architectures at all (I'll make it if needed).

2. mmap() would succeed if "offset" + "length" get overflow, although it should return EOVERFLOW.

The detail of these problems is as follows:

1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case.

POSIX says the type of the argument "off" is "off_t", which is equivalent to "long" for all architecture, so it is allowed to give a negative "off" to mmap().

In such case, it is actually regarded as big positive value because the type of "off" is "unsigned long" in the kernel. For example, off=-4096 (-0x1000) is regarded as
off = 0xfffffffffffff000 (x86_64) and as off = 0xfffff000 (x86). It results in mapping too big offset region.

2. mmap() would succeed if "offset" + "length" get overflow, although it should return EOVERFLOW.

The overflow check of mmap() almost doesn't work.

In do_mmap_pgoff(file, addr, len, prot, flags, pgoff), the existing overflow check logic is as follows.

------------------------------------------------------------------------
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, for example on x86_64, if we give off=0x1000 and len=0xfffffffffffff000, but EOVERFLOW is not returned. It is because the checking is based on the page offset, not on the byte offset.

To fix this bug, I convert this overflow check from page offset base to byte offset base.

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

---
 arch/x86/kernel/sys_x86_64.c |    3 +++
 mm/mmap.c                    |    3 ++-
 2 files changed, 5 insertions(+), 1 deletions(-)

arch/x86/kernel/sys_x86_64.c +3 -0

index 0514890..ddefd6c 100644
@@ -90,6 +90,9 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
        if (off & ~PAGE_MASK)
                goto out;
 
+       if ((off_t) off < 0)
+               goto out;
+
        error = sys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
 out:
        return error;

mm/mmap.c +2 -1

index 3f758c7..2fa99cd 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;
+       unsigned long 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? */

--
1.7.7.4

Best Regards,
Naotaka Hamaguchi

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

Venu Byravarasu

2012-02-17 09:04 UTC #1 lore raw

Quoted 10 lines

The detail of these problems is as follows:
1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case.
In such case, it is actually regarded as big positive value because the type of "off" is "unsigned long" in the kernel. For example, off=-4096 (-0x1000) is regarded as
off = 0xfffffffffffff000 (x86_64) and as off = 0xfffff000 (x86). It results in mapping too big offset region.

It is not true always.

Considering your example, say if page size is 4k, then PAGE_MASK = 0xFFF hence (off & ~PAGE_MASK) will be true and " -EINVAL" will be returned.

Naotaka Hamaguchi

author

2012-02-20 07:23 UTC #2 lore raw

Hi Venu,

Quoted 9 lines

Quoted 8 lines
1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case.
In such case, it is actually regarded as big positive value because the type of "off" is "unsigned long" in the kernel. For example, off=-4096 (-0x1000) is regarded as
off = 0xfffffffffffff000 (x86_64) and as off = 0xfffff000 (x86). It results in mapping too big offset region.

Quoted 4 lines

It is not true always.
Considering your example, say if page size is 4k, then PAGE_MASK = 0xFFF hence (off& ~PAGE_MASK) will be true and " -EINVAL" will be returned.

Is PAGE_MASK 0xfffffffffffff000 (x86_64) and 0xfffff000 (x86), isn't it? Or am I missing something?

arch/x86/include/asm/page_types.h
=================================================
...

#define PAGE_SHIFT      12
#define PAGE_SIZE       (_AC(1,UL) << PAGE_SHIFT)
#define PAGE_MASK       (~(PAGE_SIZE-1))

...
=================================================

Thanks,
Naotaka Hamaguchi

Quoted 13 lines · Venu Byravarasu

Quoted 10 lines
The detail of these problems is as follows:
1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case.
In such case, it is actually regarded as big positive value because the type of "off" is "unsigned long" in the kernel. For example, off=-4096 (-0x1000) is regarded as
off = 0xfffffffffffff000 (x86_64) and as off = 0xfffff000 (x86). It results in mapping too big offset region.
It is not true always.

Quoted 9 lines

Considering your example, say if page size is 4k, then PAGE_MASK = 0xFFF hence (off& ~PAGE_MASK) will be true and " -EINVAL" will be returned. --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hugh Dickins

2012-02-18 02:01 UTC #3 lore raw

In reply to Naotaka Hamaguchi

Quoted 9 lines · Naotaka Hamaguchi

This patch fixes two bugs of mmap():
1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case. Currently I have only checked it on x86_64 because (a) x86 seems to OK to accept a negative offset for mapping 2GB-4GB regions, and (b) I don't know about other architectures at all (I'll make it if needed).
2. mmap() would succeed if "offset" + "length" get overflow, although it should return EOVERFLOW.

I'm not convinced that either of these is a problem. Do you see an actual bug arising from these, or is it just that you think the Linux mmap() permits more than you expect from your reading of POSIX?

1. Should a negative offset necessarily return -EINVAL? At present I can mmap() /dev/kmem on x86_64 and see what's at 0xffff880000000000:
why should that say -EINVAL? (I admit that my example wanted to say 0xffffffff81000000, where /proc/kallsyms locates _text, but that did disappoint me with -EINVAL, because mmap_kmem() only understands the direct map, not the further layouts which architectures may use.)

2. We will have bugs if you manage to mmap an area crossing from pgoff -1 to pgoff 0, but I thought the existing checks prevented that.

mmap() should be permitting as far as it safely can; but it's a bug if a fault on an offset beyond (page-rounded-up) end-of-file does not then give SIGBUS.

Quoted 75 lines

The detail of these problems is as follows:
1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case.
POSIX says the type of the argument "off" is "off_t", which is equivalent to "long" for all architecture, so it is allowed to give a negative "off" to mmap().
In such case, it is actually regarded as big positive value because the type of "off" is "unsigned long" in the kernel. For example, off=-4096 (-0x1000) is regarded as
off = 0xfffffffffffff000 (x86_64) and as off = 0xfffff000 (x86). It results in mapping too big offset region.
2. mmap() would succeed if "offset" + "length" get overflow, although it should return EOVERFLOW.
The overflow check of mmap() almost doesn't work.
In do_mmap_pgoff(file, addr, len, prot, flags, pgoff), the existing overflow check logic is as follows.
------------------------------------------------------------------------
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, for example on x86_64, if we give off=0x1000 and len=0xfffffffffffff000, but EOVERFLOW is not returned. It is because the checking is based on the page offset, not on the byte offset.
To fix this bug, I convert this overflow check from page offset base to byte offset base.
Signed-off-by Naotaka Hamaguchi <[email protected]>
---
 arch/x86/kernel/sys_x86_64.c |    3 +++
 mm/mmap.c                    |    3 ++-
 2 files changed, 5 insertions(+), 1 deletions(-)
arch/x86/kernel/sys_x86_64.c +3 -0
index 0514890..ddefd6c 100644
@@ -90,6 +90,9 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
        if (off & ~PAGE_MASK)
                goto out;
 
+       if ((off_t) off < 0)
+               goto out;
+
        error = sys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
 out:
        return error;
mm/mmap.c +1 -0
index 3f758c7..2fa99cd 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;
+       unsigned long 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;

I think you are taking away the 32-bit kernel's ability to mmap() files up to MAX_LFS_FILESIZE.

Hugh

Quoted 7 lines

mm/mmap.c +0 -1
        /* Too many mappings? */
--
1.7.7.4
Best Regards,
Naotaka Hamaguchi

Naotaka Hamaguchi

author

2012-02-21 08:30 UTC #4 lore raw

Hi Hugh,

Quoted 10 lines

1. Should a negative offset necessarily return -EINVAL? At present I can mmap() /dev/kmem on x86_64 and see what's at 0xffff880000000000:
why should that say -EINVAL? (I admit that my example wanted to say 0xffffffff81000000, where /proc/kallsyms locates _text, but that did disappoint me with -EINVAL, because mmap_kmem() only understands the direct map, not the further layouts which architectures may use.)
2. We will have bugs if you manage to mmap an area crossing from pgoff -1 to pgoff 0, but I thought the existing checks prevented that.

Quoted 6 lines

Review
Quoted 3 lines
(quoted diff) +1 -1
-       if ((pgoff + (len>>  PAGE_SHIFT))<  pgoff)
+       if ((off + len)<  off)
                 return -EOVERFLOW;
I think you are taking away the 32-bit kernel's ability to mmap() files up to MAX_LFS_FILESIZE.

Thanks, I see. I drop this patch.

BTW, I think the current error check of EOVERFLOW is meaningless, isn't it?

mm/mmap.c
===================================================================
unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
unsigned long len, unsigned long prot, unsigned long flags, unsigned long pgoff)
{
...

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

...
===================================================================

Thanks,
Naotaka Hamaguchi

Quoted 121 lines · Hugh Dickins

Quoted 9 lines · Naotaka Hamaguchi
This patch fixes two bugs of mmap():
1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case. Currently I have only checked it on x86_64 because (a) x86 seems to OK to accept a negative offset for mapping 2GB-4GB regions, and (b) I don't know about other architectures at all (I'll make it if needed).
2. mmap() would succeed if "offset" + "length" get overflow, although it should return EOVERFLOW.
I'm not convinced that either of these is a problem. Do you see an actual bug arising from these, or is it just that you think the Linux mmap() permits more than you expect from your reading of POSIX?
1. Should a negative offset necessarily return -EINVAL? At present I can mmap() /dev/kmem on x86_64 and see what's at 0xffff880000000000:
why should that say -EINVAL? (I admit that my example wanted to say 0xffffffff81000000, where /proc/kallsyms locates _text, but that did disappoint me with -EINVAL, because mmap_kmem() only understands the direct map, not the further layouts which architectures may use.)
2. We will have bugs if you manage to mmap an area crossing from pgoff -1 to pgoff 0, but I thought the existing checks prevented that.
mmap() should be permitting as far as it safely can; but it's a bug if a fault on an offset beyond (page-rounded-up) end-of-file does not then give SIGBUS.
Quoted 75 lines
The detail of these problems is as follows:
1. mmap() succeeds even if "offset" argument is a negative value, although it should return EINVAL in such case.
POSIX says the type of the argument "off" is "off_t", which is equivalent to "long" for all architecture, so it is allowed to give a negative "off" to mmap().
In such case, it is actually regarded as big positive value because the type of "off" is "unsigned long" in the kernel. For example, off=-4096 (-0x1000) is regarded as
off = 0xfffffffffffff000 (x86_64) and as off = 0xfffff000 (x86). It results in mapping too big offset region.
2. mmap() would succeed if "offset" + "length" get overflow, although it should return EOVERFLOW.
The overflow check of mmap() almost doesn't work.
In do_mmap_pgoff(file, addr, len, prot, flags, pgoff), the existing overflow check logic is as follows.
------------------------------------------------------------------------
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, for example on x86_64, if we give off=0x1000 and len=0xfffffffffffff000, but EOVERFLOW is not returned. It is because the checking is based on the page offset, not on the byte offset.
To fix this bug, I convert this overflow check from page offset base to byte offset base.
Signed-off-by Naotaka Hamaguchi<[email protected]>
---
  arch/x86/kernel/sys_x86_64.c |    3 +++
  mm/mmap.c                    |    3 ++-
  2 files changed, 5 insertions(+), 1 deletions(-)
arch/x86/kernel/sys_x86_64.c +3 -0
index 0514890..ddefd6c 100644
@@ -90,6 +90,9 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
         if (off&  ~PAGE_MASK)
                 goto out;
 
+       if ((off_t) off<  0)
+               goto out;
+
         error = sys_mmap_pgoff(addr, len, prot, flags, fd, off>>  PAGE_SHIFT);
  out:
         return error;
mm/mmap.c +1 -0
index 3f758c7..2fa99cd 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;
+       unsigned long 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;
I think you are taking away the 32-bit kernel's ability to mmap() files up to MAX_LFS_FILESIZE.
Hugh
Quoted 5 lines
mm/mmap.c +0 -1
         /* Too many mappings? */
--
1.7.7.4

Quoted 9 lines

Quoted 2 lines
Best Regards,
Naotaka Hamaguchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/