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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
