On Fri, Jul 8, 2016 at 5:48 PM, Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
