[PATCH v4 0/2] [PATCH v4 0/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[PATCH v4 0/2] [PATCH v4 0/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Akira Kawata]

 These patches fix a bug in AT_PHDR calculation. 
 
 We cannot calculate AT_PHDR as the sum of load_addr and exec->e_phoff.
 This is because exec->e_phoff is the offset of PHDRs in the file and the
 address of PHDRs in the memory may differ from it. These patches fix the
 bug by calculating the address of program headers from PT_LOADs
 directly.
 
 Sorry for my latency.
 
 Changes in v4
 - Reflecting comments from Lukas, add a refactoring commit.
 
 Changes in v3:
 - Fix a reported bug from kernel test robot.
 
 Changes in v2:
 - Remove unused load_addr from create_elf_tables.
 - Improve the commit message.

Akira Kawata (2):
  fs/binfmt_elf: Fix AT_PHDR for unusual ELF files
  fs/binfmt_elf: Refactor load_elf_binary function

 fs/binfmt_elf.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)


base-commit: 4eee8d0b64ecc3231040fa68ba750317ffca5c52
-- 
2.34.1

[PATCH v4 1/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Akira Kawata]

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=197921

As pointed out in the discussion of buglink, we cannot calculate AT_PHDR
as the sum of load_addr and exec->e_phoff.

: The AT_PHDR of ELF auxiliary vectors should point to the memory address
: of program header. But binfmt_elf.c calculates this address as follows:
:
: NEW_AUX_ENT(AT_PHDR, load_addr + exec->e_phoff);
:
: which is wrong since e_phoff is the file offset of program header and
: load_addr is the memory base address from PT_LOAD entry.
:
: The ld.so uses AT_PHDR as the memory address of program header. In normal
: case, since the e_phoff is usually 64 and in the first PT_LOAD region, it
: is the correct program header address.
:
: But if the address of program header isn't equal to the first PT_LOAD
: address + e_phoff (e.g.  Put the program header in other non-consecutive
: PT_LOAD region), ld.so will try to read program header from wrong address
: then crash or use incorrect program header.

This is because exec->e_phoff
is the offset of PHDRs in the file and the address of PHDRs in the
memory may differ from it. This patch fixes the bug by calculating the
address of program headers from PT_LOADs directly.

Signed-off-by: Akira Kawata <akirakawata1@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 fs/binfmt_elf.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index beeb1247b5c4..828e88841cb4 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -170,8 +170,8 @@ static int padzero(unsigned long elf_bss)
 
 static int
 create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
-		unsigned long load_addr, unsigned long interp_load_addr,
-		unsigned long e_entry)
+		unsigned long interp_load_addr,
+		unsigned long e_entry, unsigned long phdr_addr)
 {
 	struct mm_struct *mm = current->mm;
 	unsigned long p = bprm->p;
@@ -257,7 +257,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
 	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);
@@ -822,7 +822,7 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
 static int load_elf_binary(struct linux_binprm *bprm)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */
- 	unsigned long load_addr = 0, load_bias = 0;
+	unsigned long load_addr, load_bias = 0, phdr_addr = 0;
 	int load_addr_set = 0;
 	unsigned long error;
 	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
@@ -1168,6 +1168,13 @@ static int load_elf_binary(struct linux_binprm *bprm)
 				reloc_func_desc = load_bias;
 			}
 		}
+
+		if (elf_ppnt->p_offset <= elf_ex->e_phoff &&
+		    elf_ex->e_phoff < elf_ppnt->p_offset + elf_ppnt->p_filesz) {
+			phdr_addr = elf_ex->e_phoff - elf_ppnt->p_offset +
+				    elf_ppnt->p_vaddr;
+		}
+
 		k = elf_ppnt->p_vaddr;
 		if ((elf_ppnt->p_flags & PF_X) && k < start_code)
 			start_code = k;
@@ -1203,6 +1210,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	}
 
 	e_entry = elf_ex->e_entry + load_bias;
+	phdr_addr += load_bias;
 	elf_bss += load_bias;
 	elf_brk += load_bias;
 	start_code += load_bias;
