| 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
| ||
|
Date: Thu, 27 Jan 2022 21:56:43 +0900
From: Akira Kawata <akirakawata1@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: akpm@...ux-foundation.org, adobriyan@...il.com,
viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org,
lukas.bulwahn@...il.com, kernel test robot <lkp@...el.com>,
Eric Biederman <ebiederm@...ssion.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] fs/binfmt_elf: Fix AT_PHDR for unusual ELF files
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@...il.com>
> > Reported-by: kernel test robot <lkp@...el.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@...omium.org>
>
> --
> Kees Cook
Thank you for your review.
Akira Kawata
Powered by blists - more mailing lists