FAILED: patch "[PATCH] coredump: Use the vma snapshot in fill_files_note" failed to apply to 5.17-stable tree

FAILED: patch "[PATCH] coredump: Use the vma snapshot in fill_files_note" failed to apply to 5.17-stable tree

[gregkh]

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 {

Re: FAILED: patch "[PATCH] coredump: Use the vma snapshot in fill_files_note" failed to apply to 5.17-stable tree

[Eric W. Biederman]

<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];
>  		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 {

Re: FAILED: patch "[PATCH] coredump: Use the vma snapshot in fill_files_note" failed to apply to 5.17-stable tree

[Greg KH]

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

Re: FAILED: patch "[PATCH] coredump: Use the vma snapshot in fill_files_note" failed to apply to 5.17-stable tree

[Greg KH]

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")

Re: FAILED: patch "[PATCH] coredump: Use the vma snapshot in fill_files_note" failed to apply to 5.17-stable tree

[Eric W. Biederman]

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