@@ -1266,8 +1274,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		goto out;
 #endif /* ARCH_HAS_SETUP_ADDITIONAL_PAGES */
 
-	retval = create_elf_tables(bprm, elf_ex,
-			  load_addr, interp_load_addr, e_entry);
+	retval = create_elf_tables(bprm, elf_ex, interp_load_addr,
+				   e_entry, phdr_addr);
 	if (retval < 0)
 		goto out;
 
-- 
2.34.1

[PATCH v4 2/2] fs/binfmt_elf: Refactor load_elf_binary function

[Akira Kawata]

I delete load_addr because it is not used anymore. And I rename
load_addr_set to first_pt_load because it is used only to capture the
first iteration of the loop.

Signed-off-by: Akira Kawata <akirakawata1@gmail.com>
---
 fs/binfmt_elf.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 828e88841cb4..48206fd1a20e 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -822,8 +822,8 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
 static int load_elf_binary(struct linux_binprm *bprm)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */
-	unsigned long load_addr, load_bias = 0, phdr_addr = 0;
-	int load_addr_set = 0;
+	unsigned long load_bias = 0, phdr_addr = 0;
+	int first_pt_load = 1;
 	unsigned long error;
 	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
 	struct elf_phdr *elf_property_phdata = NULL;
@@ -1073,12 +1073,12 @@ static int load_elf_binary(struct linux_binprm *bprm)
 
 		vaddr = elf_ppnt->p_vaddr;
 		/*
-		 * The first time through the loop, load_addr_set is false:
+		 * The first time through the loop, first_pt_load is true:
 		 * layout will be calculated. Once set, use MAP_FIXED since
 		 * we know we've already safely mapped the entire region with
 		 * MAP_FIXED_NOREPLACE in the once-per-binary logic following.
 		 */
-		if (load_addr_set) {
+		if (!first_pt_load) {
 			elf_flags |= MAP_FIXED;
 		} else if (elf_ex->e_type == ET_EXEC) {
 			/*
@@ -1138,10 +1138,10 @@ static int load_elf_binary(struct linux_binprm *bprm)
 
 		/*
 		 * Calculate the entire size of the ELF mapping (total_size).
-		 * (Note that load_addr_set is set to true later once the
+		 * (Note that first_pt_load is set to false later once the
 		 * initial mapping is performed.)
 		 */
-		if (!load_addr_set) {
+		if (first_pt_load) {
 			total_size = total_mapping_size(elf_phdata,
 							elf_ex->e_phnum);
 			if (!total_size) {
@@ -1158,13 +1158,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			goto out_free_dentry;
 		}
 
-		if (!load_addr_set) {
-			load_addr_set = 1;
-			load_addr = (elf_ppnt->p_vaddr - elf_ppnt->p_offset);
+		if (first_pt_load) {
+			first_pt_load = 0;
 			if (elf_ex->e_type == ET_DYN) {
 				load_bias += error -
 				             ELF_PAGESTART(load_bias + vaddr);
-				load_addr += load_bias;
 				reloc_func_desc = load_bias;
 			}
 		}
-- 
2.34.1

[PATCH v4 0/2] [PATCH v4 0/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Akira Kawata]

 These patches fix a bug in AT_PHDR calculation. 
 
 We cannot calculate AT_PHDR as the sum of load_addr and exec->e_phoff.
 This is because exec->e_phoff is the offset of PHDRs in the file and the
 address of PHDRs in the memory may differ from it. These patches fix the
 bug by calculating the address of program headers from PT_LOADs
 directly.
 
 Sorry for my latency.
 
 Changes in v4
 - Reflecting comments from Lukas, add a refactoring commit.
 
 Changes in v3:
 - Fix a reported bug from kernel test robot.
 
 Changes in v2:
 - Remove unused load_addr from create_elf_tables.
 - Improve the commit message.

Akira Kawata (2):
  fs/binfmt_elf: Fix AT_PHDR for unusual ELF files
  fs/binfmt_elf: Refactor load_elf_binary function

 fs/binfmt_elf.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)


base-commit: 4eee8d0b64ecc3231040fa68ba750317ffca5c52
-- 
2.34.1

[PATCH v4 1/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Akira Kawata]

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=197921

As pointed out in the discussion of buglink, we cannot calculate AT_PHDR
as the sum of load_addr and exec->e_phoff.

: The AT_PHDR of ELF auxiliary vectors should point to the memory address
: of program header. But binfmt_elf.c calculates this address as follows:
:
: NEW_AUX_ENT(AT_PHDR, load_addr + exec->e_phoff);
:
: which is wrong since e_phoff is the file offset of program header and
: load_addr is the memory base address from PT_LOAD entry.
:
: The ld.so uses AT_PHDR as the memory address of program header. In normal
: case, since the e_phoff is usually 64 and in the first PT_LOAD region, it
: is the correct program header address.
:
: But if the address of program header isn't equal to the first PT_LOAD
: address + e_phoff (e.g.  Put the program header in other non-consecutive
: PT_LOAD region), ld.so will try to read program header from wrong address
: then crash or use incorrect program header.

This is because exec->e_phoff
is the offset of PHDRs in the file and the address of PHDRs in the
memory may differ from it. This patch fixes the bug by calculating the
address of program headers from PT_LOADs directly.

Signed-off-by: Akira Kawata <akirakawata1@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 fs/binfmt_elf.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index beeb1247b5c4..828e88841cb4 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -170,8 +170,8 @@ static int padzero(unsigned long elf_bss)
 
 static int
 create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
-		unsigned long load_addr, unsigned long interp_load_addr,
-		unsigned long e_entry)
+		unsigned long interp_load_addr,
+		unsigned long e_entry, unsigned long phdr_addr)
 {
 	struct mm_struct *mm = current->mm;
 	unsigned long p = bprm->p;
@@ -257,7 +257,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
 	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);
@@ -822,7 +822,7 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
 static int load_elf_binary(struct linux_binprm *bprm)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */
- 	unsigned long load_addr = 0, load_bias = 0;
+	unsigned long load_addr, load_bias = 0, phdr_addr = 0;
 	int load_addr_set = 0;
 	unsigned long error;
 	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
@@ -1168,6 +1168,13 @@ static int load_elf_binary(struct linux_binprm *bprm)
 				reloc_func_desc = load_bias;
 			}
 		}
