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 */
>>
prev parent 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).