linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Joshua Bakita <jbakita@cs.unc.edu>
To: viro@zeniv.linux.org.uk
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RESEND,PATCH] fs/binfmt_elf: Fix regression limiting ELF program header size
Date: Sun, 11 Apr 2021 17:53:59 -0400	[thread overview]
Message-ID: <fbcd2886-d505-f14e-ad6b-c4b5fa77707e@cs.unc.edu> (raw)
In-Reply-To: <7cba1c24-5034-53e1-6014-982973e66ea3@cs.unc.edu>

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@cs.unc.edu>
>> ---
>>   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 */
>>

      reply	other threads:[~2021-04-11 21:54 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-15  3:46 [RESEND,PATCH] fs/binfmt_elf: Fix regression limiting ELF program header size Joshua Bakita
2021-02-11 20:27 ` Joshua Bakita
2021-04-11 21:53   ` Joshua Bakita [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fbcd2886-d505-f14e-ad6b-c4b5fa77707e@cs.unc.edu \
    --to=jbakita@cs.unc.edu \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).