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.
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
