lkml.org 
[lkml]   [2006]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2006-08-20 17:59    [W:0.034 / U:2.760 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site