Messages in this thread | | | Date | Mon, 21 Aug 2006 23:11:04 +0200 | From | Willy Tarreau <> | Subject | Re: [PATCH] binfmt_elf.c : the BAD_ADDR macro again |
| |
On Mon, Aug 21, 2006 at 04:35:47PM -0400, Ernie Petrides wrote: > On Sunday, 20-Aug-2006 at 18:23 +0200, Willy Tarreau wrote: > > > On Sun, Aug 20, 2006 at 07:51:22PM +0400, Solar Designer wrote: > > > On Sun, Aug 20, 2006 at 11:15:15AM +0200, Willy Tarreau wrote: > > > > The proper fix would then be : > > > [...] > > > > -#define BAD_ADDR(x) ((unsigned long)(x) > TASK_SIZE) > > > > +#define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE) > > > [...] > > > > - if (k > TASK_SIZE || eppnt->p_filesz > eppnt->p_memsz || > > > > + if (BAD_ADDR(k) || eppnt->p_filesz > eppnt->p_memsz || > > > [...] > > > > - if (k > TASK_SIZE || elf_ppnt->p_filesz > elf_ppnt->p_memsz || > > > > + if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz || > > > > > > Looks OK to me. > > These are all correct.
OK.
> > > > And even then, I'm not happy with this test : > > > > > > > > TASK_SIZE - elf_ppnt->p_memsz < k > > > > > > > > because it means that we only raise the error when > > > > > > > > k + elf_ppnt->p_memsz > TASK_SIZE > > > > > > > > I really think that we want to check this instead : > > > > > > > > k + elf_ppnt->p_memsz >= TASK_SIZE > > > > > > > > Otherwise we leave a window where this is undetected : > > > > > > > > load_addr + eppnt->p_vaddr == TASK_SIZE - eppnt->p_memsz > > The reason I did not propose changing these is because these are > end-point checks (as opposed to starting address checks). I think > that the following "equals" condition is conceptually valid: > > (starting-address + region-size == TASK_SIZE)
Agreed.
> > > > This will later lead to last_bss getting readjusted to TASK_SIZE, which I > > > > don't think is expected at all : > > > > > > > > k = load_addr + eppnt->p_memsz + eppnt->p_vaddr; > > > > if (k > last_bss) > > > > last_bss = k; > > This is an interesting case, but I think the error checking works okay. > > After the ELF phdr loop, the resulting "last_bss" is used as follows: > > /* Map the last of the bss segment */ > if (last_bss > elf_bss) { > down_write(¤t->mm->mmap_sem); > error = do_brk(elf_bss, last_bss - elf_bss); > up_write(¤t->mm->mmap_sem); > if (BAD_ADDR(error)) > goto out_close; > } > > The variable "last_bss" is used to compute the size argument in the > call to do_brk(). If the section extends beyond TASK_SIZE, then do_brk() > will return -EINVAL. If the do_brk() call succeeds but "elf_bss" is itself > exactly at TASK_SIZE, then the BAD_ADDR() call above will catch it.
OK. Thanks for the explanation.
> > [...] But before this, I'd like to get comments from > > the people who discussed the subject recently. > > Thus, I think that both 2.4.33 and 2.6.<latest> are okay without any > further changes.
At least 2.4 needs the fix to use the correct BAD_ADDR (which is not OK in 2.4.33 yet).
> Cheers. -ernie
Thanks Ernie, Willy
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |