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