+
+		if (elf_ppnt->p_offset <= elf_ex->e_phoff &&
+		    elf_ex->e_phoff < elf_ppnt->p_offset + elf_ppnt->p_filesz) {
+			phdr_addr = elf_ex->e_phoff - elf_ppnt->p_offset +
+				    elf_ppnt->p_vaddr;
+		}
+
 		k = elf_ppnt->p_vaddr;
 		if ((elf_ppnt->p_flags & PF_X) && k < start_code)
 			start_code = k;
@@ -1203,6 +1210,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	}
 
 	e_entry = elf_ex->e_entry + load_bias;
+	phdr_addr += load_bias;
 	elf_bss += load_bias;
 	elf_brk += load_bias;
 	start_code += load_bias;
@@ -1266,8 +1274,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		goto out;
 #endif /* ARCH_HAS_SETUP_ADDITIONAL_PAGES */
 
-	retval = create_elf_tables(bprm, elf_ex,
-			  load_addr, interp_load_addr, e_entry);
+	retval = create_elf_tables(bprm, elf_ex, interp_load_addr,
+				   e_entry, phdr_addr);
 	if (retval < 0)
 		goto out;
 
-- 
2.34.1

[PATCH v4 2/2] fs/binfmt_elf: Refactor load_elf_binary function

[Akira Kawata]

I delete load_addr because it is not used anymore. And I rename
load_addr_set to first_pt_load because it is used only to capture the
first iteration of the loop.

Signed-off-by: Akira Kawata <akirakawata1@gmail.com>
---
 fs/binfmt_elf.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 828e88841cb4..48206fd1a20e 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -822,8 +822,8 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
 static int load_elf_binary(struct linux_binprm *bprm)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */
-	unsigned long load_addr, load_bias = 0, phdr_addr = 0;
-	int load_addr_set = 0;
+	unsigned long load_bias = 0, phdr_addr = 0;
+	int first_pt_load = 1;
 	unsigned long error;
 	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
 	struct elf_phdr *elf_property_phdata = NULL;
