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 <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
Cc: [email protected]
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" <[email protected]>
---
fs/binfmt_elf.c | 19 ++++++++++---------
fs/coredump.c | 22 +++++++++++++++++++++-
include/linux/coredump.h | 2 ++
3 files changed, 33 insertions(+), 10 deletions(-)
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 {
--
2.29.2
Jann Horn <[email protected]> writes:
> On Mon, Jan 31, 2022 at 7:47 PM Eric W. Biederman <[email protected]> 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 <[email protected]>
>> Link: https://lkml.kernel.org/r/[email protected]
>> Cc: [email protected]
>> 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" <[email protected]>
> [...]
>> +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
On Mon, Jan 31, 2022 at 7:47 PM Eric W. Biederman <[email protected]> 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 <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
> Cc: [email protected]
> 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" <[email protected]>
[...]
> +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)
> + }
> +}