mmap() doesn't like certain value...

mmap() doesn't like certain value...

[DervishD]

    Hello all :))

    While writing a test program I used mmap using SIZE_MAX (which is
the maximum value storeable in a size_t variable) as the length, just
to see when mmap starts failing with EINVAL or ENOMEM.

    Well, mmap() acted quite good and reasonable and gave me the
values I was searching for... except when a value of SIZE_MAX-4095 or
higher is passed to it.

    I've taken a look at the kernel sources and in the file
mm/mmap.c, in the function do_mmap_pgoff, the length supplied is page
aligned (thru the macro PAGE_ALIGN). But this macro correctly says
that when we are at address SIZE_MAX-4095 or higher the next page in
the addressable space is the page at address 0. But we are dealing
with *sizes*, not addresses.

    The matter is that mmap() doesn't fail with values of
SIZE_MAX-4095 and higher (as it should do), but succeeds returning
'0' as the address... This is due the calculus that PAGE_ALIGN does
with the enormous length passed (namely 4294963200 or higher, up to
the limit marked by SIZE_MAX: 2^32-1). This macro cannot be used with
numbers near to the limit. mmap() should return -1 and set errno to
EINVAL, as properly does when the enormous length is less than
2^32-4096.

    I know: this lengths are enormous, nobody uses them, etc... but I
think that mmap shouldn't behave as bad just because nobody will use
the entire domain of the function. If the length domain is [0, 2^32)
the function should behave correctly, returning errors as
appropriate, not succeeding falsely. So please consider correcting
the problem (should suffice with eliminating the use of PAGE_ALIGN or
adding special cases to the test prior to its use).

    If this is not a bug, but an intended behaviour please excuse me.
Moreover, I can provide a patch (I suppose) against the 2.4.18 tree.

    Thanks a lot for reading this :)
    Raúl

Re: mmap() doesn't like certain value...

[DervishD]

    Hi Denis :)

    Thanks for answering :)

>        if ((len = PAGE_ALIGN(len)) == 0)
>                return addr;

    This is the problem.

>        if (len > TASK_SIZE)
>                return -EINVAL;

    And is corrected just by inverting the two quoted code snips :)

    If we test for len being greater than TASK_SIZE *before* aligning
it then the function works properly (the alignment will be done
properly in the task address space). No patch needed really ;)

>>     I know: this lengths are enormous, nobody uses them, etc... but I
>Looks like you found an obscure corner case. Good!

    I'll give a try to the inversion, that should work. I have
written a small stress program for mmap, so in a few hours the patch
will be ready. Must I post it here or send it directly to Marcello?

>>     If this is not a bug, but an intended behaviour please excuse me.
>> Moreover, I can provide a patch (I suppose) against the 2.4.18 tree.
>Do it.

    Ok :) I'll prepare the patch right now. Unified diff?

>BTW, does anybody know why len==0 is not flagged as error?

    Well, I suppose that if you request 0 bytes, any address returned
is valid, since it cannot be accessed (SIGSEGV...). Anyway the man
page for mmap() clearly says that the address returned is NEVER 0, so
we must test for len==0 and invert the other tests.

    Thanks a lot for answering, I was supposing that anybody on the
list was ignoring this obvious and easy to correct bug :?

    Raúl

Re: mmap() doesn't like certain value...

[Denis Vlasenko]

On 8 May 2002 18:00, DervishD wrote:
>     While writing a test program I used mmap using SIZE_MAX (which is
> the maximum value storeable in a size_t variable) as the length, just
> to see when mmap starts failing with EINVAL or ENOMEM.
>
>     Well, mmap() acted quite good and reasonable and gave me the
> values I was searching for... except when a value of SIZE_MAX-4095 or
> higher is passed to it.
>
>     I've taken a look at the kernel sources and in the file
> mm/mmap.c, in the function do_mmap_pgoff, the length supplied is page
> aligned (thru the macro PAGE_ALIGN). But this macro correctly says
> that when we are at address SIZE_MAX-4095 or higher the next page in
> the addressable space is the page at address 0. But we are dealing
> with *sizes*, not addresses.