@@ -1073,12 +1073,12 @@ static int load_elf_binary(struct linux_binprm *bprm)
 
 		vaddr = elf_ppnt->p_vaddr;
 		/*
-		 * The first time through the loop, load_addr_set is false:
+		 * The first time through the loop, first_pt_load is true:
 		 * layout will be calculated. Once set, use MAP_FIXED since
 		 * we know we've already safely mapped the entire region with
 		 * MAP_FIXED_NOREPLACE in the once-per-binary logic following.
 		 */
-		if (load_addr_set) {
+		if (!first_pt_load) {
 			elf_flags |= MAP_FIXED;
 		} else if (elf_ex->e_type == ET_EXEC) {
 			/*
@@ -1138,10 +1138,10 @@ static int load_elf_binary(struct linux_binprm *bprm)
 
 		/*
 		 * Calculate the entire size of the ELF mapping (total_size).
-		 * (Note that load_addr_set is set to true later once the
+		 * (Note that first_pt_load is set to false later once the
 		 * initial mapping is performed.)
 		 */
-		if (!load_addr_set) {
+		if (first_pt_load) {
 			total_size = total_mapping_size(elf_phdata,
 							elf_ex->e_phnum);
 			if (!total_size) {
@@ -1158,13 +1158,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			goto out_free_dentry;
 		}
 
-		if (!load_addr_set) {
-			load_addr_set = 1;
-			load_addr = (elf_ppnt->p_vaddr - elf_ppnt->p_offset);
+		if (first_pt_load) {
+			first_pt_load = 0;
 			if (elf_ex->e_type == ET_DYN) {
 				load_bias += error -
 				             ELF_PAGESTART(load_bias + vaddr);
-				load_addr += load_bias;
 				reloc_func_desc = load_bias;
 			}
 		}
-- 
2.34.1

Re: [PATCH v4 0/2] [PATCH v4 0/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Akira Kawata]

On Mon, Dec 13, 2021 at 08:24:13AM +0900, Akira Kawata wrote:
>  These patches fix a bug in AT_PHDR calculation. 
>  
>  We cannot calculate AT_PHDR as the sum of load_addr and exec->e_phoff.
>  This is because exec->e_phoff is the offset of PHDRs in the file and the
>  address of PHDRs in the memory may differ from it. These patches fix the
>  bug by calculating the address of program headers from PT_LOADs
>  directly.
>  
>  Sorry for my latency.
>  
>  Changes in v4
>  - Reflecting comments from Lukas, add a refactoring commit.
>  
>  Changes in v3:
>  - Fix a reported bug from kernel test robot.
>  
>  Changes in v2:
>  - Remove unused load_addr from create_elf_tables.
>  - Improve the commit message.
> 
> Akira Kawata (2):
>   fs/binfmt_elf: Fix AT_PHDR for unusual ELF files
>   fs/binfmt_elf: Refactor load_elf_binary function
> 
>  fs/binfmt_elf.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> 
> base-commit: 4eee8d0b64ecc3231040fa68ba750317ffca5c52
> -- 
> 2.34.1
> 

I am sorry for sending duplicated emails by mistake.
Please ignore the later ones.

Akira

Re: [PATCH v4 1/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Kees Cook]

On Mon, Dec 13, 2021 at 08:24:11AM +0900, Akira Kawata wrote:
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=197921
> 
> As pointed out in the discussion of buglink, we cannot calculate AT_PHDR
> as the sum of load_addr and exec->e_phoff.
> 
> : The AT_PHDR of ELF auxiliary vectors should point to the memory address
> : of program header. But binfmt_elf.c calculates this address as follows:
> :
> : NEW_AUX_ENT(AT_PHDR, load_addr + exec->e_phoff);
> :
> : which is wrong since e_phoff is the file offset of program header and
> : load_addr is the memory base address from PT_LOAD entry.
> :
> : The ld.so uses AT_PHDR as the memory address of program header. In normal
> : case, since the e_phoff is usually 64 and in the first PT_LOAD region, it
> : is the correct program header address.
> :
> : But if the address of program header isn't equal to the first PT_LOAD
> : address + e_phoff (e.g.  Put the program header in other non-consecutive
> : PT_LOAD region), ld.so will try to read program header from wrong address
> : then crash or use incorrect program header.
> 
> This is because exec->e_phoff
> is the offset of PHDRs in the file and the address of PHDRs in the
> memory may differ from it. This patch fixes the bug by calculating the
> address of program headers from PT_LOADs directly.
> 
> Signed-off-by: Akira Kawata <akirakawata1@gmail.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  fs/binfmt_elf.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index beeb1247b5c4..828e88841cb4 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -170,8 +170,8 @@ static int padzero(unsigned long elf_bss)
>  
>  static int
>  create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
> -		unsigned long load_addr, unsigned long interp_load_addr,
> -		unsigned long e_entry)
> +		unsigned long interp_load_addr,
> +		unsigned long e_entry, unsigned long phdr_addr)
>  {
>  	struct mm_struct *mm = current->mm;
>  	unsigned long p = bprm->p;
> @@ -257,7 +257,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
>  	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);
> @@ -822,7 +822,7 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
>  static int load_elf_binary(struct linux_binprm *bprm)
>  {
>  	struct file *interpreter = NULL; /* to shut gcc up */
> - 	unsigned long load_addr = 0, load_bias = 0;
> +	unsigned long load_addr, load_bias = 0, phdr_addr = 0;
>  	int load_addr_set = 0;
>  	unsigned long error;
>  	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
> @@ -1168,6 +1168,13 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  				reloc_func_desc = load_bias;
>  			}
>  		}
> +
> +		if (elf_ppnt->p_offset <= elf_ex->e_phoff &&
> +		    elf_ex->e_phoff < elf_ppnt->p_offset + elf_ppnt->p_filesz) {
> +			phdr_addr = elf_ex->e_phoff - elf_ppnt->p_offset +
> +				    elf_ppnt->p_vaddr;
> +		}

This chunk could really use a comment above it. Maybe something like:

/*
 * Figure out which segment in the file contains the Program
 * Header table, and map to the associated memory address.
 */

Some additional thoughts:

1) The ELF spec says e_phoff is 0 if there's no program header table.

The old code would just pass the load_addr as a result. This patch will
now retain the same result (phdr_addr defaults to 0). I wonder if there
is a bug in this behavior, though? (To be addressed in a different patch
if needed...)

2) This finds any matching segment, not just PT_PHDR, which is good,
since PT_PHDR isn't strictly required.

