On 7 Sep 2010 at 19:35, Roland McGrath wrote: > Check that the initial stack is not too large to make it possible > to map in any executable. We're not checking that the actual > executable (or intepreter, for binfmt_elf) will fit. So those > mappings might clobber part of the initial stack mapping. But > that is just userland lossage that userland made happen, not a > kernel problem. > > Signed-off-by: Roland McGrath <roland@xxxxxxxxxx> > --- > fs/exec.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 2d94552..1b63237 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -594,6 +594,11 @@ int setup_arg_pages(struct linux_binprm *bprm, > #else > stack_top = arch_align_stack(stack_top); > stack_top = PAGE_ALIGN(stack_top); > + > + if (unlikely(stack_top < mmap_min_addr) || this could arguably elicit some warning, or even better, prevent the sysctl from setting mmap_min_addr too high in the first place. or alternatively, just check for mmap_min_addr > stack_top - (vma->vm_end - vma->vm_start) which i think is more clear as to the intent of the check. and since the next line already computes stack_shift as vma->vm_end - stack_top, you could do the whole check afterwards as: mmap_min_addr > vma->vm_start - stack_shift which is even simpler and more to the point (you want what is called new_start in shift_arg_pages to not violate mmap_min_addr). now you just need the int overflow check, say: vma->vm_start < stack_shift, so the whole thing would become: if (vma->vm_start < stack_shift || mmap_min_addr > vma->vm_start - stack_shift) return -EFAULT; which looks even better if done in shift_arg_pages as i've done it in PaX: - BUG_ON(new_start > new_end); + if (new_start >= new_end || new_start < mmap_min_addr) + return -EFAULT; + unlikely(vma->vm_end - vma->vm_start >= stack_top - mmap_min_addr)) i think the = case is as valid as any other value close to it ;). also this code is not a fast path, no need for unlikely i think. > + return -ENOMEM; i'd say EFAULT is more consistent, ENOMEM is used for cases when the kernel couldn't allocate physical memory for something, EFAULT is when there's some issue with the address space itself (based on the reaction to find_vma or expand_stack failures). also what about the BUG_ON in shift_arg_pages? it could go now, right? personally, i prefer to do this the way i did it in PaX: move up the shift_arg_pages call a bit and turn the BUG_ON into a proper check. > stack_shift = vma->vm_end - stack_top; > > bprm->p -= stack_shift; > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
