Re: [PATCH] binfmt_elf.c : the BAD_ADDR macro again

[Solar Designer <solar@openwall.com>]

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