linux-kernel - Re: [PATCH] binfmt_elf.c : the BAD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20060820155122.GA20108@openwall.com>
Date:	Sun, 20 Aug 2006 19:51:22 +0400
From:	Solar Designer <solar@...nwall.com>
To:	Willy Tarreau <w@....eu>
Cc:	linux-kernel@...r.kernel.org,
	Chuck Ebbert <76306.1226@...puserve.com>,
	Andrew Morton <akpm@...l.org>,
	Ernie Petrides <petrides@...hat.com>
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/