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

[Linus Torvalds]

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.

             Linus