Re: patch: VFS: fix passing of AT_PHDR value in auxv to ELF interpreter

[Quentin Godfroy <godfroy@clipper.ens.fr>]

On Fri, May 04, 2007 at 10:09:21AM -0400, Quentin Godfroy wrote:
> On a dynamic ELF executable, the current kernel loader gives to the
> interpreter (in the AUXV vector) the AT_PHDR argument as :
> offset_of_phdr_in_file + first address.
> 
> It can be wrong for an executable where the program headers are not located 
> in the first loaded segment.
> 
> This patch corrects the behaviour.
> 
> Signed-off-by: Quentin Godfroy <godfroy@clipper.ens.fr>
> ---
>  Here is an example of such an ELF executable which the current code
>  fails on :
>  ftp://quatramaran.ens.fr/pub/godfroy/addrpath/broken-sample
> 
> --- linux-2.6.21.1/fs/binfmt_elf.c	2007-05-04 03:20:00.000000000 -0400
> +++ linux-2.6.21.1-patch/fs/binfmt_elf.c	2007-05-04 08:02:18.000000000 -0400
> @@ -134,6 +134,7 @@ static int padzero(unsigned long elf_bss
>  static int
>  create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
>  		int interp_aout, unsigned long load_addr,
> +		unsigned long phdr_addr,
>  		unsigned long interp_load_addr)
>  {
>  	unsigned long p = bprm->p;
> @@ -190,7 +191,7 @@ create_elf_tables(struct linux_binprm *b
>  	NEW_AUX_ENT(AT_HWCAP, ELF_HWCAP);
>  	NEW_AUX_ENT(AT_PAGESZ, ELF_EXEC_PAGESIZE);
>  	NEW_AUX_ENT(AT_CLKTCK, CLOCKS_PER_SEC);
> -	NEW_AUX_ENT(AT_PHDR, load_addr + exec->e_phoff);
> +	NEW_AUX_ENT(AT_PHDR, phdr_addr);
>  	NEW_AUX_ENT(AT_PHENT, sizeof(struct elf_phdr));
>  	NEW_AUX_ENT(AT_PHNUM, exec->e_phnum);
>  	NEW_AUX_ENT(AT_BASE, interp_load_addr);
> @@ -529,7 +530,7 @@ static unsigned long randomize_stack_top
>  static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>  {
>  	struct file *interpreter = NULL; /* to shut gcc up */
> - 	unsigned long load_addr = 0, load_bias = 0;
> + 	unsigned long load_addr = 0, load_bias = 0, phdr_addr = 0;
>  	int load_addr_set = 0;
>  	char * elf_interpreter = NULL;
>  	unsigned int interpreter_type = INTERPRETER_NONE;
> @@ -718,6 +719,16 @@ static int load_elf_binary(struct linux_
>  			break;
>  		}
>  
> +	elf_ppnt = elf_phdata;
> +	for (i = 0; i< loc->elf_ex.e_phnum; i++, elf_ppnt++)
> +		if (elf_ppnt->p_type == PT_PHDR) {
> +			phdr_addr = elf_ppnt->p_vaddr;
> +			break;
> +		}
> +	retval = -ENOEXEC;
> +	if (!phdr_addr)
> +		goto out_free_dentry;
> +
>  	/* Some simple consistency checks for the interpreter */
>  	if (elf_interpreter) {
>  		interpreter_type = INTERPRETER_ELF | INTERPRETER_AOUT;
> @@ -987,7 +998,7 @@ static int load_elf_binary(struct linux_
>  	current->flags &= ~PF_FORKNOEXEC;
>  	create_elf_tables(bprm, &loc->elf_ex,
>  			  (interpreter_type == INTERPRETER_AOUT),
> -			  load_addr, interp_load_addr);
> +			  load_addr, phdr_addr, interp_load_addr);
>  	/* N.B. passed_fileno might not be initialized? */
>  	if (interpreter_type == INTERPRETER_AOUT)
>  		current->mm->arg_start += strlen(passed_fileno) + 1;

I have made another patch, which I hope should not break anything this
time. I tested it on an x86_64 kernel with 32 and 64 bits executables,
PIE and not PIE (well my only PIE are libc-2.5.so and ld-2.5.so). I
rejoined the loops as suggested, and the kernel falls back on load_addr +
exec->e_phoff if there was no PT_PHDR entry in the program headers.

Signed-off-by: Quentin Godfroy <godfroy@clipper.ens.fr>
---
--- linux-2.6.21.1/fs/binfmt_elf.c	2007-05-07 10:58:38.000000000 -0400
+++ linux-2.6.21.1-patch/fs/binfmt_elf.c	2007-05-07 13:14:33.000000000 -0400
@@ -134,6 +134,7 @@ static int padzero(unsigned long elf_bss
 static int
 create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 		int interp_aout, unsigned long load_addr,
+		unsigned long phdr_addr,
 		unsigned long interp_load_addr)
 {
 	unsigned long p = bprm->p;
@@ -190,7 +191,10 @@ create_elf_tables(struct linux_binprm *b
 	NEW_AUX_ENT(AT_HWCAP, ELF_HWCAP);
 	NEW_AUX_ENT(AT_PAGESZ, ELF_EXEC_PAGESIZE);
 	NEW_AUX_ENT(AT_CLKTCK, CLOCKS_PER_SEC);
-	NEW_AUX_ENT(AT_PHDR, load_addr + exec->e_phoff);
+	if(phdr_addr)
+		NEW_AUX_ENT(AT_PHDR, phdr_addr);
+	else
+		NEW_AUX_ENT(AT_PHDR, load_addr + exec->e_phoff);
 	NEW_AUX_ENT(AT_PHENT, sizeof(struct elf_phdr));
 	NEW_AUX_ENT(AT_PHNUM, exec->e_phnum);
 	NEW_AUX_ENT(AT_BASE, interp_load_addr);
@@ -529,7 +533,7 @@ static unsigned long randomize_stack_top
 static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */
- 	unsigned long load_addr = 0, load_bias = 0;
+ 	unsigned long load_addr = 0, load_bias = 0, phdr_addr = 0;
 	int load_addr_set = 0;
 	char * elf_interpreter = NULL;
 	unsigned int interpreter_type = INTERPRETER_NONE;
@@ -620,7 +624,12 @@ static int load_elf_binary(struct linux_
 	start_data = 0;
 	end_data = 0;
 
-	for (i = 0; i < loc->elf_ex.e_phnum; i++) {
+	for (i = 0; i < loc->elf_ex.e_phnum; i++, elf_ppnt++) {
+		if (elf_ppnt->p_type == PT_PHDR) {
+			phdr_addr = elf_ppnt->p_vaddr;
+			continue;
+		}
+
 		if (elf_ppnt->p_type == PT_INTERP) {
 			/* This is the program interpreter used for
 			 * shared libraries - for now assume that this
@@ -703,20 +712,17 @@ static int load_elf_binary(struct linux_
 			/* Get the exec headers */
 			loc->interp_ex = *((struct exec *)bprm->buf);
 			loc->interp_elf_ex = *((struct elfhdr *)bprm->buf);
-			break;
+			continue;
 		}
-		elf_ppnt++;
-	}
 
-	elf_ppnt = elf_phdata;
-	for (i = 0; i < loc->elf_ex.e_phnum; i++, elf_ppnt++)
 		if (elf_ppnt->p_type == PT_GNU_STACK) {
 			if (elf_ppnt->p_flags & PF_X)
 				executable_stack = EXSTACK_ENABLE_X;
 			else
 				executable_stack = EXSTACK_DISABLE_X;
-			break;
+			continue;
 		}
+	}
 
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {
@@ -985,9 +991,12 @@ static int load_elf_binary(struct linux_
 
 	compute_creds(bprm);
 	current->flags &= ~PF_FORKNOEXEC;
+	if (phdr_addr)
+		phdr_addr += load_bias;
+
 	create_elf_tables(bprm, &loc->elf_ex,
 			  (interpreter_type == INTERPRETER_AOUT),
-			  load_addr, interp_load_addr);
+			  load_addr, phdr_addr, interp_load_addr);
 	/* N.B. passed_fileno might not be initialized? */
 	if (interpreter_type == INTERPRETER_AOUT)
 		current->mm->arg_start += strlen(passed_fileno) + 1;
-
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/