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