unsigned long do_mmap_pgoff(struct file * file, unsigned long addr, unsigned 
long len,
        unsigned long prot, unsigned long flags, unsigned long pgoff)
{
        [local var defs snipped]
        if (file && (!file->f_op || !file->f_op->mmap))
                return -ENODEV;
        if ((len = PAGE_ALIGN(len)) == 0)
                return addr;
        if (len > TASK_SIZE)
                return -EINVAL;
        /* offset overflow? */
        if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
                return -EINVAL;
...

>     The matter is that mmap() doesn't fail with values of
> SIZE_MAX-4095 and higher (as it should do), but succeeds returning
> '0' as the address... This is due the calculus that PAGE_ALIGN does
> with the enormous length passed (namely 4294963200 or higher, up to
> the limit marked by SIZE_MAX: 2^32-1). This macro cannot be used with
> numbers near to the limit. mmap() should return -1 and set errno to
> EINVAL, as properly does when the enormous length is less than
> 2^32-4096.

So,

-	if ((len = PAGE_ALIGN(len)) == 0)
-		return addr;
-	if (len > TASK_SIZE)
+	if (!len)
+		return addr;
+	len = PAGE_ALIGN(len);
+	if (!len || len > TASK_SIZE) /* !len: address wrapped to 0 in ALIGN */

>     I know: this lengths are enormous, nobody uses them, etc... but I

Looks like you found an obscure corner case. Good!

> think that mmap shouldn't behave as bad just because nobody will use
> the entire domain of the function. If the length domain is [0, 2^32)
> the function should behave correctly, returning errors as
> appropriate, not succeeding falsely. So please consider correcting
> the problem (should suffice with eliminating the use of PAGE_ALIGN or
> adding special cases to the test prior to its use).
>
>     If this is not a bug, but an intended behaviour please excuse me.
> Moreover, I can provide a patch (I suppose) against the 2.4.18 tree.

Do it.

BTW, does anybody know why len==0 is not flagged as error?
--
vda

Re: mmap() doesn't like certain value...

[Denis Vlasenko]

On 10 May 2002 06:39, DervishD wrote:
> >        if ((len = PAGE_ALIGN(len)) == 0)
> >                return addr;
>
>     This is the problem.
>
> >        if (len > TASK_SIZE)
> >                return -EINVAL;
>
>     And is corrected just by inverting the two quoted code snips :)

You are right

>     I'll give a try to the inversion, that should work. I have
> written a small stress program for mmap, so in a few hours the patch
> will be ready. Must I post it here or send it directly to Marcello?

Post here and to Marcelo. BTW, is 2.5 affected?
--
vda

Re: mmap() doesn't like certain value...

[DervishD]

    Hi Denis and Marcelo :)

>>     And is corrected just by inverting the two quoted code snips :)
>You are right

    I've added a couple of comments and one more test. I don't think
that mmap() should return 'addr' when len=0. Moreover, mmap() cannot
return '0' under *any* circumstance (so says the man page).

    I've tested the patch with a little program to test every
possible 'len' value and checking for mmap() returning the correct
value.

    This is the patch:

--- begin ---

--- mm/mmap.c.orig	2002-05-10 10:40:51.000000000 +0200
+++ mm/mmap.c	2002-05-10 10:47:54.000000000 +0200
@@ -389,6 +389,11 @@
 	return 0;
 }
 
+
+/*
+	NOTE: in this function we rely on TASK_SIZE being lower than
+SIZE_MAX-PAGE_SIZE at least. I'm pretty sure that it is.
+*/
 unsigned long do_mmap_pgoff(struct file * file, unsigned long addr, unsigned long len,
 	unsigned long prot, unsigned long flags, unsigned long pgoff)
 {
@@ -402,12 +407,11 @@
 	if (file && (!file->f_op || !file->f_op->mmap))
 		return -ENODEV;
 
-	if ((len = PAGE_ALIGN(len)) == 0)
-		return addr;
-
-	if (len > TASK_SIZE)
+	if (!len || len > TASK_SIZE)
 		return -EINVAL;
 
+	len = PAGE_ALIGN(len);  /* This CANNOT be zero */
+
 	/* offset overflow? */
 	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
 		return -EINVAL;

--- end ---

>>     I'll give a try to the inversion, that should work. I have
>> written a small stress program for mmap, so in a few hours the patch
>> will be ready. Must I post it here or send it directly to Marcello?
>Post here and to Marcelo. BTW, is 2.5 affected?

    I don't really know, but I'm pretty sure... Anyway I think that
the same patch is valid for both kernels with little or no
modification.

    BTW, the patch is not only mine, David Gómez Espinosa
(davidge@viadomus.com) has helped me with this issue too.

    Raúl