[PATCH] Bug fix to ELF Loader which rejects valid ELFs

[PATCH] Bug fix to ELF Loader which rejects valid ELFs

[Burrow, Ryan - 0553 - MITLL]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]


Check was incorrectly being applied to size of elf phdrs, instead
of the number. The ELF standard allows for up to 65535 headers, but
the check was being compared to the number of headers multiplied by
the size of a program header.
---
 fs/binfmt_elf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 9fe3b51c116a..5605b5205f84 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -445,11 +445,11 @@ static struct elf_phdr *load_elf_phdrs(const struct
elfhdr *elf_ex,
 		goto out;
 
 	/* Sanity check the number of program headers... */
-	/* ...and their total size. */
-	size = sizeof(struct elf_phdr) * elf_ex->e_phnum;
-	if (size == 0 || size > 65536 || size > ELF_MIN_ALIGN)
+	if (elf_ex->e_phnum == 0 || elf_ex->phnum > 65535)
 		goto out;
 
+	size = sizeof(struct elf_phdr) * elf_ex->e_phnum;
+
 	elf_phdata = kmalloc(size, GFP_KERNEL);
 	if (!elf_phdata)
 		goto out;
-- 
2.17.1

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5529 bytes --]

Re: [PATCH] Bug fix to ELF Loader which rejects valid ELFs

[Matthew Wilcox]

On Tue, Aug 11, 2020 at 02:44:08PM +0000, Burrow, Ryan - 0553 - MITLL wrote:
>  	/* Sanity check the number of program headers... */
> -	/* ...and their total size. */
> -	size = sizeof(struct elf_phdr) * elf_ex->e_phnum;
> -	if (size == 0 || size > 65536 || size > ELF_MIN_ALIGN)
> +	if (elf_ex->e_phnum == 0 || elf_ex->phnum > 65535)

umm, did you compile-test this?

assuming you meant e_phnum, it's a 16-bit quantity, so it can't be bigger
than 65535.

>  		goto out;
>  
> +	size = sizeof(struct elf_phdr) * elf_ex->e_phnum;

use array_size() here?

RE: [PATCH] Bug fix to ELF Loader which rejects valid ELFs

[Burrow, Ryan - 0553 - MITLL]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

> -----Original Message-----
> From: Matthew Wilcox <willy@infradead.org>
> Sent: Tuesday, August 11, 2020 11:05 AM
> To: Burrow, Ryan - 0553 - MITLL <Ryan.Burrow@ll.mit.edu>
> Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Bug fix to ELF Loader which rejects valid ELFs
> 
> On Tue, Aug 11, 2020 at 02:44:08PM +0000, Burrow, Ryan - 0553 - MITLL
wrote:
> >  	/* Sanity check the number of program headers... */
> > -	/* ...and their total size. */
> > -	size = sizeof(struct elf_phdr) * elf_ex->e_phnum;
> > -	if (size == 0 || size > 65536 || size > ELF_MIN_ALIGN)
> > +	if (elf_ex->e_phnum == 0 || elf_ex->phnum > 65535)
> 
> umm, did you compile-test this?
> 

My apologies, I had made these edits in the context of other changes which I
didn't want to include in this patch - I replicated these changes
individually and (mistakenly) assumed I had done so correctly.

> assuming you meant e_phnum, it's a 16-bit quantity, so it can't be bigger
> than 65535.
> 
> >  		goto out;
> >
> > +	size = sizeof(struct elf_phdr) * elf_ex->e_phnum;
> 
> use array_size() here?
> 

That's a good catch - I had really just moved the previous allocation of
size down to after the check, but I'll use array_size instead. Just to
confirm the correct process, should I do an inline response with the updated
commit, or submit a new patch email?


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5529 bytes --]