linux-kernel - Re: [PATCH v3 01/21] vmcore: reference e_phoff member explicitly to get position of program header table

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 19 Mar 2013 14:44:16 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	HATAYAMA Daisuke <d.hatayama@...fujitsu.com>
Cc:	vgoyal@...hat.com, cpw@....com, kumagai-atsushi@....nes.nec.co.jp,
	lisa.mitchell@...com, heiko.carstens@...ibm.com,
	akpm@...ux-foundation.org, kexec@...ts.infradead.org,
	linux-kernel@...r.kernel.org, zhangyanfei@...fujitsu.com
Subject: Re: [PATCH v3 01/21] vmcore: reference e_phoff member explicitly to get position of program header table

HATAYAMA Daisuke <d.hatayama@...fujitsu.com> writes:

> Currently, the code assumes that position of program header table is
> next to ELF header. But future change can break the assumption on
> kexec-tools and the 1st kernel. To avoid worst case, reference e_phoff
> member explicitly to get position of program header table in
> file-offset.

In principle this looks good.  However when I read this it looks like
you are going a little too far.

You are changing not only the reading of the supplied headers, but
you are changing the generation of the new new headers that describe
the data provided by /proc/vmcore.

I get lost in following this after you mangle merge_note_headers.

In principle removing silly assumptions seems reasonable, but I think
it is completely orthogonal to the task of maping vmcore mmapable.

I think it is fine to claim that the assumptions made here in vmcore are
part of the kexec on panic ABI at this point, which would generally make
this change unnecessary.

> Signed-off-by: Zhang Yanfei <zhangyanfei@...fujitsu.com>
> Signed-off-by: HATAYAMA Daisuke <d.hatayama@...fujitsu.com>
> ---
>
>  fs/proc/vmcore.c |   56 +++++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index b870f74..163281e 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -221,8 +221,8 @@ static u64 __init get_vmcore_size_elf64(char *elfptr)
>  	Elf64_Phdr *phdr_ptr;
>  
>  	ehdr_ptr = (Elf64_Ehdr *)elfptr;
> -	phdr_ptr = (Elf64_Phdr*)(elfptr + sizeof(Elf64_Ehdr));
> -	size = sizeof(Elf64_Ehdr) + ((ehdr_ptr->e_phnum) * sizeof(Elf64_Phdr));
> +	phdr_ptr = (Elf64_Phdr*)(elfptr + ehdr_ptr->e_phoff);
> +	size = ehdr_ptr->e_phoff + ((ehdr_ptr->e_phnum) * sizeof(Elf64_Phdr));
>  	for (i = 0; i < ehdr_ptr->e_phnum; i++) {
>  		size += phdr_ptr->p_memsz;
>  		phdr_ptr++;
> @@ -238,8 +238,8 @@ static u64 __init get_vmcore_size_elf32(char *elfptr)
>  	Elf32_Phdr *phdr_ptr;
>  
>  	ehdr_ptr = (Elf32_Ehdr *)elfptr;
> -	phdr_ptr = (Elf32_Phdr*)(elfptr + sizeof(Elf32_Ehdr));
> -	size = sizeof(Elf32_Ehdr) + ((ehdr_ptr->e_phnum) * sizeof(Elf32_Phdr));
> +	phdr_ptr = (Elf32_Phdr*)(elfptr + ehdr_ptr->e_phoff);
> +	size = ehdr_ptr->e_phoff + ((ehdr_ptr->e_phnum) * sizeof(Elf32_Phdr));
>  	for (i = 0; i < ehdr_ptr->e_phnum; i++) {
>  		size += phdr_ptr->p_memsz;
>  		phdr_ptr++;

> @@ -259,7 +259,7 @@ static int __init merge_note_headers_elf64(char *elfptr, size_t *elfsz,
>  	u64 phdr_sz = 0, note_off;
>  
>  	ehdr_ptr = (Elf64_Ehdr *)elfptr;
> -	phdr_ptr = (Elf64_Phdr*)(elfptr + sizeof(Elf64_Ehdr));
> +	phdr_ptr = (Elf64_Phdr*)(elfptr + ehdr_ptr->e_phoff);
>  	for (i = 0; i < ehdr_ptr->e_phnum; i++, phdr_ptr++) {
>  		int j;
>  		void *notes_section;

Up to here things look good.

> @@ -305,7 +305,7 @@ static int __init merge_note_headers_elf64(char *elfptr, size_t *elfsz,
>  	/* Prepare merged PT_NOTE program header. */
>  	phdr.p_type    = PT_NOTE;
>  	phdr.p_flags   = 0;
> -	note_off = sizeof(Elf64_Ehdr) +
> +	note_off = ehdr_ptr->e_phoff +
>  			(ehdr_ptr->e_phnum - nr_ptnote +1) * sizeof(Elf64_Phdr);

And this is is just silly.  There is no point in changing where the
regenerated headers live.

>  	phdr.p_offset  = note_off;
>  	phdr.p_vaddr   = phdr.p_paddr = 0;
> @@ -313,14 +313,14 @@ static int __init merge_note_headers_elf64(char *elfptr, size_t *elfsz,
>  	phdr.p_align   = 0;
>  
>  	/* Add merged PT_NOTE program header*/
> -	tmp = elfptr + sizeof(Elf64_Ehdr);
> +	tmp = elfptr + ehdr_ptr->e_phoff;
Again this looks very silly.

>  	memcpy(tmp, &phdr, sizeof(phdr));
>  	tmp += sizeof(phdr);
>  
>  	/* Remove unwanted PT_NOTE program headers. */
>  	i = (nr_ptnote - 1) * sizeof(Elf64_Phdr);
>  	*elfsz = *elfsz - i;
> -	memmove(tmp, tmp+i, ((*elfsz)-sizeof(Elf64_Ehdr)-sizeof(Elf64_Phdr)));
> +	memmove(tmp, tmp+i, ((*elfsz)-ehdr_ptr->e_phoff-sizeof(Elf64_Phdr)));

This is a regenerated header so this change is dubious.


Eric
--
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/