From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Hogan Subject: Re: [PATCH] binfmt_elf: Fix bug in loading of PIE binaries. Date: Tue, 19 May 2015 16:01:00 +0100 Message-ID: References: <1428965343-17762-1-git-send-email-md@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Alexander Viro , Jiri Kosina , Andrew Morton , linux-fsdevel@vger.kernel.org, LKML , James Hogan To: Michael Davidson Return-path: In-Reply-To: <1428965343-17762-1-git-send-email-md@google.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org Hi Michael, On 13 April 2015 at 23:49, Michael Davidson 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 > --- > 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/