| Message ID | 87iltzn3nd.fsf_-_@email.froward.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix fill_files_note | expand |
On Mon, Jan 31, 2022 at 7:47 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > Matthew Wilcox reported that there is a missing mmap_lock in > file_files_note that could possibly lead to a user after free. > > Solve this by using the existing vma snapshot for consistency > and to avoid the need to take the mmap_lock anywhere in the > coredump code except for dump_vma_snapshot. > > Update the dump_vma_snapshot to capture vm_pgoff and vm_file > that are neeeded by fill_files_note. > > Add free_vma_snapshot to free the captured values of vm_file. > > Reported-by: Matthew Wilcox <willy@infradead.org> > Link: https://lkml.kernel.org/r/20220131153740.2396974-1-willy@infradead.org > Cc: stable@vger.kernel.org > Fixes: a07279c9a8cd ("binfmt_elf, binfmt_elf_fdpic: use a VMA list snapshot") > Fixes: 2aa362c49c31 ("coredump: extend core dump note section to contain file names of mapped files") > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> [...] > +static int fill_files_note(struct memelfnote *note, struct coredump_params *cprm) > { > struct mm_struct *mm = current->mm; > - struct vm_area_struct *vma; > unsigned count, size, names_ofs, remaining, n; > user_long_t *data; > user_long_t *start_end_ofs; > char *name_base, *name_curpos; > + int i; > > /* *Estimated* file count and total data size needed */ > count = mm->map_count; This function is still looking at mm->map_count in two spots, please change those spots to also look at cprm->vma_count. In particular the second one looks like it can cause memory corruption if the map_count changed since we created the snapshot. [...] > +static void free_vma_snapshot(struct coredump_params *cprm) > +{ > + if (cprm->vma_meta) { > + int i; > + for (i = 0; i < cprm->vma_count; i++) { > + struct file *file = cprm->vma_meta[i].file; > + if (file) > + fput(file); > + } > + kvfree(cprm->vma_meta); > + cprm->vma_meta = NULL; (this NULL write is superfluous, but it also doesn't hurt) > + } > +}
Jann Horn <jannh@google.com> writes: > On Mon, Jan 31, 2022 at 7:47 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >> Matthew Wilcox reported that there is a missing mmap_lock in >> file_files_note that could possibly lead to a user after free. >> >> Solve this by using the existing vma snapshot for consistency >> and to avoid the need to take the mmap_lock anywhere in the >> coredump code except for dump_vma_snapshot. >> >> Update the dump_vma_snapshot to capture vm_pgoff and vm_file >> that are neeeded by fill_files_note. >> >> Add free_vma_snapshot to free the captured values of vm_file. >> >> Reported-by: Matthew Wilcox <willy@infradead.org> >> Link: https://lkml.kernel.org/r/20220131153740.2396974-1-willy@infradead.org >> Cc: stable@vger.kernel.org >> Fixes: a07279c9a8cd ("binfmt_elf, binfmt_elf_fdpic: use a VMA list snapshot") >> Fixes: 2aa362c49c31 ("coredump: extend core dump note section to contain file names of mapped files") >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > [...] >> +static int fill_files_note(struct memelfnote *note, struct coredump_params *cprm) >> { >> struct mm_struct *mm = current->mm; >> - struct vm_area_struct *vma; >> unsigned count, size, names_ofs, remaining, n; >> user_long_t *data; >> user_long_t *start_end_ofs; >> char *name_base, *name_curpos; >> + int i; >> >> /* *Estimated* file count and total data size needed */ >> count = mm->map_count; > > This function is still looking at mm->map_count in two spots, please > change those spots to also look at cprm->vma_count. In particular the > second one looks like it can cause memory corruption if the map_count > changed since we created the snapshot. Could catch I will fix that. Correcting it not to use mm->map_count looks like a fundamental part of the fix, and I missed it. Oops! > [...] >> +static void free_vma_snapshot(struct coredump_params *cprm) >> +{ >> + if (cprm->vma_meta) { >> + int i; >> + for (i = 0; i < cprm->vma_count; i++) { >> + struct file *file = cprm->vma_meta[i].file; >> + if (file) >> + fput(file); >> + } >> + kvfree(cprm->vma_meta); >> + cprm->vma_meta = NULL; > > (this NULL write is superfluous, but it also doesn't hurt) Agreed. It just makes the possible failure modes nicer. Eric
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 272032b1f9a2..5fcaa01d211e 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1619,14 +1619,14 @@ static void fill_siginfo_note(struct memelfnote *note, user_siginfo_t *csigdata, * long file_ofs * followed by COUNT filenames in ASCII: "FILE1" NUL "FILE2" NUL... */ -static int fill_files_note(struct memelfnote *note) +static int fill_files_note(struct memelfnote *note, struct coredump_params *cprm) { struct mm_struct *mm = current->mm; - struct vm_area_struct *vma; unsigned count, size, names_ofs, remaining, n; user_long_t *data; user_long_t *start_end_ofs; char *name_base, *name_curpos; + int i; /* *Estimated* file count and total data size needed */ count = mm->map_count; @@ -1651,11 +1651,12 @@ static int fill_files_note(struct memelfnote *note) name_base = name_curpos = ((char *)data) + names_ofs; remaining = size - names_ofs; count = 0; - for (vma = mm->mmap; vma != NULL; vma = vma->vm_next) { + for (i = 0; i < cprm->vma_count; i++) { + struct core_vma_metadata *m = &cprm->vma_meta[i]; struct file *file; const char *filename; - file = vma->vm_file; + file = m->file; if (!file) continue; filename = file_path(file, name_curpos, remaining); @@ -1675,9 +1676,9 @@ static int fill_files_note(struct memelfnote *note) memmove(name_curpos, filename, n); name_curpos += n; - *start_end_ofs++ = vma->vm_start; - *start_end_ofs++ = vma->vm_end; - *start_end_ofs++ = vma->vm_pgoff; + *start_end_ofs++ = m->start; + *start_end_ofs++ = m->end; + *start_end_ofs++ = m->pgoff; count++; } @@ -1887,7 +1888,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs, fill_auxv_note(&info->auxv, current->mm); info->size += notesize(&info->auxv); - if (fill_files_note(&info->files) == 0) + if (fill_files_note(&info->files, cprm) == 0) info->size += notesize(&info->files); return 1; @@ -2076,7 +2077,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs, fill_auxv_note(info->notes + 3, current->mm); info->numnote = 4; - if (fill_files_note(info->notes + info->numnote) == 0) { + if (fill_files_note(info->notes + info->numnote, cprm) == 0) { info->notes_files = info->notes + info->numnote; info->numnote++; } diff --git a/fs/coredump.c b/fs/coredump.c index c5e7d63525c6..6a97a8ea7295 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -54,6 +54,7 @@ #include <trace/events/sched.h> static bool dump_vma_snapshot(struct coredump_params *cprm); +static void free_vma_snapshot(struct coredump_params *cprm); static int core_uses_pid; static unsigned int core_pipe_limit; @@ -764,7 +765,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) dump_emit(&cprm, "", 1); } file_end_write(cprm.file); - kvfree(cprm.vma_meta); + free_vma_snapshot(&cprm); } if (ispipe && core_pipe_limit) wait_for_dump_helpers(cprm.file); @@ -1085,6 +1086,20 @@ static struct vm_area_struct *next_vma(struct vm_area_struct *this_vma, return gate_vma; } +static void free_vma_snapshot(struct coredump_params *cprm) +{ + if (cprm->vma_meta) { + int i; + for (i = 0; i < cprm->vma_count; i++) { + struct file *file = cprm->vma_meta[i].file; + if (file) + fput(file); + } + kvfree(cprm->vma_meta); + cprm->vma_meta = NULL; + } +} + /* * Under the mmap_lock, take a snapshot of relevant information about the task's * VMAs. @@ -1121,6 +1136,11 @@ static bool dump_vma_snapshot(struct coredump_params *cprm) m->end = vma->vm_end; m->flags = vma->vm_flags; m->dump_size = vma_dump_size(vma, cprm->mm_flags); + m->pgoff = vma->vm_pgoff; + + m->file = vma->vm_file; + if (m->file) + get_file(m->file); cprm->vma_data_size += m->dump_size; } diff --git a/include/linux/coredump.h b/include/linux/coredump.h index 7d05370e555e..08a1d3e7e46d 100644 --- a/include/linux/coredump.h +++ b/include/linux/coredump.h @@ -12,6 +12,8 @@ struct core_vma_metadata { unsigned long start, end; unsigned long flags; unsigned long dump_size; + unsigned long pgoff; + struct file *file; }; struct coredump_params {
Matthew Wilcox reported that there is a missing mmap_lock in file_files_note that could possibly lead to a user after free. Solve this by using the existing vma snapshot for consistency and to avoid the need to take the mmap_lock anywhere in the coredump code except for dump_vma_snapshot. Update the dump_vma_snapshot to capture vm_pgoff and vm_file that are neeeded by fill_files_note. Add free_vma_snapshot to free the captured values of vm_file. Reported-by: Matthew Wilcox <willy@infradead.org> Link: https://lkml.kernel.org/r/20220131153740.2396974-1-willy@infradead.org Cc: stable@vger.kernel.org Fixes: a07279c9a8cd ("binfmt_elf, binfmt_elf_fdpic: use a VMA list snapshot") Fixes: 2aa362c49c31 ("coredump: extend core dump note section to contain file names of mapped files") Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/binfmt_elf.c | 19 ++++++++++--------- fs/coredump.c | 22 +++++++++++++++++++++- include/linux/coredump.h | 2 ++ 3 files changed, 33 insertions(+), 10 deletions(-)