> +
>  		k = elf_ppnt->p_vaddr;
>  		if ((elf_ppnt->p_flags & PF_X) && k < start_code)
>  			start_code = k;
> @@ -1203,6 +1210,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  	}
>  
>  	e_entry = elf_ex->e_entry + load_bias;
> +	phdr_addr += load_bias;
>  	elf_bss += load_bias;
>  	elf_brk += load_bias;
>  	start_code += load_bias;
> @@ -1266,8 +1274,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  		goto out;
>  #endif /* ARCH_HAS_SETUP_ADDITIONAL_PAGES */
>  
> -	retval = create_elf_tables(bprm, elf_ex,
> -			  load_addr, interp_load_addr, e_entry);
> +	retval = create_elf_tables(bprm, elf_ex, interp_load_addr,
> +				   e_entry, phdr_addr);
>  	if (retval < 0)
>  		goto out;

Looks good!

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v4 2/2] fs/binfmt_elf: Refactor load_elf_binary function

[Kees Cook]

On Mon, Dec 13, 2021 at 08:24:12AM +0900, Akira Kawata wrote:
> I delete load_addr because it is not used anymore. And I rename
> load_addr_set to first_pt_load because it is used only to capture the
> first iteration of the loop.
> 
> Signed-off-by: Akira Kawata <akirakawata1@gmail.com>

Thanks for splitting this out. :)

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v4 1/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Akira Kawata]

