Re: Is it legal to return positive value when do_execve() succeeds?

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

> KOSAKI Motohiro wrote:
> > I think you should read do_brk() itself. the spec is
> > 
> > success case:
> > 	return addr argument
> > 
> > error case: 
> > 	return error code
> > 
> > When does it return invalid address?
> 
> If addr argument is bogus.
> 
> 
> 
> load_elf_binary() in fs/binfmt_elf.c
> 758                 if (unlikely (elf_brk > elf_bss)) {
> 759                         unsigned long nbyte;
> 760                     
> 761                         /* There was a PT_LOAD segment with p_memsz > p_filesz
> 762                            before this one. Map anonymous pages, if needed,
> 763                            and clear the area.  */
> 764                         retval = set_brk (elf_bss + load_bias,
> 765                                           elf_brk + load_bias);
> 766                         if (retval) {
> 767                                 send_sig(SIGKILL, current, 0);
> 
> I think this function is using address information from untrusted source
> (executable file). 

True.

> Thus, I think retval could be
> ELF_PAGEALIGN(elf_bss + load_bias) if elf_bss + load_bias is bogus.

What's mean bogus?
do_brk() call get_unmapped_area() and it check an argument is correctly
unmapped and userland address. If elf_bss + load_bias point to invalid
address, set_brk doesn't return elf_bss+load_bias.


> I'm OK with this if it is guaranteed that elf_bss + load_bias is always valid
> and set_brk() never returns ELF_PAGEALIGN(elf_bss + load_bias).

I think elf_bss + load_bias could be invalid (i.e. >TASK_SIZE). 
but set_brk can detect it.

-------------------------------------------------------------------------
unsigned long
get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
                unsigned long pgoff, unsigned long flags)
{
        unsigned long (*get_area)(struct file *, unsigned long,
                                  unsigned long, unsigned long, unsigned long);

        unsigned long error = arch_mmap_check(addr, len, flags);
        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;
        addr = get_area(file, addr, len, pgoff, flags);
        if (IS_ERR_VALUE(addr))
                return addr;

        if (addr > TASK_SIZE - len)			// HERE
                return -ENOMEM;
        if (addr & ~PAGE_MASK)
                return -EINVAL;

        return arch_rebalance_pgtables(addr, len);
}
-------------------------------------------------------------------------


> 
> 768                                 goto out_free_dentry;
> 769                         }
> (...snipped...)
> 835                 /*
> 836                  * Check to see if the section's size will overflow the
> 837                  * allowed task size. Note that p_filesz must always be
> 838                  * <= p_memsz so it is only necessary to check p_memsz.
> 839                  */
> 840                 if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
> 841                     elf_ppnt->p_memsz > TASK_SIZE ||
> 842                     TASK_SIZE - elf_ppnt->p_memsz < k) {
> 843                         /* set_brk can never work. Avoid overflows. */
> 844                         send_sig(SIGKILL, current, 0);
> 845                         retval = -EINVAL;
> 
> Here setting -EINVAL. So why not to set a valid error code for set_brk() above?

I don't know. perhaps I am missing anything :)
Please double check.