* [RESEND,PATCH] fs/binfmt_elf: Fix regression limiting ELF program header size
@ 2020-12-15 3:46 Joshua Bakita
2021-02-11 20:27 ` Joshua Bakita
0 siblings, 1 reply; 3+ messages in thread
From: Joshua Bakita @ 2020-12-15 3:46 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, Joshua Bakita
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 */
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RESEND,PATCH] fs/binfmt_elf: Fix regression limiting ELF program header size
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
0 siblings, 1 reply; 3+ messages in thread
From: Joshua Bakita @ 2021-02-11 20:27 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel
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 */
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RESEND,PATCH] fs/binfmt_elf: Fix regression limiting ELF program header size
2021-02-11 20:27 ` Joshua Bakita
@ 2021-04-11 21:53 ` Joshua Bakita
0 siblings, 0 replies; 3+ messages in thread
From: Joshua Bakita @ 2021-04-11 21:53 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel
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 */
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-11 21:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 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).