[PATCH] Fix overflow in vma length when copying mmap on clone

[PATCH] Fix overflow in vma length when copying mmap on clone

[Siddhesh Poyarekar]

The vma length in dup_mmap is calculated and stored in a unsigned int,
which is insufficient and hence overflows for very large maps (beyond
16TB). The following program demonstrates this:

\#include <stdio.h>
\#include <unistd.h>
\#include <sys/mman.h>

\#define GIG 1024 * 1024 * 1024L
\#define EXTENT 16393

int main(void)
{
        int i, r;
        void *m;
        char buf[1024];

        for (i = 0; i < EXTENT; i++) {
                m = mmap(NULL, (size_t) 1 * 1024 * 1024 * 1024L,
                         PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);

                if (m == (void *)-1)
                        printf("MMAP Failed: %d\n", m);
                else
                        printf("%d : MMAP returned %p\n", i, m);

                r = fork();

                if (r == 0) {
                        printf("%d: successed\n", i);
                        return 0;
                } else if (r < 0)
                        printf("FORK Failed: %d\n", r);
                else if (r > 0)
                        wait(NULL);
        }
        return 0;
}

This trivial patch increases the storage size of the result to
unsigned long, which should be sufficient for storing the difference
between addresses.

Signed-off-by: Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>
---
 kernel/fork.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index b9372a0..7acaee1 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -355,7 +355,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
 		}
 		charge = 0;
 		if (mpnt->vm_flags & VM_ACCOUNT) {
-			unsigned int len = (mpnt->vm_end - mpnt->vm_start) >> PAGE_SHIFT;
+			unsigned long len;
+			len = (mpnt->vm_end - mpnt->vm_start) >> PAGE_SHIFT;
 			if (security_vm_enough_memory_mm(oldmm, len)) /* sic */
 				goto fail_nomem;
 			charge = len;
-- 
1.7.7.6

Re: [PATCH] Fix overflow in vma length when copying mmap on clone

[Hugh Dickins]

On Tue, 24 Apr 2012, Siddhesh Poyarekar wrote:
> The vma length in dup_mmap is calculated and stored in a unsigned int,
> which is insufficient and hence overflows for very large maps (beyond
> 16TB). The following program demonstrates this:
> 
> \#include <stdio.h>
> \#include <unistd.h>
> \#include <sys/mman.h>
> 
> \#define GIG 1024 * 1024 * 1024L
> \#define EXTENT 16393
> 
> int main(void)
> {
>         int i, r;
>         void *m;
>         char buf[1024];
> 
>         for (i = 0; i < EXTENT; i++) {
>                 m = mmap(NULL, (size_t) 1 * 1024 * 1024 * 1024L,
>                          PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> 
>                 if (m == (void *)-1)
>                         printf("MMAP Failed: %d\n", m);
>                 else
>                         printf("%d : MMAP returned %p\n", i, m);
> 
>                 r = fork();
> 
>                 if (r == 0) {
>                         printf("%d: successed\n", i);
>                         return 0;
>                 } else if (r < 0)
>                         printf("FORK Failed: %d\n", r);
>                 else if (r > 0)
>                         wait(NULL);
>         }
>         return 0;
> }
> 
> This trivial patch increases the storage size of the result to
> unsigned long, which should be sufficient for storing the difference
> between addresses.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>

Good catch, thank you, remarkable that's survived for so long.
For the patch,

Acked-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org

But I didn't (try very hard to) work out what your demo program shows
- though I am amused by your sense of humour in using %d for a pointer
there!  I wonder what setting of /proc/sys/vm/overcommit_memory is
needed for it to behave as you intend?

Personally, I wouldn't bother with the demo and describing it more fully,
I'd just be glad to get that fix in at last.

> ---
>  kernel/fork.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b9372a0..7acaee1 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -355,7 +355,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  		}
>  		charge = 0;
>  		if (mpnt->vm_flags & VM_ACCOUNT) {
> -			unsigned int len = (mpnt->vm_end - mpnt->vm_start) >> PAGE_SHIFT;
> +			unsigned long len;
> +			len = (mpnt->vm_end - mpnt->vm_start) >> PAGE_SHIFT;
>  			if (security_vm_enough_memory_mm(oldmm, len)) /* sic */
>  				goto fail_nomem;
>  			charge = len;
> -- 
> 1.7.7.6

Re: [PATCH] Fix overflow in vma length when copying mmap on clone

[Siddhesh Poyarekar]

On 25 April 2012 00:31, Hugh Dickins <hughd@google.com> wrote:
> But I didn't (try very hard to) work out what your demo program shows
> - though I am amused by your sense of humour in using %d for a pointer
> there!  I wonder what setting of /proc/sys/vm/overcommit_memory is
> needed for it to behave as you intend?

That was supposed to be errno, not the pointer. I had added my own
syscall wrappers to eliminate glibc and then reverted it to the
original smaller-to-read reproducer and this got left behind in the
process. The demo program is supposed to show "successed" for
iterations 16383 to 16390 since the overflow happens at 16TB. All
iterations before it (and after) show a fork failure.

/proc/sys/vm/overcommit_memory is 0.

Perhaps a cleaner demo program would have been:

#include <stdio.h>
#include <unistd.h>
#include <sys/mman.h>
#include <errno.h>

#define GIG 1024 * 1024 * 1024L
#define EXTENT 16393

int main(void)
{
        int i, r;
        void *m;
        char buf[1024];
        int prev_failed = 0;

        for (i = 0; i < EXTENT; i++) {
                m = mmap(NULL, (size_t) 1 * 1024 * 1024 * 1024L,
                         PROT_READ | PROT_WRITE, MAP_PRIVATE |
MAP_ANONYMOUS, 0, 0);

                if (m == (void *)-1) {
                        printf("MMAP Failed: %d\n", errno);
                        return 1;
                }

                r = fork();

                if (r == 0) {
                        return 0;
                } else if (r < 0) {
                        prev_failed = 1;
                        /* Fork failed as expected */
                }
                else if (r > 0 && prev_failed) {
                        printf("Unexpected success at %d\n", i);
                        wait(NULL);
                        return 1;
                }
        }
        return 0;
}


-- 
Siddhesh Poyarekar
http://siddhesh.in

Re: [PATCH] Fix overflow in vma length when copying mmap on clone

[Siddhesh Poyarekar]

On 25 April 2012 01:10, Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com> wrote:
> That was supposed to be errno, not the pointer. I had added my own
> syscall wrappers to eliminate glibc and then reverted it to the
> original smaller-to-read reproducer and this got left behind in the
> process. The demo program is supposed to show "successed" for
> iterations 16383 to 16390 since the overflow happens at 16TB. All
> iterations before it (and after) show a fork failure.
>
> /proc/sys/vm/overcommit_memory is 0.
>
> Perhaps a cleaner demo program would have been:

Ugh, I missed some details once again. The demo below will show
"Unexpected success" without this patch in place. The system I've
tested this patch on is an x86_64 F-16 box with 4GB RAM and 6GB swap.

> #include <stdio.h>
> #include <unistd.h>
> #include <sys/mman.h>
> #include <errno.h>
>
> #define GIG 1024 * 1024 * 1024L
> #define EXTENT 16393
>
> int main(void)
> {
>        int i, r;
>        void *m;
>        char buf[1024];
>        int prev_failed = 0;
>
>        for (i = 0; i < EXTENT; i++) {
>                m = mmap(NULL, (size_t) 1 * 1024 * 1024 * 1024L,
>                         PROT_READ | PROT_WRITE, MAP_PRIVATE |
> MAP_ANONYMOUS, 0, 0);
>
>                if (m == (void *)-1) {
>                        printf("MMAP Failed: %d\n", errno);
>                        return 1;
>                }
>
>                r = fork();
>
>                if (r == 0) {
>                        return 0;
>                } else if (r < 0) {
>                        prev_failed = 1;
>                        /* Fork failed as expected */
>                }
>                else if (r > 0 && prev_failed) {
>                        printf("Unexpected success at %d\n", i);
>                        wait(NULL);
>                        return 1;
>                }
>        }
>        return 0;
> }
>



-- 
Siddhesh Poyarekar
http://siddhesh.in