Re: [RESEND,PATCH] fs/binfmt_elf: Fix regression limiting ELF program header size

[Joshua Bakita]

Hello,

I'd greatly appreciate it if this patch would be reviewed. It's been 
nearly 6 months since I first submitted it, there's clear evidence that 
this regression effects real programs, and the fix is simple. If no 
maintainers on this list have the time to review this change, I would 
appreciate suggestions on alternative lists and/or maintainers who I 
could reach out to instead.

Best,

Joshua Bakita

On 2/11/21 3:27 PM, Joshua Bakita wrote:
> Hello all,
>
> I raised this patch on #linuxfs on IRC, and I got asked if this actually 
> effects real programs. To demonstrate that it does, I wrote up a simple 
> C program which just does a table lookup of a prime number. The table is 
> stored sparsely, so newer versions of GCC+LD automatically put each 
> table entry in its own program section and segment. This results in over 
> 100 ELF program header entries, which Linux since 3.19 will refuse to 
> load with ENOEXEC due to the errant limit fixed in my patch. (The 
> current broken limit is 73, whereas the manpage states a limit of 64k.)
>
> My example program is available at 
> https://www.cs.unc.edu/~jbakita/get_prime.c and should be built as gcc 
> get_prime.c -o get_prime. I know this works with GCC 9.3.0 and LD 2.34 
> (GCC 7.5.0 and LD 2.30 are too old). You can verify it built correctly 
> by checking the "Number of program headers" as printed by readelf -h is 
> at least 100.
>
> I tried to keep this patch small to make it easy to review, but there 
> are a few other bugs (like the 64KB limit) in the ELF loader. Would it 
> be more helpful or make review easier to just fix all the bugs at once? 
> This is my first kernel patch, and I'd really like to make it the first 
> of many.
>
> Best,
>
> Joshua Bakita
>
> On 12/14/20 10:46 PM, Joshua Bakita wrote:
> > Commit 6a8d38945cf4 ("binfmt_elf: Hoist ELF program header loading to a
> > function") merged load_elf_binary and load_elf_interp into
> > load_elf_phdrs. This change imposed a limit that the program headers of
> > all ELF binaries are smaller than ELF_MIN_ALIGN. This is a mistake for
> > two reasons:
> > 1. load_elf_binary previously had no such constraint, meaning that
> >     previously valid ELF program headers are now rejected by the 
> > kernel as
> >     oversize and invalid.
> > 2. The ELF interpreter's program headers should never have been 
> > limited to
> >     ELF_MIN_ALIGN (and previously PAGE_SIZE) in the first place. Commit
> >     057f54fbba73 ("Import 1.1.54") introduced this limit to the ELF
> >     interpreter alongside the initial ELF parsing support without any
> >     explanation.
> > This patch removes the ELF_MIN_ALIGN size constraint in favor of only
> > relying on an earlier check that the allocation will be less than 64KiB.
> > (It's worth mentioning that the 64KiB limit is also unnecessarily strict,
> > but that's not addressed here for simplicity. The ELF manpage says that
> > the program header size is supposed to have at most 64 thousand entries,
> > not less than 64 thousand bytes.)
> >
> > Fixes: 6a8d38945cf4 ("binfmt_elf: Hoist ELF program header loading to 
> > a function")
> > Signed-off-by: Joshua Bakita <jbakita@xxxxxxxxxx>
> > ---
> >   fs/binfmt_elf.c | 4 ----
> >   1 file changed, 4 deletions(-)
> >
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 2472af2798c7..55162056590f 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -412,15 +412,11 @@ static struct elf_phdr *load_elf_phdrs(struct 
> > elfhdr *elf_ex,
> >       /* Sanity check the number of program headers... */
> >       if (elf_ex->e_phnum < 1 ||
> >           elf_ex->e_phnum > 65536U / sizeof(struct elf_phdr))
> >           goto out;
> > -    /* ...and their total size. */
> >       size = sizeof(struct elf_phdr) * elf_ex->e_phnum;
> > -    if (size > ELF_MIN_ALIGN)
> > -        goto out;
> > -
> >       elf_phdata = kmalloc(size, GFP_KERNEL);
> >       if (!elf_phdata)
> >           goto out;
> >       /* Read in the program headers */