FAILED: patch "[PATCH] coredump: Use the vma snapshot in fill_files_note" failed to apply to 5.17-stable tree
The patch below does not apply to the 5.17-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 390031c942116d4733310f0684beb8db19885fe6 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" ebiederm@xmission.com Date: Tue, 8 Mar 2022 13:04:19 -0600 Subject: [PATCH] coredump: Use the vma snapshot in fill_files_note
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") Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: "Eric W. Biederman" ebiederm@xmission.com
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 7f0c391832cf..ca5296cae979 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1641,17 +1641,16 @@ 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; + count = cprm->vma_count; if (count > UINT_MAX / 64) return -EINVAL; size = count * 64; @@ -1673,11 +1672,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); @@ -1697,9 +1697,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++; }
@@ -1710,7 +1710,7 @@ static int fill_files_note(struct memelfnote *note) * Count usually is less than mm->map_count, * we need to move filenames down. */ - n = mm->map_count - count; + n = cprm->vma_count - count; if (n != 0) { unsigned shift_bytes = n * 3 * sizeof(data[0]); memmove(name_base - shift_bytes, name_base, @@ -1909,7 +1909,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; @@ -2098,7 +2098,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 7f100a637264..7ed7d601e5e0 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -55,6 +55,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; @@ -765,7 +766,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); @@ -1099,6 +1100,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. @@ -1135,6 +1150,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); }
mmap_write_unlock(mm); 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 {
gregkh@linuxfoundation.org writes:
The patch below does not apply to the 5.17-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
I believe it requires backporting these first.
commit 84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF libraries") commit 95c5436a4883 ("coredump: Snapshot the vmas in do_coredump") commit 49c1866348f3 ("coredump: Remove the WARN_ON in dump_vma_snapshot")
The first is a more interesting bug fix from Jann Horn. The other two are prerequisite cleanup-patches.
I will let other folks judge how concerned they are about missing locking that was detected by code review.
Eric
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 390031c942116d4733310f0684beb8db19885fe6 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" ebiederm@xmission.com Date: Tue, 8 Mar 2022 13:04:19 -0600 Subject: [PATCH] coredump: Use the vma snapshot in fill_files_note
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") Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: "Eric W. Biederman" ebiederm@xmission.com
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 7f0c391832cf..ca5296cae979 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1641,17 +1641,16 @@ 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;
- count = cprm->vma_count; if (count > UINT_MAX / 64) return -EINVAL; size = count * 64;
@@ -1673,11 +1672,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];
file = vma->vm_file;
file = m->file;@@ -1697,9 +1697,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;@@ -1710,7 +1710,7 @@ static int fill_files_note(struct memelfnote *note) * Count usually is less than mm->map_count, * we need to move filenames down. */
- n = mm->map_count - count;
- n = cprm->vma_count - count; if (n != 0) { unsigned shift_bytes = n * 3 * sizeof(data[0]); memmove(name_base - shift_bytes, name_base,
@@ -1909,7 +1909,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; @@ -2098,7 +2098,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 7f100a637264..7ed7d601e5e0 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -55,6 +55,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; @@ -765,7 +766,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);@@ -1099,6 +1100,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.
@@ -1135,6 +1150,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);mmap_write_unlock(mm); 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 {
On Mon, Apr 04, 2022 at 11:00:19AM -0500, Eric W. Biederman wrote:
gregkh@linuxfoundation.org writes:
The patch below does not apply to the 5.17-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
I believe it requires backporting these first.
commit 84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF libraries") commit 95c5436a4883 ("coredump: Snapshot the vmas in do_coredump") commit 49c1866348f3 ("coredump: Remove the WARN_ON in dump_vma_snapshot")
The first is a more interesting bug fix from Jann Horn. The other two are prerequisite cleanup-patches.
Thanks, that worked!
I will let other folks judge how concerned they are about missing locking that was detected by code review.
locking where? And if it's not resolved in Linus's tree yet, not much I can do.
Also, what about for kernels older than 5.10? Is this an issue there?
thanks,
greg k-h
On Tue, Apr 05, 2022 at 08:32:49AM +0200, Greg KH wrote:
On Mon, Apr 04, 2022 at 11:00:19AM -0500, Eric W. Biederman wrote:
gregkh@linuxfoundation.org writes:
The patch below does not apply to the 5.17-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
I believe it requires backporting these first.
commit 84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF libraries") commit 95c5436a4883 ("coredump: Snapshot the vmas in do_coredump") commit 49c1866348f3 ("coredump: Remove the WARN_ON in dump_vma_snapshot")
The first is a more interesting bug fix from Jann Horn. The other two are prerequisite cleanup-patches.
Thanks, that worked!
Nope, also had to add 9ec7d3230717 ("coredump/elf: Pass coredump_params into fill_note_info")
Greg KH gregkh@linuxfoundation.org writes:
On Mon, Apr 04, 2022 at 11:00:19AM -0500, Eric W. Biederman wrote:
gregkh@linuxfoundation.org writes:
The patch below does not apply to the 5.17-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
I believe it requires backporting these first.
commit 84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF libraries") commit 95c5436a4883 ("coredump: Snapshot the vmas in do_coredump") commit 49c1866348f3 ("coredump: Remove the WARN_ON in dump_vma_snapshot")
The first is a more interesting bug fix from Jann Horn. The other two are prerequisite cleanup-patches.
Thanks, that worked!
I will let other folks judge how concerned they are about missing locking that was detected by code review.
locking where? And if it's not resolved in Linus's tree yet, not much I can do.
Sorry for being unclear. This patch "coredump: Use the vma snapshot in fill_file_note" solves a problem of missing locking by refactoring code so the locking is unnecessary.
Also, what about for kernels older than 5.10? Is this an issue there?
The first fix for missing/problematic locking was added in commit a07279c9a8cd ("binfmt_elf, binfmt_elf_fdpic: use a VMA list snapshot"). Which is 5.10. I don't know if that fix has ever been backported.
The actual issue of problematic locking that this change was addressing looks like it dates back to commit 2aa362c49c31 ("coredump: extend core dump note section to contain file names of mapped files").
These are the kinds of bugs that creative people can use to get the kernel to do things it is not supposed to do. On an ordinary day no one trips over so they are not a problem. I am not good at assessing their impact so I just fix them and let other people figure out how much they want the fixes.
Eric
linux-stable-mirror@lists.linaro.org
-
Eric W. Biederman -
Greg KH -
gregkh＠linuxfoundation.org