linux-kernel - Re: [PATCH v3 4/5] binfmt_elf, binfmt_elf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez3pMcPTHrbgjeVbCAV1n7VQW1tqJw8kNsL4wgRxV_Fr9Q@mail.gmail.com>
Date:   Tue, 18 Aug 2020 10:31:20 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Christoph Hellwig <hch@....de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH v3 4/5] binfmt_elf, binfmt_elf_fdpic: Use a VMA list snapshot

On Tue, Aug 18, 2020 at 10:18 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Mon, Aug 17, 2020 at 11:13 PM Jann Horn <jannh@...gle.com> 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.