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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <fbcd2886-d505-f14e-ad6b-c4b5fa77707e@cs.unc.edu>
Date:   Sun, 11 Apr 2021 17:53:59 -0400
From:   Joshua Bakita <jbakita@...unc.edu>
To:     viro@...iv.linux.org.uk
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RESEND,PATCH] fs/binfmt_elf: Fix regression limiting ELF program
 header size

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