From: Kees Cook <keescook@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>,
Hector Marco-Gisbert <hecmargi@upv.es>,
Ismael Ripoll Ripoll <iripoll@upv.es>,
Alexander Viro <viro@zeniv.linux.org.uk>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Oleg Nesterov <oleg@redhat.com>,
Chen Gang <gang.chen.5i5j@gmail.com>,
Michal Hocko <mhocko@suse.com>,
Konstantin Khlebnikov <koct9i@gmail.com>,
Andrea Arcangeli <aarcange@redhat.com>,
Andrey Ryabinin <aryabinin@virtuozzo.com>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
Linux-MM <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] binfmt_elf: fix calculations for bss padding
Date: Tue, 12 Jul 2016 18:32:43 -0400 [thread overview]
Message-ID: <CAGXu5j+FJiVQr6VomDUTJjCo8OgLyg6LGhgGQQwO5verj5UC=Q@mail.gmail.com> (raw)
In-Reply-To: <1468014494-25291-2-git-send-email-keescook@chromium.org>
On Fri, Jul 8, 2016 at 5:48 PM, Kees Cook <keescook@chromium.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@upv.es>
> Signed-off-by: Kees Cook <keescook@chromium.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
next prev parent reply other threads:[~2016-07-12 22:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-08 21:48 [PATCH 0/2] binfmt_elf: fix calculations for bss padding Kees Cook
2016-07-08 21:48 ` [PATCH 1/2] " Kees Cook
2016-07-12 22:32 ` Kees Cook [this message]
2016-07-08 21:48 ` [PATCH 2/2] mm: refuse wrapped vm_brk requests Kees Cook
2016-07-11 12:28 ` Oleg Nesterov
2016-07-11 18:01 ` Kees Cook
2016-07-12 13:39 ` Oleg Nesterov
2016-07-12 17:15 ` Kees Cook
2016-07-13 17:00 ` Oleg Nesterov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGXu5j+FJiVQr6VomDUTJjCo8OgLyg6LGhgGQQwO5verj5UC=Q@mail.gmail.com' \
--to=keescook@chromium.org \
--cc=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=aryabinin@virtuozzo.com \
--cc=gang.chen.5i5j@gmail.com \
--cc=hecmargi@upv.es \
--cc=iripoll@upv.es \
--cc=kirill.shutemov@linux.intel.com \
--cc=koct9i@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=oleg@redhat.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).