Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size

[pageexec@freemail.hu]

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@redhat.com>
> ---
>  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;
>