[1/4] elf: don't be afraid of overflow
diff mbox series

Message ID 20190204202715.GA27482@avx2
State New, archived
Headers show
Series
  • [1/4] elf: don't be afraid of overflow
Related show

Commit Message

Alexey Dobriyan Feb. 4, 2019, 8:27 p.m. UTC
Number of ELF program headers is 16-bit by spec, so total size
comfortably fits into "unsigned int".

Space savings: 7 bytes!

	add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-7 (-7)
	Function                                     old     new   delta
	load_elf_phdrs                               137     130      -7

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/binfmt_elf.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Pavel Machek March 11, 2019, 11:04 a.m. UTC | #1
On Mon 2019-02-04 23:27:15, Alexey Dobriyan wrote:
> Number of ELF program headers is 16-bit by spec, so total size
> comfortably fits into "unsigned int".

If it can't overflow, gcc should know too, and optimize checks
out... right?

> @@ -429,13 +430,9 @@ static struct elf_phdr *load_elf_phdrs(struct elfhdr *elf_ex,
>  		goto out;
>  
>  	/* 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. */

This is just wrong. You removed check for zero, and I'm pretty sure
sizeof() is not 1, so this one can trigger, too.
									Pavel
Alexey Dobriyan March 11, 2019, 5:10 p.m. UTC | #2
On Mon, Mar 11, 2019 at 12:04:23PM +0100, Pavel Machek wrote:
> On Mon 2019-02-04 23:27:15, Alexey Dobriyan wrote:
> > Number of ELF program headers is 16-bit by spec, so total size
> > comfortably fits into "unsigned int".
> 
> If it can't overflow, gcc should know too, and optimize checks
> out... right?

Turns out it doesn't.

> > @@ -429,13 +430,9 @@ static struct elf_phdr *load_elf_phdrs(struct elfhdr *elf_ex,
> >  		goto out;
> >  
> >  	/* 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. */
> 
> This is just wrong. You removed check for zero, and I'm pretty sure
> sizeof() is not 1, so this one can trigger, too.

No. ->e_phnum is 65535 max.

	size = sizeof(struct elf_phdr) * elf_ex->e_phnum;
        if (size == 0 || size > 65536 || size > ELF_MIN_ALIGN)
                goto out;
Pavel Machek March 12, 2019, 9:37 a.m. UTC | #3
On Mon 2019-03-11 20:10:06, Alexey Dobriyan wrote:
> On Mon, Mar 11, 2019 at 12:04:23PM +0100, Pavel Machek wrote:
> > On Mon 2019-02-04 23:27:15, Alexey Dobriyan wrote:
> > > Number of ELF program headers is 16-bit by spec, so total size
> > > comfortably fits into "unsigned int".
> > 
> > If it can't overflow, gcc should know too, and optimize checks
> > out... right?
> 
> Turns out it doesn't.

Tells me you are doing something wrong.

> > > @@ -429,13 +430,9 @@ static struct elf_phdr *load_elf_phdrs(struct elfhdr *elf_ex,
> > >  		goto out;
> > >  
> > >  	/* 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. */
> > 
> > This is just wrong. You removed check for zero, and I'm pretty sure
> > sizeof() is not 1, so this one can trigger, too.
> 
> No. ->e_phnum is 65535 max.

That does not invalidate my argument.

									Pavel

Patch
diff mbox series

--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -418,8 +418,9 @@  static struct elf_phdr *load_elf_phdrs(struct elfhdr *elf_ex,
 				       struct file *elf_file)
 {
 	struct elf_phdr *elf_phdata = NULL;
-	int retval, size, err = -1;
+	int retval, err = -1;
 	loff_t pos = elf_ex->e_phoff;
+	unsigned int size;
 
 	/*
 	 * If the size of this structure has changed, then punt, since
@@ -429,13 +430,9 @@  static struct elf_phdr *load_elf_phdrs(struct elfhdr *elf_ex,
 		goto out;
 
 	/* 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)
+	if (size == 0 || size > 65536 || size > ELF_MIN_ALIGN)
 		goto out;
 
 	elf_phdata = kmalloc(size, GFP_KERNEL);