Re: [PATCH v3 4/5] binfmt_elf, binfmt_elf_fdpic: Use a VMA list snapshot

[Jann Horn]

On Tue, Aug 18, 2020 at 10:18 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Aug 17, 2020 at 11:13 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
> >
> >         /*
> >          * If this looks like the beginning of a DSO or executable mapping,
> > +        * we'll check for an ELF header. If we find one, we'll dump the first
> > +        * page to aid in determining what was mapped here.
> > +        * However, we shouldn't sleep on userspace reads while holding the
> > +        * mmap_lock, so we just return a placeholder for now that will be fixed
> > +        * up later in vma_dump_size_fixup().
>
> I still don't like this.
>
> And I still don't think it's necessary.
>
> The whole - and only - point of "check if it's an ELF header" is that
> we don't want to dump data that could just be found by looking at the
> original binary.
>
> But by the time we get to this point, we already know that
>
>  (a) it's a private mapping with file backing, and it's the first page
> of the file
>
>  (b) it has never been written to and it's mapped for reading
>
> and the choice at this point is "don't dump at all", or "dump just the
> first page".
>
> And honestly, that whole "check if it has the ELF header" signature
> was always just a heuristic. Nothing should depend on it anyway.
>
> We already skip dumping file data under a lot of other circumstances
> (and perhaps equally importantly, we already decided to dump it all
> under other circumstances).
>
> I think this DUMP_SIZE_MAYBE_ELFHDR_PLACEHOLDER hackery is worse than
> just changing the heuristic.
>
> So instead, just say "ok, if the file was executable, let's dump the
> first page".
>
> The test might be as simple as jjust checking
>
>        if (file_inode(vma->vm_file)->i_mode & 0111)
>
> and you'd be done. That's likely a _better_ heuristic than the "let's
> go look at the random first word in memory".
>
> Your patches look otherwise fine, but I really really despise that
> DUMP_SIZE_MAYBE_ELFHDR_PLACEHOLDER, and I don't think it's even
> necessary.

Yeah, good point, it's a pretty ugly hack. I'll make a new version
along the lines of what you suggested.