[RFC/PATCH mmap2: better determine overflow

[RFC/PATCH mmap2: better determine overflow

[Randy Dunlap, Randy Dunlap]

mm/mmap.c::do_mmap_pgoff() checks for overflow like:

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

However, using pgoff (page indexes) to determine address range
overflow doesn't overflow.  Change to use byte offsets instead,
so that overflow can actually happen and be noticed.
Also return EOVERFLOW instead of ENOMEM when PAGE_ALIGN(len)
is 0.

Tested on i686 and x86_64.

Test program is at:  http://www.xenotime.net/linux/src/mmap-test.c

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---
 mm/mmap.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--- linux-2618-work.orig/mm/mmap.c
+++ linux-2618-work/mm/mmap.c
@@ -923,13 +923,16 @@ unsigned long do_mmap_pgoff(struct file 
 
 	/* Careful about overflows.. */
 	len = PAGE_ALIGN(len);
-	if (!len || len > TASK_SIZE)
-		return -ENOMEM;
+	if (!len)
+		return -EOVERFLOW;
 
 	/* offset overflow? */
-	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
+	if (((pgoff << PAGE_SHIFT) + len) < (pgoff << PAGE_SHIFT))
                return -EOVERFLOW;
 
+	if (len > TASK_SIZE)
+		return -ENOMEM;
+
 	/* Too many mappings? */
 	if (mm->map_count > sysctl_max_map_count)
 		return -ENOMEM;

---

--
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: [RFC/PATCH mmap2: better determine overflow

[Hugh Dickins]

On Tue, 26 Sep 2006, Randy Dunlap wrote:
> From: Randy Dunlap <rdunlap@xenotime.net>
> 
> mm/mmap.c::do_mmap_pgoff() checks for overflow like:
> 
> 	/* offset overflow? */
> 	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
>                return -EOVERFLOW;
> 
> However, using pgoff (page indexes) to determine address range
> overflow doesn't overflow.  Change to use byte offsets instead,
> so that overflow can actually happen and be noticed.

I think you're mistaken there.  Thinking in particular of 32-bit
arches, isn't the check precisely about allowing an mmap at a high
offset of a file >4GB in length; but not at so high an offset that
pgoff (page index) wraps back to 0?  Whereas you're changing it
now to fail at 4GB.

> Also return EOVERFLOW instead of ENOMEM when PAGE_ALIGN(len)
> is 0.

Which standard mandates that change?

Hugh

> 
> Tested on i686 and x86_64.
> 
> Test program is at:  http://www.xenotime.net/linux/src/mmap-test.c
> 
> Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
> ---
>  mm/mmap.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> --- linux-2618-work.orig/mm/mmap.c
> +++ linux-2618-work/mm/mmap.c
> @@ -923,13 +923,16 @@ unsigned long do_mmap_pgoff(struct file 
>  
>  	/* Careful about overflows.. */
>  	len = PAGE_ALIGN(len);
> -	if (!len || len > TASK_SIZE)
> -		return -ENOMEM;
> +	if (!len)
> +		return -EOVERFLOW;
>  
>  	/* offset overflow? */
> -	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
> +	if (((pgoff << PAGE_SHIFT) + len) < (pgoff << PAGE_SHIFT))
>                 return -EOVERFLOW;
>  
> +	if (len > TASK_SIZE)
> +		return -ENOMEM;
> +
>  	/* Too many mappings? */
>  	if (mm->map_count > sysctl_max_map_count)
>  		return -ENOMEM;
> 
> ---

--
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: [RFC/PATCH mmap2: better determine overflow

[Randy Dunlap]

On Tue, 26 Sep 2006 19:10:41 +0100 (BST) Hugh Dickins wrote:

> On Tue, 26 Sep 2006, Randy Dunlap wrote:
> > From: Randy Dunlap <rdunlap@xenotime.net>
> > 
> > mm/mmap.c::do_mmap_pgoff() checks for overflow like:
> > 
> > 	/* offset overflow? */
> > 	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
> >                return -EOVERFLOW;
> > 
> > However, using pgoff (page indexes) to determine address range
> > overflow doesn't overflow.  Change to use byte offsets instead,
> > so that overflow can actually happen and be noticed.
> 
> I think you're mistaken there.  Thinking in particular of 32-bit
> arches, isn't the check precisely about allowing an mmap at a high
> offset of a file >4GB in length; but not at so high an offset that
> pgoff (page index) wraps back to 0?  Whereas you're changing it
> now to fail at 4GB.

OK, I think I see.  I'll check/test/verify more.

> > Also return EOVERFLOW instead of ENOMEM when PAGE_ALIGN(len)
> > is 0.
> 
> Which standard mandates that change?

It was an interpretation.  Perhaps a mis-interpretation.
This comes after:

	if (!len)
		return -EINVAL;
...then

 	len = PAGE_ALIGN(len);
b:
-	if (!len || len > TASK_SIZE)
-		return -ENOMEM;
+	if (!len)
+		return -EOVERFLOW;

so if len is 0 at b:, then it was a very large unsigned long number
(larger than 0 - PAGE_SIZE, i.e., >= 0xfffff001 on 32-bit or
>= 0xffffffff_fffff001 on 64-bit), and PAGE_ALIGN() rounded it "up"
to 0.  That seems more like an overflow than a NOMEM to me.
That's all.


So, I'm interested in the EOVERFLOW case(s).
Would you attempt to translate this return value case for me?
(from http://www.opengroup.org/onlinepubs/009695399/functions/mmap.html:)

[EOVERFLOW]
    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.

I'm not concerned about the "off plus len" since I am looking at
mmap2() [using pgoff's instead].  I'm more concerned about the
"offset maximum established in the open file description associated
with fildes."

Does mmap2() on Linux use the actual filesize as a limit for the
mmap() area [not that I can see] or does it just use (effectively)
ULONG_MAX, without regard file actual filesize?

Thanks for looking/helping.

> Hugh
> 
> > 
> > Tested on i686 and x86_64.
> > 
> > Test program is at:  http://www.xenotime.net/linux/src/mmap-test.c
> > 
> > Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
> > ---
> >  mm/mmap.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > --- linux-2618-work.orig/mm/mmap.c
> > +++ linux-2618-work/mm/mmap.c
> > @@ -923,13 +923,16 @@ unsigned long do_mmap_pgoff(struct file 
> >  
> >  	/* Careful about overflows.. */
> >  	len = PAGE_ALIGN(len);
> > -	if (!len || len > TASK_SIZE)
> > -		return -ENOMEM;
> > +	if (!len)
> > +		return -EOVERFLOW;
> >  
> >  	/* offset overflow? */
> > -	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
> > +	if (((pgoff << PAGE_SHIFT) + len) < (pgoff << PAGE_SHIFT))
> >                 return -EOVERFLOW;
> >  
> > +	if (len > TASK_SIZE)
> > +		return -ENOMEM;
> > +
> >  	/* Too many mappings? */
> >  	if (mm->map_count > sysctl_max_map_count)
> >  		return -ENOMEM;
> > 
> > ---

---
~Randy

--
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: [RFC/PATCH mmap2: better determine overflow

[Hugh Dickins]

On Tue, 26 Sep 2006, Randy Dunlap wrote:
> 
> It was an interpretation.  Perhaps a mis-interpretation.
> This comes after:
> 
> 	if (!len)
> 		return -EINVAL;
> ...then
> 
>  	len = PAGE_ALIGN(len);
> b:
> -	if (!len || len > TASK_SIZE)
> -		return -ENOMEM;
> +	if (!len)
> +		return -EOVERFLOW;
> 
> so if len is 0 at b:, then it was a very large unsigned long number
> (larger than 0 - PAGE_SIZE, i.e., >= 0xfffff001 on 32-bit or
> >= 0xffffffff_fffff001 on 64-bit), and PAGE_ALIGN() rounded it "up"
> to 0.  That seems more like an overflow than a NOMEM to me.
> That's all.

I agree that len 0 at that point arises from an extremely big len
specified by the user: it's just another case of len > TASK_SIZE
that the preceding PAGE_ALIGN has now disguised as len 0.  And
the errno for "there is insufficient room in the address space
to effect the mapping" is said to be ENOMEM.  That should stay.

> 
> So, I'm interested in the EOVERFLOW case(s).
> Would you attempt to translate this return value case for me?
> (from http://www.opengroup.org/onlinepubs/009695399/functions/mmap.html:)
> 
> [EOVERFLOW]
>     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.
> 
> I'm not concerned about the "off plus len" since I am looking at
> mmap2() [using pgoff's instead].  I'm more concerned about the
> "offset maximum established in the open file description associated
> with fildes."

I suspect it means that on a 32-bit system, if the file was not opened
with O_LARGEFILE, off-plus-len needs to stay within 2GB.  Whereas on a
64-bit system, or when opened with O_LARGEFILE, off-plus-len needs to
stay within the max the filesystem and VFS can support.  We're enforcing
the latter, without regard to whether or not it was opened with
O_LARGEFILE.  Change that?  I doubt it's worth the possibility
of now breaking userspace.

> 
> Does mmap2() on Linux use the actual filesize as a limit for the
> mmap() area [not that I can see]

That's right, it does not (and would be wrong to do so:
the file can be extended or truncated while it's mapped).

> or does it just use (effectively)
> ULONG_MAX, without regard file actual filesize?

I'd say that limit is TASK_SIZE rather than ULONG_MAX.

Hugh

--
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: [RFC/PATCH mmap2: better determine overflow

[Randy Dunlap]

On Tue, 26 Sep 2006 21:44:09 +0100 (BST) Hugh Dickins wrote:

> On Tue, 26 Sep 2006, Randy Dunlap wrote:
> > 
> > It was an interpretation.  Perhaps a mis-interpretation.
> > This comes after:
> > 
> > 	if (!len)
> > 		return -EINVAL;
> > ...then
> > 
> >  	len = PAGE_ALIGN(len);
> > b:
> > -	if (!len || len > TASK_SIZE)
> > -		return -ENOMEM;
> > +	if (!len)
> > +		return -EOVERFLOW;
> > 
> > so if len is 0 at b:, then it was a very large unsigned long number
> > (larger than 0 - PAGE_SIZE, i.e., >= 0xfffff001 on 32-bit or
> > >= 0xffffffff_fffff001 on 64-bit), and PAGE_ALIGN() rounded it "up"
> > to 0.  That seems more like an overflow than a NOMEM to me.
> > That's all.
> 
> I agree that len 0 at that point arises from an extremely big len
> specified by the user: it's just another case of len > TASK_SIZE
> that the preceding PAGE_ALIGN has now disguised as len 0.  And
> the errno for "there is insufficient room in the address space
> to effect the mapping" is said to be ENOMEM.  That should stay.

I see.

> > So, I'm interested in the EOVERFLOW case(s).
> > Would you attempt to translate this return value case for me?
> > (from http://www.opengroup.org/onlinepubs/009695399/functions/mmap.html:)
> > 
> > [EOVERFLOW]
> >     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.
> > 
> > I'm not concerned about the "off plus len" since I am looking at
> > mmap2() [using pgoff's instead].  I'm more concerned about the
> > "offset maximum established in the open file description associated
> > with fildes."
> 
> I suspect it means that on a 32-bit system, if the file was not opened
> with O_LARGEFILE, off-plus-len needs to stay within 2GB.  Whereas on a
> 64-bit system, or when opened with O_LARGEFILE, off-plus-len needs to
> stay within the max the filesystem and VFS can support.  We're enforcing
> the latter, without regard to whether or not it was opened with
> O_LARGEFILE.  Change that?  I doubt it's worth the possibility
> of now breaking userspace.

Agreed.

> > Does mmap2() on Linux use the actual filesize as a limit for the
> > mmap() area [not that I can see]
> 
> That's right, it does not (and would be wrong to do so:
> the file can be extended or truncated while it's mapped).
> 
> > or does it just use (effectively)
> > ULONG_MAX, without regard file actual filesize?
> 
> I'd say that limit is TASK_SIZE rather than ULONG_MAX.

Right.

Thanks for all of your helpful comments.

---
~Randy

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