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.
