Messages in this thread | | | Date | Sun, 20 Aug 2006 19:51:22 +0400 | From | Solar Designer <> | Subject | Re: [PATCH] binfmt_elf.c : the BAD_ADDR macro again |
| |
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.
> 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 > > 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; > > Then I think we should change this at both places : > > - TASK_SIZE - elf_ppnt->p_memsz < k) { > + BAD_ADDR(k + elf_ppnt->p_memsz)) {
I am not sure about these re-arrangements - I'd need to review them in full context to make sure that we don't inadvertently change things as it relates to behavior on integer overflows, which I presently do not have the time for. I'll trust that you do such a review with integer overflows and variable type differences (size, signedness) in mind now that I've mentioned this potential danger. ;-) Alternatively, you can simply change the comparisons from < to <= and from > to >= rather than use the BAD_ADDR() macro.
Thanks,
Alexander - 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/
| |