On Wed, Jan 26, 2022 at 09:01:30PM -0800, Kees Cook wrote:
> On Mon, Dec 13, 2021 at 08:24:11AM +0900, Akira Kawata wrote:
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=197921
> > 
> > As pointed out in the discussion of buglink, we cannot calculate AT_PHDR
> > as the sum of load_addr and exec->e_phoff.
> > 
> > : The AT_PHDR of ELF auxiliary vectors should point to the memory address
> > : of program header. But binfmt_elf.c calculates this address as follows:
> > :
> > : NEW_AUX_ENT(AT_PHDR, load_addr + exec->e_phoff);
> > :
> > : which is wrong since e_phoff is the file offset of program header and
> > : load_addr is the memory base address from PT_LOAD entry.
> > :
> > : The ld.so uses AT_PHDR as the memory address of program header. In normal
> > : case, since the e_phoff is usually 64 and in the first PT_LOAD region, it
> > : is the correct program header address.
> > :
> > : But if the address of program header isn't equal to the first PT_LOAD
> > : address + e_phoff (e.g.  Put the program header in other non-consecutive
> > : PT_LOAD region), ld.so will try to read program header from wrong address
> > : then crash or use incorrect program header.
> > 
> > This is because exec->e_phoff
> > is the offset of PHDRs in the file and the address of PHDRs in the
> > memory may differ from it. This patch fixes the bug by calculating the
> > address of program headers from PT_LOADs directly.
> > 
> > Signed-off-by: Akira Kawata <akirakawata1@gmail.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
> >  fs/binfmt_elf.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index beeb1247b5c4..828e88841cb4 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -170,8 +170,8 @@ static int padzero(unsigned long elf_bss)
> >  
> >  static int
> >  create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
> > -		unsigned long load_addr, unsigned long interp_load_addr,
> > -		unsigned long e_entry)
> > +		unsigned long interp_load_addr,
> > +		unsigned long e_entry, unsigned long phdr_addr)
> >  {
> >  	struct mm_struct *mm = current->mm;
> >  	unsigned long p = bprm->p;
> > @@ -257,7 +257,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
> >  	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);
> > @@ -822,7 +822,7 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
> >  static int load_elf_binary(struct linux_binprm *bprm)
> >  {
> >  	struct file *interpreter = NULL; /* to shut gcc up */
> > - 	unsigned long load_addr = 0, load_bias = 0;
> > +	unsigned long load_addr, load_bias = 0, phdr_addr = 0;
> >  	int load_addr_set = 0;
> >  	unsigned long error;
> >  	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
> > @@ -1168,6 +1168,13 @@ static int load_elf_binary(struct linux_binprm *bprm)
> >  				reloc_func_desc = load_bias;
> >  			}
> >  		}
> > +
> > +		if (elf_ppnt->p_offset <= elf_ex->e_phoff &&
> > +		    elf_ex->e_phoff < elf_ppnt->p_offset + elf_ppnt->p_filesz) {
> > +			phdr_addr = elf_ex->e_phoff - elf_ppnt->p_offset +
> > +				    elf_ppnt->p_vaddr;
> > +		}
> 
> This chunk could really use a comment above it. Maybe something like:
> 
> /*
>  * Figure out which segment in the file contains the Program
>  * Header table, and map to the associated memory address.
>  */

Thank you. It looks good to me. I made v5 which contains it.

> 
> Some additional thoughts:
> 
> 1) The ELF spec says e_phoff is 0 if there's no program header table.
> 
> The old code would just pass the load_addr as a result. This patch will
> now retain the same result (phdr_addr defaults to 0). I wonder if there
> is a bug in this behavior, though? (To be addressed in a different patch
> if needed...)
>

It is better to return NULL from load_elf_phdrs when e_phoff == 0, I
think.

