Re: [PATCH] binfmt_elf: Fix bug in loading of PIE binaries.

[James Hogan <james.hogan@imgtec.com>]

Hi Michael,

On 13 April 2015 at 23:49, Michael Davidson <md@google.com> wrote:
> With CONFIG_ARCH_BINFMT_ELF_RANDOMIZE_PIE enabled, and a normal
> top-down address allocation strategy, load_elf_binary() will
> attempt to map a PIE binary into an address range immediately
> below mm->mmap_base.
>
> Unfortunately, load_elf_ binary() does not take account of the
> need to allocate sufficient space for the entire binary which
> means that, while the first PT_LOAD segment is mapped below
> mm->mmap_base, the subsequent PT_LOAD segment(s) end up being
> mapped above mm->mmap_base into the are that is supposed to
> be the "gap" between the stack and the binary.
>
> Since the size of the "gap" on x86_64 is only guaranteed to be
> 128MB this means that binaries with large data segments > 128MB
> can end up mapping part of their data segment over their stack
> resulting in corruption of the stack (and the data segment once
> the binary starts to run).
>
> Any PIE binary with a data segment > 128MB is vulnerable to this
> although address randomization means that the actual gap between
> the stack and the end of the binary is normally greater than 128MB.
> The larger the data segment of the binary the higher the probability
> of failure.
>
> Fix this by calculating the total size of the binary in the same
> way as load_elf_interp().
>
> Signed-off-by: Michael Davidson <md@google.com>
> ---
>  fs/binfmt_elf.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 995986b..d925f55 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -862,6 +862,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
>             i < loc->elf_ex.e_phnum; i++, elf_ppnt++) {
>                 int elf_prot = 0, elf_flags;
>                 unsigned long k, vaddr;
> +               unsigned long total_size = 0;
>
>                 if (elf_ppnt->p_type != PT_LOAD)
>                         continue;
> @@ -924,10 +925,16 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  #else
>                         load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #endif
> +                       total_size = total_mapping_size(elf_phdata,
> +                                                       loc->elf_ex.e_phnum);
> +                       if (!total_size) {
> +                               error = -EINVAL;

I was just printk debugging this function, and this stood out. Should
that be retval instead of error?

Cheers
James

> +                               goto out_free_dentry;
> +                       }
>                 }
>
>                 error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt,
> -                               elf_prot, elf_flags, 0);
> +                               elf_prot, elf_flags, total_size);
>                 if (BAD_ADDR(error)) {
>                         retval = IS_ERR((void *)error) ?
>                                 PTR_ERR((void*)error) : -EINVAL;
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/