[v5,0/7] Fix ELF / FDPIC ELF core dumping, and use mmap_lock properly in there

Message

Jann Horn Aug. 27, 2020, 11:49 a.m. UTC

new in v5:
 - patches 1-3 and 6 are unchanged
 - added patch 4: rework vma_dump_size() into a common helper (Linus)
 - added patch 7: actually do the mmget_still_valid() removal (Linus)
 - for now, let dump_vma_snapshot() take the mmap_lock in write mode
   instead of read mode to avoid the data race with stack expansion

new in v4:
 - simplify patch 4/5 by replacing the heuristic for dumping the first
   pages of ELF mappings with what Linus suggested


At the moment, we have that rather ugly mmget_still_valid() helper to
work around <https://crbug.com/project-zero/1790>: ELF core dumping
doesn't take the mmap_sem while traversing the task's VMAs, and if
anything (like userfaultfd) then remotely messes with the VMA tree,
fireworks ensue. So at the moment we use mmget_still_valid() to bail
out in any writers that might be operating on a remote mm's VMAs.

With this series, I'm trying to get rid of the need for that as
cleanly as possible. ("cleanly" meaning "avoid holding the mmap_lock
across unbounded sleeps".)


Patches 1, 2, 3 and 4 are relatively unrelated cleanups in the core
dumping code.

Patches 5 and 6 implement the main change: Instead of repeatedly
accessing the VMA list with sleeps in between, we snapshot it at the
start with proper locking, and then later we just use our copy of
the VMA list. This ensures that the kernel won't crash, that VMA
metadata in the coredump is consistent even in the presence of
concurrent modifications, and that any virtual addresses that aren't
being concurrently modified have their contents show up in the core
dump properly.

The disadvantage of this approach is that we need a bit more memory
during core dumping for storing metadata about all VMAs.

At the end of the series, patch 7 removes the old workaround for
this issue (mmget_still_valid()).


I have tested:

 - Creating a simple core dump on X86-64 still works.
 - The created coredump on X86-64 opens in GDB and looks plausible.
 - X86-64 core dumps contain the first page for executable mappings at
   offset 0, and don't contain the first page for non-executable file
   mappings or executable mappings at offset !=0.
 - NOMMU 32-bit ARM can still generate plausible-looking core dumps
   through the FDPIC implementation. (I can't test this with GDB because
   GDB is missing some structure definition for nommu ARM, but I've
   poked around in the hexdump and it looked decent.)

Jann Horn (7):
  binfmt_elf_fdpic: Stop using dump_emit() on user pointers on !MMU
  coredump: Let dump_emit() bail out on short writes
  coredump: Refactor page range dumping into common helper
  coredump: Rework elf/elf_fdpic vma_dump_size() into common helper
  binfmt_elf, binfmt_elf_fdpic: Use a VMA list snapshot
  mm/gup: Take mmap_lock in get_dump_page()
  mm: Remove the now-unnecessary mmget_still_valid() hack

 drivers/infiniband/core/uverbs_main.c |   3 -
 drivers/vfio/pci/vfio_pci.c           |  38 ++--
 fs/binfmt_elf.c                       | 238 +++-----------------------
 fs/binfmt_elf_fdpic.c                 | 162 +++---------------
 fs/coredump.c                         | 236 +++++++++++++++++++++++--
 fs/proc/task_mmu.c                    |  18 --
 fs/userfaultfd.c                      |  28 +--
 include/linux/coredump.h              |  11 ++
 include/linux/sched/mm.h              |  25 ---
 mm/gup.c                              |  61 +++----
 mm/khugepaged.c                       |   2 +-
 mm/madvise.c                          |  17 --
 mm/mmap.c                             |   5 +-
 13 files changed, 346 insertions(+), 498 deletions(-)


base-commit: 06a4ec1d9dc652e17ee3ac2ceb6c7cf6c2b75cdd

Comments

Linus Torvalds Aug. 27, 2020, 5:15 p.m. UTC | #1

On Thu, Aug 27, 2020 at 4:49 AM Jann Horn <[e-mail]> wrote:
>
>  13 files changed, 346 insertions(+), 498 deletions(-)

Me likey. I had one comment, but I don't think it really matters for
this series. So ack to all of these as far as I'm concerned.

Does anybody else see any problems?

           Linus