> 2) This finds any matching segment, not just PT_PHDR, which is good,
> since PT_PHDR isn't strictly required.
> 
> > +
> >  		k = elf_ppnt->p_vaddr;
> >  		if ((elf_ppnt->p_flags & PF_X) && k < start_code)
> >  			start_code = k;
> > @@ -1203,6 +1210,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> >  	}
> >  
> >  	e_entry = elf_ex->e_entry + load_bias;
> > +	phdr_addr += load_bias;
> >  	elf_bss += load_bias;
> >  	elf_brk += load_bias;
> >  	start_code += load_bias;
> > @@ -1266,8 +1274,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
> >  		goto out;
> >  #endif /* ARCH_HAS_SETUP_ADDITIONAL_PAGES */
> >  
> > -	retval = create_elf_tables(bprm, elf_ex,
> > -			  load_addr, interp_load_addr, e_entry);
> > +	retval = create_elf_tables(bprm, elf_ex, interp_load_addr,
> > +				   e_entry, phdr_addr);
> >  	if (retval < 0)
> >  		goto out;
> 
> Looks good!
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> -- 
> Kees Cook

Thank you for your review.

Akira Kawata

Re: [PATCH v4 1/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Kees Cook]

On Thu, Jan 27, 2022 at 09:56:43PM +0900, Akira Kawata wrote:
> On Wed, Jan 26, 2022 at 09:01:30PM -0800, Kees Cook wrote:
> > [...]
> > 1) The ELF spec says e_phoff is 0 if there's no program header table.
> > 
> > The old code would just pass the load_addr as a result. This patch will
> > now retain the same result (phdr_addr defaults to 0). I wonder if there
> > is a bug in this behavior, though? (To be addressed in a different patch
> > if needed...)
> >
> 
> It is better to return NULL from load_elf_phdrs when e_phoff == 0, I
> think.

Yeah, right now it just returns a pointer to file offset 0.

I also wonder if we should sanity-check e_phoff vs PT_PHDR? Right now
Linux ignores PT_PHDR. Should we reject loading when e_phoff != PT_PHDR
file offset? (And I wonder if there are "broken" binaries right now that
have bad PT_PHDR segments that have gone unnoticed...)

And now I'm thinking about the excellent ELF loading analysis at:
https://nathanotterness.com/2021/10/tiny_elf_modernized.html

;)

-- 
Kees Cook

Re: [PATCH v4 1/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Akira Kawata]

On Thu, Jan 27, 2022 at 08:23:51AM -0800, Kees Cook wrote:
> On Thu, Jan 27, 2022 at 09:56:43PM +0900, Akira Kawata wrote:
> > On Wed, Jan 26, 2022 at 09:01:30PM -0800, Kees Cook wrote:
> > > [...]
> > > 1) The ELF spec says e_phoff is 0 if there's no program header table.
> > > 
> > > The old code would just pass the load_addr as a result. This patch will
> > > now retain the same result (phdr_addr defaults to 0). I wonder if there
> > > is a bug in this behavior, though? (To be addressed in a different patch
> > > if needed...)
> > >
> > 
> > It is better to return NULL from load_elf_phdrs when e_phoff == 0, I
> > think.
> 
> Yeah, right now it just returns a pointer to file offset 0.
> 
> I also wonder if we should sanity-check e_phoff vs PT_PHDR? Right now
> Linux ignores PT_PHDR. Should we reject loading when e_phoff != PT_PHDR
> file offset? (And I wonder if there are "broken" binaries right now that
> have bad PT_PHDR segments that have gone unnoticed...)

I agree that unnoticed broken binaries exist. I checked glibc rtld and
there is no check of e_phoff != PT_PHDR file offset.

> 
> And now I'm thinking about the excellent ELF loading analysis at:
> https://nathanotterness.com/2021/10/tiny_elf_modernized.html
> 
> ;)

I think you have interested in https://shinh.skr.jp/obf/bingolf.html
also.

> 
> -- 
> Kees Cook

Akira Kawata

Re: [PATCH v4 1/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files

[Kees Cook]

On Fri, Jan 28, 2022 at 08:10:34PM +0900, Akira Kawata wrote:
> On Thu, Jan 27, 2022 at 08:23:51AM -0800, Kees Cook wrote:
> > And now I'm thinking about the excellent ELF loading analysis at:
> > https://nathanotterness.com/2021/10/tiny_elf_modernized.html
> > 
> > ;)
> 
> I think you have interested in https://shinh.skr.jp/obf/bingolf.html
> also.

Ah, nice! The 104b one great. :)

-- 
Kees Cook