linux-kernel - Re: [PATCH 1/2] binfmt_elf: fix calculations for bss padding

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Jul 2016 18:32:43 -0400
From:	Kees Cook <keescook@...omium.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Kees Cook <keescook@...omium.org>,
	Hector Marco-Gisbert <hecmargi@....es>,
	Ismael Ripoll Ripoll <iripoll@....es>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Chen Gang <gang.chen.5i5j@...il.com>,
	Michal Hocko <mhocko@...e.com>,
	Konstantin Khlebnikov <koct9i@...il.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	Linux-MM <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] binfmt_elf: fix calculations for bss padding

On Fri, Jul 8, 2016 at 5:48 PM, Kees Cook <keescook@...omium.org> wrote:
> A double-bug exists in the bss calculation code, where an overflow can
> happen in the "last_bss - elf_bss" calculation, but vm_brk internally
> aligns the argument, underflowing it, wrapping back around safe. We
> shouldn't depend on these bugs staying in sync, so this cleans up the bss
> padding handling to avoid the overflow.
>
> This moves the bss padzero() before the last_bss > elf_bss case, since
> the zero-filling of the ELF_PAGE should have nothing to do with the
> relationship of last_bss and elf_bss: any trailing portion should be
> zeroed, and a zero size is already handled by padzero().
>
> Then it handles the math on elf_bss vs last_bss correctly. These need
> to both be ELF_PAGE aligned to get the comparison correct, since that's
> the expected granularity of the mappings. Since elf_bss already had
> alignment-based padding happen in padzero(), the "start" of the new
> vm_brk() should be moved forward as done in the original code. However,
> since the "end" of the vm_brk() area will already become PAGE_ALIGNed in
> vm_brk() then last_bss should get aligned here to avoid hiding it as a
> side-effect.
>
> Additionally makes a cosmetic change to the initial last_bss calculation
> so it's easier to read in comparison to the load_addr calculation above it
> (i.e. the only difference is p_filesz vs p_memsz).
>
> Reported-by: Hector Marco-Gisbert <hecmargi@....es>
> Signed-off-by: Kees Cook <keescook@...omium.org>

Andrew or Al, can you pick this up for -next? It doesn't depend on the
do_brk() fix (patch 2/2)...

-Kees

> ---
>  fs/binfmt_elf.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index e158b22ef32f..fe948933bcc5 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -605,28 +605,30 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
>                          * Do the same thing for the memory mapping - between
>                          * elf_bss and last_bss is the bss section.
>                          */
> -                       k = load_addr + eppnt->p_memsz + eppnt->p_vaddr;
> +                       k = load_addr + eppnt->p_vaddr + eppnt->p_memsz;
>                         if (k > last_bss)
>                                 last_bss = k;
>                 }
>         }
>
> +       /*
> +        * Now fill out the bss section: first pad the last page from
> +        * the file up to the page boundary, and zero it from elf_bss
> +        * up to the end of the page.
> +        */
> +       if (padzero(elf_bss)) {
> +               error = -EFAULT;
> +               goto out;
> +       }
> +       /*
> +        * Next, align both the file and mem bss up to the page size,
> +        * since this is where elf_bss was just zeroed up to, and where
> +        * last_bss will end after the vm_brk() below.
> +        */
> +       elf_bss = ELF_PAGEALIGN(elf_bss);
> +       last_bss = ELF_PAGEALIGN(last_bss);
> +       /* Finally, if there is still more bss to allocate, do it. */
>         if (last_bss > elf_bss) {
> -               /*
> -                * Now fill out the bss section.  First pad the last page up
> -                * to the page boundary, and then perform a mmap to make sure
> -                * that there are zero-mapped pages up to and including the
> -                * last bss page.
> -                */
> -               if (padzero(elf_bss)) {
> -                       error = -EFAULT;
> -                       goto out;
> -               }
> -
> -               /* What we have mapped so far */
> -               elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
> -
> -               /* Map the last of the bss segment */
>                 error = vm_brk(elf_bss, last_bss - elf_bss);
>                 if (error)
>                         goto out;
> --
> 2.7.4
>



-- 
Kees Cook
Chrome OS & Brillo Security