[PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

[PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

[Vladimir Murzin]

From: Vladimir Murzin <murzin.v@gmail>

The current get_unmapped_area code calls the f_ops->get_unmapped_area or
the arch's one (via the mm) only when check for TASK_SIZE is passed. However,
generic code and some arches do the same check in their a_g_u_a implementation.

This series of patches fix the check order for TASK_SIZE in archs'
get_unmapped_area() implementations, and then removes extra check in
high-level get_unmapped_area().

Vladimir Murzin (6):
  get_unmapped_area checks for TASK_SIZE before MAP_FIXED on arm
  get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sh
  get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc32
  get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc64
  get_unmapped_area checks for TASK_SIZE before MAP_FIXED on x86_64
  get_unmapped_area remove extra check for TASK_SIZE

 arch/arm/mm/mmap.c               |    6 +++---
 arch/sh/mm/mmap.c                |   12 ++++++------
 arch/sparc/kernel/sys_sparc_32.c |    9 +++++----
 arch/sparc/kernel/sys_sparc_64.c |   16 ++++++++--------
 arch/x86/kernel/sys_x86_64.c     |    6 +++---
 mm/mmap.c                        |    4 ----
 6 files changed, 25 insertions(+), 28 deletions(-)

-- 
1.7.2.5

[PATCH 1/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on arm

[Vladimir Murzin]

Move check for TASK_SIZE before MAP_FIXED in arm's arch_get_unmapped_area().

Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
---
 arch/arm/mm/mmap.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index ce8cb19..288cc5f 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -80,6 +80,9 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	if (aliasing)
 		do_align = filp || (flags & MAP_SHARED);
 
+	if (len > TASK_SIZE)
+		return -ENOMEM;
+
 	/*
 	 * We enforce the MAP_FIXED case.
 	 */
@@ -90,9 +93,6 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
 		return addr;
 	}
 
-	if (len > TASK_SIZE)
-		return -ENOMEM;
-
 	if (addr) {
 		if (do_align)
 			addr = COLOUR_ALIGN(addr, pgoff);
-- 
1.7.2.5

[PATCH 2/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sh

[Vladimir Murzin]

Move check for TASK_SIZE before MAP_FIXED in sh's
arch_get_unmapped_area() and arch_get_unmapped_area_topdown().

Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
---
 arch/sh/mm/mmap.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/sh/mm/mmap.c b/arch/sh/mm/mmap.c
index afeb710..2c61159 100644
--- a/arch/sh/mm/mmap.c
+++ b/arch/sh/mm/mmap.c
@@ -50,6 +50,9 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	unsigned long start_addr;
 	int do_colour_align;
 
+	if (unlikely(len > TASK_SIZE))
+		return -ENOMEM;
+
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
 		 * cache aliasing constraints.
@@ -60,9 +63,6 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
 		return addr;
 	}
 
-	if (unlikely(len > TASK_SIZE))
-		return -ENOMEM;
-
 	do_colour_align = 0;
 	if (filp || (flags & MAP_SHARED))
 		do_colour_align = 1;
@@ -132,6 +132,9 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	unsigned long addr = addr0;
 	int do_colour_align;
 
+	if (unlikely(len > TASK_SIZE))
+		return -ENOMEM;
+
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
 		 * cache aliasing constraints.
@@ -142,9 +145,6 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 		return addr;
 	}
 
-	if (unlikely(len > TASK_SIZE))
-		return -ENOMEM;
-
 	do_colour_align = 0;
 	if (filp || (flags & MAP_SHARED))
 		do_colour_align = 1;
-- 
1.7.2.5

[PATCH 3/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc32

[Vladimir Murzin]

Move check for TASK_SIZE before MAP_FIXED in sparc32's arch_get_unmapped_area().

Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
---
 arch/sparc/kernel/sys_sparc_32.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/sparc/kernel/sys_sparc_32.c b/arch/sparc/kernel/sys_sparc_32.c
index 42b282f..7bf30de 100644
--- a/arch/sparc/kernel/sys_sparc_32.c
+++ b/arch/sparc/kernel/sys_sparc_32.c
@@ -40,6 +40,11 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsi
 {
 	struct vm_area_struct * vmm;
 
+	if (len > TASK_SIZE - PAGE_SIZE)
+		return -ENOMEM;
+	if (ARCH_SUN4C && len > 0x20000000)
+		return -ENOMEM;
+
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
 		 * cache aliasing constraints.
@@ -51,10 +56,6 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsi
 	}
 
 	/* See asm-sparc/uaccess.h */
-	if (len > TASK_SIZE - PAGE_SIZE)
-		return -ENOMEM;
-	if (ARCH_SUN4C && len > 0x20000000)
-		return -ENOMEM;
 	if (!addr)
 		addr = TASK_UNMAPPED_BASE;
 
-- 
1.7.2.5

Re: [PATCH 3/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc32

[Sam Ravnborg]

On Tue, May 08, 2012 at 06:40:19PM +0400, Vladimir Murzin wrote:
> Move check for TASK_SIZE before MAP_FIXED in sparc32's arch_get_unmapped_area().

I would be good to copy relevant parts of your cover letter into each patch.
when the patches are applied we looses the cover letter...


> 
> Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
> ---
>  arch/sparc/kernel/sys_sparc_32.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/sparc/kernel/sys_sparc_32.c b/arch/sparc/kernel/sys_sparc_32.c
> index 42b282f..7bf30de 100644
> --- a/arch/sparc/kernel/sys_sparc_32.c
> +++ b/arch/sparc/kernel/sys_sparc_32.c
> @@ -40,6 +40,11 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsi
>  {
>  	struct vm_area_struct * vmm;
>  
> +	if (len > TASK_SIZE - PAGE_SIZE)
> +		return -ENOMEM;
This is not the same as the check it replaces - what is going on here?

	Sam

Re: [PATCH 3/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc32

[Vladimir Murzin]

On Tue, May 08, 2012 at 06:27:22PM +0200, Sam Ravnborg wrote:
> On Tue, May 08, 2012 at 06:40:19PM +0400, Vladimir Murzin wrote:
> > Move check for TASK_SIZE before MAP_FIXED in sparc32's arch_get_unmapped_area().
> 
> I would be good to copy relevant parts of your cover letter into each patch.
> when the patches are applied we looses the cover letter...
> 

In my opinion patches are quite trivial and self-explained. However,
if there are requests to update patches in this way I'll do it in v2. 

> 
> > 
> > Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
> > ---
> >  arch/sparc/kernel/sys_sparc_32.c |    9 +++++----
> >  1 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/sparc/kernel/sys_sparc_32.c b/arch/sparc/kernel/sys_sparc_32.c
> > index 42b282f..7bf30de 100644
> > --- a/arch/sparc/kernel/sys_sparc_32.c
> > +++ b/arch/sparc/kernel/sys_sparc_32.c
> > @@ -40,6 +40,11 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsi
> >  {
> >  	struct vm_area_struct * vmm;
> >  
> > +	if (len > TASK_SIZE - PAGE_SIZE)
> > +		return -ENOMEM;
> This is not the same as the check it replaces - what is going on here?
> 

It shouldn't be the same. The a_g_u_a serves to help arches customize
get_unmapped_are depends on their specific like cache aliasing,
etc. Checking for length (in this case) could be also customized.

> 	Sam

Best wishes
Vladimir Murzin

Re: [PATCH 3/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc32

[Sam Ravnborg]

On Wed, May 09, 2012 at 12:07:08PM +0400, Vladimir Murzin wrote:
> On Tue, May 08, 2012 at 06:27:22PM +0200, Sam Ravnborg wrote:
> > On Tue, May 08, 2012 at 06:40:19PM +0400, Vladimir Murzin wrote:
> > > Move check for TASK_SIZE before MAP_FIXED in sparc32's arch_get_unmapped_area().
> > 
> > I would be good to copy relevant parts of your cover letter into each patch.
> > when the patches are applied we looses the cover letter...
> > 
> 
> In my opinion patches are quite trivial and self-explained. However,
> if there are requests to update patches in this way I'll do it in v2. 

The cahngelog say this:

    Move check for TASK_SIZE before MAP_FIXED in sparc32's arch_get_unmapped_area().

And this is exactly what the code does.

But neither say anything WHY this is done.
And if that WHY is not explained then the changelog is not good.

	Sam

Re: [PATCH 3/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc32

[Vladimir Murzin]

On Wed, May 09, 2012 at 06:18:34PM +0200, Sam Ravnborg wrote:
> On Wed, May 09, 2012 at 12:07:08PM +0400, Vladimir Murzin wrote:
> > On Tue, May 08, 2012 at 06:27:22PM +0200, Sam Ravnborg wrote:
> > > On Tue, May 08, 2012 at 06:40:19PM +0400, Vladimir Murzin wrote:
> > > > Move check for TASK_SIZE before MAP_FIXED in sparc32's arch_get_unmapped_area().
> > > 
> > > I would be good to copy relevant parts of your cover letter into each patch.
> > > when the patches are applied we looses the cover letter...
> > > 
> > 
> > In my opinion patches are quite trivial and self-explained. However,
> > if there are requests to update patches in this way I'll do it in v2. 
> 
> The cahngelog say this:
> 
>     Move check for TASK_SIZE before MAP_FIXED in sparc32's arch_get_unmapped_area().
> 
> And this is exactly what the code does.
> 
> But neither say anything WHY this is done.
> And if that WHY is not explained then the changelog is not good.
> 
> 	Sam

Ok. I'll update patches in v2.

Best wishes
Vladimir Murzin

Re: [PATCH 3/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc32

[David Miller]

From: Vladimir Murzin <murzin.v@gmail.com>
Date: Tue,  8 May 2012 18:40:19 +0400

> Move check for TASK_SIZE before MAP_FIXED in sparc32's arch_get_unmapped_area().
> 
> Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>

Acked-by: David S. Miller <davem@davemloft.net>

[PATCH 4/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc64

[Vladimir Murzin]

Move check for TASK_SIZE before MAP_FIXED in sparc64's
arch_get_unmapped_area() and arch_get_unmapped_area_topdown().

Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
---
 arch/sparc/kernel/sys_sparc_64.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/sparc/kernel/sys_sparc_64.c b/arch/sparc/kernel/sys_sparc_64.c
index 232df99..df1a121 100644
--- a/arch/sparc/kernel/sys_sparc_64.c
+++ b/arch/sparc/kernel/sys_sparc_64.c
@@ -120,6 +120,11 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsi
 	unsigned long start_addr;
 	int do_color_align;
 
+	if (test_thread_flag(TIF_32BIT))
+		task_size = STACK_TOP32;
+	if (unlikely(len > task_size || len >= VA_EXCLUDE_START))
+		return -ENOMEM;
+
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
 		 * cache aliasing constraints.
@@ -130,11 +135,6 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsi
 		return addr;
 	}
 
-	if (test_thread_flag(TIF_32BIT))
-		task_size = STACK_TOP32;
-	if (unlikely(len > task_size || len >= VA_EXCLUDE_START))
-		return -ENOMEM;
-
 	do_color_align = 0;
 	if (filp || (flags & MAP_SHARED))
 		do_color_align = 1;
@@ -211,6 +211,9 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	/* This should only ever run for 32-bit processes.  */
 	BUG_ON(!test_thread_flag(TIF_32BIT));
 
+	if (unlikely(len > task_size))
+		return -ENOMEM;
+
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
 		 * cache aliasing constraints.
@@ -221,9 +224,6 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 		return addr;
 	}
 
-	if (unlikely(len > task_size))
-		return -ENOMEM;
-
 	do_color_align = 0;
 	if (filp || (flags & MAP_SHARED))
 		do_color_align = 1;
-- 
1.7.2.5

Re: [PATCH 4/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on sparc64

[David Miller]

From: Vladimir Murzin <murzin.v@gmail.com>
Date: Tue,  8 May 2012 18:40:20 +0400

> Move check for TASK_SIZE before MAP_FIXED in sparc64's
> arch_get_unmapped_area() and arch_get_unmapped_area_topdown().
> 
> Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>

Acked-by: David S. Miller <davem@davemloft.net>

[PATCH 5/6] get_unmapped_area checks for TASK_SIZE before MAP_FIXED on x86_64

[Vladimir Murzin]

Move check for TASK_SIZE before MAP_FIXED in x86_64's arch_get_unmapped_area().

Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
---
 arch/x86/kernel/sys_x86_64.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
index 0514890..1988a9f 100644
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c
@@ -129,14 +129,14 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	unsigned long start_addr;
 	unsigned long begin, end;
 
-	if (flags & MAP_FIXED)
-		return addr;
-
 	find_start_end(flags, &begin, &end);
 
 	if (len > end)
 		return -ENOMEM;
 
+	if (flags & MAP_FIXED)
+		return addr;
+
 	if (addr) {
 		addr = PAGE_ALIGN(addr);
 		vma = find_vma(mm, addr);
-- 
1.7.2.5

[PATCH 6/6] get_unmapped_area remove extra check for TASK_SIZE

[Vladimir Murzin]

architecture specific and generic arch_get_unmapped_area() and
arch_get_unmapped_area_topdown() check for TASK_SIZE before
MAP_FIXED.

It is safe to remove check for TASK_SIZE in get_unmapped_area().

Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
---
 mm/mmap.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index eae90af..6e6f934 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1544,10 +1544,6 @@ get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
 	if (error)
 		return error;
 
-	/* Careful about overflows.. */
-	if (len > TASK_SIZE)
-		return -ENOMEM;
-
 	get_area = current->mm->get_unmapped_area;
 	if (file && file->f_op && file->f_op->get_unmapped_area)
 		get_area = file->f_op->get_unmapped_area;
-- 
1.7.2.5

Re: [PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

[Russell King - ARM Linux]

On Tue, May 08, 2012 at 06:40:16PM +0400, Vladimir Murzin wrote:
> From: Vladimir Murzin <murzin.v@gmail>
> 
> The current get_unmapped_area code calls the f_ops->get_unmapped_area or
> the arch's one (via the mm) only when check for TASK_SIZE is passed. However,
> generic code and some arches do the same check in their a_g_u_a implementation.
> 
> This series of patches fix the check order for TASK_SIZE in archs'
> get_unmapped_area() implementations, and then removes extra check in
> high-level get_unmapped_area().

Do we even need this check in arch code?  AFAICS it's already checked in
get_unmapped_area(), and this will be called prior to any
arch_get_unmapped_area() implementation.  Given that this is a potential
security issue, please check my analysis of this.

unsigned long
get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
		unsigned long pgoff, unsigned long flags)
{
	...
	/* Careful about overflows.. */
	if (len > TASK_SIZE)
		return -ENOMEM;

	get_area = current->mm->get_unmapped_area;
	if (file && file->f_op && file->f_op->get_unmapped_area)
		get_area = file->f_op->get_unmapped_area;

Re: [PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

[Vladimir Murzin]

On Wed, May 09, 2012 at 05:26:57PM +0100, Russell King - ARM Linux wrote:
> On Tue, May 08, 2012 at 06:40:16PM +0400, Vladimir Murzin wrote:
> > From: Vladimir Murzin <murzin.v@gmail>
> > 
> > The current get_unmapped_area code calls the f_ops->get_unmapped_area or
> > the arch's one (via the mm) only when check for TASK_SIZE is passed. However,
> > generic code and some arches do the same check in their a_g_u_a implementation.
> > 
> > This series of patches fix the check order for TASK_SIZE in archs'
> > get_unmapped_area() implementations, and then removes extra check in
> > high-level get_unmapped_area().
> 
> Do we even need this check in arch code?  AFAICS it's already checked in
> get_unmapped_area(), and this will be called prior to any
> arch_get_unmapped_area() implementation.  Given that this is a potential
> security issue, please check my analysis of this.
> 
> unsigned long
> get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
> 		unsigned long pgoff, unsigned long flags)
> {
> 	...
> 	/* Careful about overflows.. */
> 	if (len > TASK_SIZE)
> 		return -ENOMEM;
> 
> 	get_area = current->mm->get_unmapped_area;
> 	if (file && file->f_op && file->f_op->get_unmapped_area)
> 		get_area = file->f_op->get_unmapped_area;

Thanks for analysis.

Most of arches do checking for (len > TASK_SIZE) in their a_g_u_a or in
generic one. However, mips, alpha, sparc and ia64 at least do this
checking in a slightly different way.

So, for arches which use generic implementation or have no any special
case

 	/* Careful about overflows.. */
	if (len > TASK_SIZE)
 		return -ENOMEM;
 
 	get_area = current->mm->get_unmapped_area;

is expanded into

 	/* Careful about overflows.. */
	if (len > TASK_SIZE)
 		return -ENOMEM;
 
 	/* there is arch_get_unmapped_area started */

	if (len > TASK_SIZE)
 		return -ENOMEM;

 	/* other stuff in arch_get_unmapped_area */

On the other hand, for arches which have to handle special case for
length checking test for (len > TASK_SIZE) has no sense.

To avoid security issue checking for length should be done
first. Unfortunately, not all arches follow this rule and test in
get_unmapped_area() doesn't cover some cases. For instanse, sparc32 do
checking like

unsigned long arch_get_unmapped_area(struct file *filp, unsigned long
 addr, unsigned long len, unsigned long pgoff, unsigned long flags)
{
        struct vm_area_struct * vmm;

        if (flags & MAP_FIXED) {
                /* We do not accept a shared mapping if it would violate
                 * cache aliasing constraints.
                 */
                if ((flags & MAP_SHARED) &&
                    ((addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1)))
                        return -EINVAL;
                return addr;
        }

        /* See asm-sparc/uaccess.h */
        if (len > TASK_SIZE - PAGE_SIZE)
                return -ENOMEM;

...

It seems we could be successful in request a page at fixed address
(TASK_SIZE - PAGE_SIZE) despite the fact that it isn't desirable.

Best wishes
Vladimir Murzin

Re: [PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

[Russell King - ARM Linux]

On Wed, May 09, 2012 at 09:56:03PM +0400, Vladimir Murzin wrote:
> On Wed, May 09, 2012 at 05:26:57PM +0100, Russell King - ARM Linux wrote:
> > On Tue, May 08, 2012 at 06:40:16PM +0400, Vladimir Murzin wrote:
> > > From: Vladimir Murzin <murzin.v@gmail>
> > > 
> > > The current get_unmapped_area code calls the f_ops->get_unmapped_area or
> > > the arch's one (via the mm) only when check for TASK_SIZE is passed. However,
> > > generic code and some arches do the same check in their a_g_u_a implementation.
> > > 
> > > This series of patches fix the check order for TASK_SIZE in archs'
> > > get_unmapped_area() implementations, and then removes extra check in
> > > high-level get_unmapped_area().
> > 
> > Do we even need this check in arch code?  AFAICS it's already checked in
> > get_unmapped_area(), and this will be called prior to any
> > arch_get_unmapped_area() implementation.  Given that this is a potential
> > security issue, please check my analysis of this.
> > 
> > unsigned long
> > get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
> > 		unsigned long pgoff, unsigned long flags)
> > {
> > 	...
> > 	/* Careful about overflows.. */
> > 	if (len > TASK_SIZE)
> > 		return -ENOMEM;
> > 
> > 	get_area = current->mm->get_unmapped_area;
> > 	if (file && file->f_op && file->f_op->get_unmapped_area)
> > 		get_area = file->f_op->get_unmapped_area;
> 
> Thanks for analysis.
> 
> Most of arches do checking for (len > TASK_SIZE) in their a_g_u_a or in
> generic one. However, mips, alpha, sparc and ia64 at least do this
> checking in a slightly different way.

I think you missed what I said above.  I think get_unmapped_area() gets
called, which _then_ calls into arch_get_unmapped_area() or some alternative
replacement.  I also think that nothing other than get_unmapped_area()
ultimately calls through to arch_get_unmapped_area().

So if get_unmapped_area() is doing:

	/* Careful about overflows.. */
	if (len > TASK_SIZE)
		return -ENOMEM;

_before_ it passes control to arch_get_unmapped_area(), is there any point
arch_get_unmapped_area() duplicating this exact same check?

Can't we just delete all these duplicate checks in arch_get_unmapped_area()
and be done with it, because...

> So, for arches which use generic implementation or have no any special
> case
> 
>  	/* Careful about overflows.. */
> 	if (len > TASK_SIZE)
>  		return -ENOMEM;
>  
>  	get_area = current->mm->get_unmapped_area;
> 
> is expanded into
> 
>  	/* Careful about overflows.. */
> 	if (len > TASK_SIZE)
>  		return -ENOMEM;
>  
>  	/* there is arch_get_unmapped_area started */
> 
> 	if (len > TASK_SIZE)
>  		return -ENOMEM;
> 
>  	/* other stuff in arch_get_unmapped_area */

... having that second check there is pointless.

> On the other hand, for arches which have to handle special case for
> length checking test for (len > TASK_SIZE) has no sense.
> 
> To avoid security issue checking for length should be done
> first. Unfortunately, not all arches follow this rule and test in
> get_unmapped_area() doesn't cover some cases.

But does it matter?  get_unmapped_area() has already checked 'len' and
would have failed if this was larger than TASK_SIZE already.

> For instanse, sparc32 do checking like
> 
> unsigned long arch_get_unmapped_area(struct file *filp, unsigned long
>  addr, unsigned long len, unsigned long pgoff, unsigned long flags)
> {
>         struct vm_area_struct * vmm;
> 
>         if (flags & MAP_FIXED) {
>                 /* We do not accept a shared mapping if it would violate
>                  * cache aliasing constraints.
>                  */
>                 if ((flags & MAP_SHARED) &&
>                     ((addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1)))
>                         return -EINVAL;
>                 return addr;
>         }
> 
>         /* See asm-sparc/uaccess.h */
>         if (len > TASK_SIZE - PAGE_SIZE)
>                 return -ENOMEM;

Where an arch does a different check (eg, and it is a smaller size) then
yes, there could be a problem.

But for all those which duplicate the:

	if (len > TASK_SIZE)
		return -ENOMEM;

check, it seems totally pointless to have that code in the arch function,
and I think we should be deleting that from the arch functions.

Re: [PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

[Vladimir Murzin]

On Wed, May 09, 2012 at 07:07:01PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 09, 2012 at 09:56:03PM +0400, Vladimir Murzin wrote:
> > On Wed, May 09, 2012 at 05:26:57PM +0100, Russell King - ARM Linux wrote:
> > > On Tue, May 08, 2012 at 06:40:16PM +0400, Vladimir Murzin wrote:
> > > > From: Vladimir Murzin <murzin.v@gmail>
> > > > 
> > > > The current get_unmapped_area code calls the f_ops->get_unmapped_area or
> > > > the arch's one (via the mm) only when check for TASK_SIZE is passed. However,
> > > > generic code and some arches do the same check in their a_g_u_a implementation.
> > > > 
> > > > This series of patches fix the check order for TASK_SIZE in archs'
> > > > get_unmapped_area() implementations, and then removes extra check in
> > > > high-level get_unmapped_area().
> > > 
> > > Do we even need this check in arch code?  AFAICS it's already checked in
> > > get_unmapped_area(), and this will be called prior to any
> > > arch_get_unmapped_area() implementation.  Given that this is a potential
> > > security issue, please check my analysis of this.
> > > 
> > > unsigned long
> > > get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
> > > 		unsigned long pgoff, unsigned long flags)
> > > {
> > > 	...
> > > 	/* Careful about overflows.. */
> > > 	if (len > TASK_SIZE)
> > > 		return -ENOMEM;
> > > 
> > > 	get_area = current->mm->get_unmapped_area;
> > > 	if (file && file->f_op && file->f_op->get_unmapped_area)
> > > 		get_area = file->f_op->get_unmapped_area;
> > 
> > Thanks for analysis.
> > 
> > Most of arches do checking for (len > TASK_SIZE) in their a_g_u_a or in
> > generic one. However, mips, alpha, sparc and ia64 at least do this
> > checking in a slightly different way.
> 
> I think you missed what I said above.  I think get_unmapped_area() gets
> called, which _then_ calls into arch_get_unmapped_area() or some alternative
> replacement.  I also think that nothing other than get_unmapped_area()
> ultimately calls through to arch_get_unmapped_area().
> 
> So if get_unmapped_area() is doing:
> 
> 	/* Careful about overflows.. */
> 	if (len > TASK_SIZE)
> 		return -ENOMEM;
> 
> _before_ it passes control to arch_get_unmapped_area(), is there any point
> arch_get_unmapped_area() duplicating this exact same check?
> 
> Can't we just delete all these duplicate checks in arch_get_unmapped_area()
> and be done with it, because...
> 
> > So, for arches which use generic implementation or have no any special
> > case
> > 
> >  	/* Careful about overflows.. */
> > 	if (len > TASK_SIZE)
> >  		return -ENOMEM;
> >  
> >  	get_area = current->mm->get_unmapped_area;
> > 
> > is expanded into
> > 
> >  	/* Careful about overflows.. */
> > 	if (len > TASK_SIZE)
> >  		return -ENOMEM;
> >  
> >  	/* there is arch_get_unmapped_area started */
> > 
> > 	if (len > TASK_SIZE)
> >  		return -ENOMEM;
> > 
> >  	/* other stuff in arch_get_unmapped_area */
> 
> ... having that second check there is pointless.
> 
> > On the other hand, for arches which have to handle special case for
> > length checking test for (len > TASK_SIZE) has no sense.
> > 
> > To avoid security issue checking for length should be done
> > first. Unfortunately, not all arches follow this rule and test in
> > get_unmapped_area() doesn't cover some cases.
> 
> But does it matter?  get_unmapped_area() has already checked 'len' and
> would have failed if this was larger than TASK_SIZE already.
> 
> > For instanse, sparc32 do checking like
> > 
> > unsigned long arch_get_unmapped_area(struct file *filp, unsigned long
> >  addr, unsigned long len, unsigned long pgoff, unsigned long flags)
> > {
> >         struct vm_area_struct * vmm;
> > 
> >         if (flags & MAP_FIXED) {
> >                 /* We do not accept a shared mapping if it would violate
> >                  * cache aliasing constraints.
> >                  */
> >                 if ((flags & MAP_SHARED) &&
> >                     ((addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1)))
> >                         return -EINVAL;
> >                 return addr;
> >         }
> > 
> >         /* See asm-sparc/uaccess.h */
> >         if (len > TASK_SIZE - PAGE_SIZE)
> >                 return -ENOMEM;
> 
> Where an arch does a different check (eg, and it is a smaller size) then
> yes, there could be a problem.
> 
> But for all those which duplicate the:
> 
> 	if (len > TASK_SIZE)
> 		return -ENOMEM;
> 
> check, it seems totally pointless to have that code in the arch function,
> and I think we should be deleting that from the arch functions.

I was thinking about your suggestion. We are speaking about the same
problem but different solutions. Let me summarize shortly why I came
up with current solution:
   * leaving check in arches make them isolated, so
     mm->get_unmapped_area could be called safely anywhere (currently it
     is done in hugetlb and get_fb_unmapped_area stuff)
   * we have no any extra (pointless?) checks for arches which has
     stricter test for length
   * we have no any duplicating checks for arches which follows generic
     implementation for get_ynmapped_area

Best wishes
Vladimir Murzin

Re: [PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

[Russell King - ARM Linux]

On Thu, May 10, 2012 at 07:01:46AM +0400, Vladimir Murzin wrote:
> I was thinking about your suggestion. We are speaking about the same
> problem but different solutions. Let me summarize shortly why I came
> up with current solution:

I don't think there is a problem here.  I think there's just redundant
code in every arch apart from Sparc.

>    * leaving check in arches make them isolated, so
>      mm->get_unmapped_area could be called safely anywhere (currently it
>      is done in hugetlb and get_fb_unmapped_area stuff)

Right.  So is get_fb_unmapped_area() called without first going through
get_unmapped_area() ?  As far as I can see, it isn't.  Same for hugetlbfs.

I don't think you've analysed the issue you are trying to address correctly.
As such I will *not* be giving you an ACK for your current changes to
arch/arm.

Re: [PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

[Vladimir Murzin]

On Thu, May 10, 2012 at 08:55:04AM +0100, Russell King - ARM Linux wrote:
> On Thu, May 10, 2012 at 07:01:46AM +0400, Vladimir Murzin wrote:
> > I was thinking about your suggestion. We are speaking about the same
> > problem but different solutions. Let me summarize shortly why I came
> > up with current solution:
> 
> I don't think there is a problem here.  I think there's just redundant
> code in every arch apart from Sparc.
> 
> >    * leaving check in arches make them isolated, so
> >      mm->get_unmapped_area could be called safely anywhere (currently it
> >      is done in hugetlb and get_fb_unmapped_area stuff)
> 
> Right.  So is get_fb_unmapped_area() called without first going through
> get_unmapped_area() ?  As far as I can see, it isn't.  Same for hugetlbfs.

D'0h I've missed f_op there.. Thanks for pointing that.

> 
> I don't think you've analysed the issue you are trying to address correctly.
> As such I will *not* be giving you an ACK for your current changes to
> arch/arm.

I should have analysed this more carefully. By now I can see even more
redundant code. Actually, for some arches test for length has already
done in arch_mmap_check(). For instance, sparc32 and sparc64 do the
same check in sparc_mmap_check() as in arch_get_unmapped_area(). The
only one difference in return value: -EINVAL in lieu of -ENOMEM.
The same case for ia64.

Hmmm... What about extending generic arch_mmap_check with check (len >
TASK_SIZE) and updating arches which are coping with special cases?

Best wishes
Vladimir Murzin