[PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

[PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

[Matthew Wilcox (Oracle)]

I'm not sure if the VMA list can change under us, but dump_vma_snapshot()
is very careful to take the mmap_lock in write mode.  We only need to
take it in read mode here as we do not care if the size of the stack
VMA changes underneath us.

If it can be changed underneath us, this is a potential use-after-free
for a multithreaded process which is dumping core.

Fixes: 2aa362c49c31 ("coredump: extend core dump note section to contain file names of mapped files")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
---
 fs/binfmt_elf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 605017eb9349..dc2318355762 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1651,6 +1651,7 @@ static int fill_files_note(struct memelfnote *note)
 	name_base = name_curpos = ((char *)data) + names_ofs;
 	remaining = size - names_ofs;
 	count = 0;
+	mmap_read_lock(mm);
 	for (vma = mm->mmap; vma != NULL; vma = vma->vm_next) {
 		struct file *file;
 		const char *filename;
@@ -1661,6 +1662,7 @@ static int fill_files_note(struct memelfnote *note)
 		filename = file_path(file, name_curpos, remaining);
 		if (IS_ERR(filename)) {
 			if (PTR_ERR(filename) == -ENAMETOOLONG) {
+				mmap_read_unlock(mm);
 				kvfree(data);
 				size = size * 5 / 4;
 				goto alloc;
@@ -1680,6 +1682,7 @@ static int fill_files_note(struct memelfnote *note)
 		*start_end_ofs++ = vma->vm_pgoff;
 		count++;
 	}
+	mmap_read_unlock(mm);
 
 	/* Now we know exact count of files, can store it */
 	data[0] = count;
-- 
2.34.1

Re: [PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

[Eric W. Biederman]

"Matthew Wilcox (Oracle)" <willy@infradead.org> writes:

> I'm not sure if the VMA list can change under us, but dump_vma_snapshot()
> is very careful to take the mmap_lock in write mode.  We only need to
> take it in read mode here as we do not care if the size of the stack
> VMA changes underneath us.
>
> If it can be changed underneath us, this is a potential use-after-free
> for a multithreaded process which is dumping core.

The problem is not multi-threaded process so much as processes that
share their mm.

I think rather than take a lock we should be using the snapshot captured
with dump_vma_snapshot.  Otherwise we have the very real chance that the
two get out of sync.  Which would result in a non-sense core file.

Probably that means that dump_vma_snapshot needs to call get_file on
vma->vm_file store it in core_vma_metadata.

Do you think you can fix it something like that?

Eric


> Fixes: 2aa362c49c31 ("coredump: extend core dump note section to contain file names of mapped files")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> ---
>  fs/binfmt_elf.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 605017eb9349..dc2318355762 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1651,6 +1651,7 @@ static int fill_files_note(struct memelfnote *note)
>  	name_base = name_curpos = ((char *)data) + names_ofs;
>  	remaining = size - names_ofs;
>  	count = 0;
> +	mmap_read_lock(mm);
>  	for (vma = mm->mmap; vma != NULL; vma = vma->vm_next) {
>  		struct file *file;
>  		const char *filename;
> @@ -1661,6 +1662,7 @@ static int fill_files_note(struct memelfnote *note)
>  		filename = file_path(file, name_curpos, remaining);
>  		if (IS_ERR(filename)) {
>  			if (PTR_ERR(filename) == -ENAMETOOLONG) {
> +				mmap_read_unlock(mm);
>  				kvfree(data);
>  				size = size * 5 / 4;
>  				goto alloc;
> @@ -1680,6 +1682,7 @@ static int fill_files_note(struct memelfnote *note)
>  		*start_end_ofs++ = vma->vm_pgoff;
>  		count++;
>  	}
> +	mmap_read_unlock(mm);
>  
>  	/* Now we know exact count of files, can store it */
>  	data[0] = count;

Re: [PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

[Matthew Wilcox]

On Mon, Jan 31, 2022 at 10:03:31AM -0600, Eric W. Biederman wrote:
> "Matthew Wilcox (Oracle)" <willy@infradead.org> writes:
> 
> > I'm not sure if the VMA list can change under us, but dump_vma_snapshot()
> > is very careful to take the mmap_lock in write mode.  We only need to
> > take it in read mode here as we do not care if the size of the stack
> > VMA changes underneath us.
> >
> > If it can be changed underneath us, this is a potential use-after-free
> > for a multithreaded process which is dumping core.
> 
> The problem is not multi-threaded process so much as processes that
> share their mm.

I don't understand the difference.  I appreciate that another process can
get read access to an mm through, eg, /proc, but how can another process
(that isn't a thread of this process) modify the VMAs?

> I think rather than take a lock we should be using the snapshot captured
> with dump_vma_snapshot.  Otherwise we have the very real chance that the
> two get out of sync.  Which would result in a non-sense core file.
> 
> Probably that means that dump_vma_snapshot needs to call get_file on
> vma->vm_file store it in core_vma_metadata.
> 
> Do you think you can fix it something like that?

Uhh .. that seems like it needs a lot more understanding of binfmt_elf
than I currently possess.  I'd rather spend my time working on folios
than learning much more about binfmt_elf.  I was just trying to fix an
assertion failure with the maple tree patches (we now assert that you're
holding a lock when walking the list of VMAs).

Re: [PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

[Eric W. Biederman]

Matthew Wilcox <willy@infradead.org> writes:

> On Mon, Jan 31, 2022 at 10:03:31AM -0600, Eric W. Biederman wrote:
>> "Matthew Wilcox (Oracle)" <willy@infradead.org> writes:
>> 
>> > I'm not sure if the VMA list can change under us, but dump_vma_snapshot()
>> > is very careful to take the mmap_lock in write mode.  We only need to
>> > take it in read mode here as we do not care if the size of the stack
>> > VMA changes underneath us.
>> >
>> > If it can be changed underneath us, this is a potential use-after-free
>> > for a multithreaded process which is dumping core.
>> 
>> The problem is not multi-threaded process so much as processes that
>> share their mm.
>
> I don't understand the difference.  I appreciate that another process can
> get read access to an mm through, eg, /proc, but how can another process
> (that isn't a thread of this process) modify the VMAs?

There are a couple of ways.

A classic way is a multi-threads process can call vfork, and the
mm_struct is shared with the child until exec is called.

A process can do this more deliberately by forking a child using
clone(CLONE_VM) and not including CLONE_THREAD.   Supporting this case
is a hold over from before CLONE_THREAD was supported in the kernel and
such processes were used to simulate threads.

The practical difference between a CLONE_THREAD thread and a
non-CLONE_THREAD process is that the signal handling is not shared.
Without sharing the signal handlers it does not make sense for a fatal
signal to kill the other process.

From the perspective of coredump generation it stops the execution of
all CLONE_THREAD threads that are going to be part of the coredump
and allows anyone else who shared the mm_struct to keep running.


It also happens that there are subsystems in the kernel that do things
like kthread_use_mm that can also be modifying the mm during a coredump.

Which is why we have dump_vma_snapshot.  Preventing the mm_struct and
the vmas from being modified during a coredump is not really practical.


>> I think rather than take a lock we should be using the snapshot captured
>> with dump_vma_snapshot.  Otherwise we have the very real chance that the
>> two get out of sync.  Which would result in a non-sense core file.
>> 
>> Probably that means that dump_vma_snapshot needs to call get_file on
>> vma->vm_file store it in core_vma_metadata.
>> 
>> Do you think you can fix it something like that?
>
> Uhh .. that seems like it needs a lot more understanding of binfmt_elf
> than I currently possess.  I'd rather spend my time working on folios
> than learning much more about binfmt_elf.  I was just trying to fix an
> assertion failure with the maple tree patches (we now assert that you're
> holding a lock when walking the list of VMAs).

Fair enough.  I will put it on my list of things to address.

Eric

Re: [PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

[Matthew Wilcox]

On Mon, Jan 31, 2022 at 10:26:11AM -0600, Eric W. Biederman wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
> > On Mon, Jan 31, 2022 at 10:03:31AM -0600, Eric W. Biederman wrote:
> >> "Matthew Wilcox (Oracle)" <willy@infradead.org> writes:
> >> 
> >> > I'm not sure if the VMA list can change under us, but dump_vma_snapshot()
> >> > is very careful to take the mmap_lock in write mode.  We only need to
> >> > take it in read mode here as we do not care if the size of the stack
> >> > VMA changes underneath us.
> >> >
> >> > If it can be changed underneath us, this is a potential use-after-free
> >> > for a multithreaded process which is dumping core.
> >> 
> >> The problem is not multi-threaded process so much as processes that
> >> share their mm.
> >
> > I don't understand the difference.  I appreciate that another process can
> > get read access to an mm through, eg, /proc, but how can another process
> > (that isn't a thread of this process) modify the VMAs?
> 
> There are a couple of ways.
> 
> A classic way is a multi-threads process can call vfork, and the
> mm_struct is shared with the child until exec is called.

While true, I thought the semantics of vfork() were that the parent
was suspended.  Given that, it can't core dump until the child execs
... right?

> A process can do this more deliberately by forking a child using
> clone(CLONE_VM) and not including CLONE_THREAD.   Supporting this case
> is a hold over from before CLONE_THREAD was supported in the kernel and
> such processes were used to simulate threads.

That is a multithreaded process then!  Maybe not in the strict POSIX
compliance sense, but the intent is to be a multithreaded process.
ie multiple threads of execution, sharing an address space.

> It also happens that there are subsystems in the kernel that do things
> like kthread_use_mm that can also be modifying the mm during a coredump.

Yikes.  That's terrifying.  It's really legitimate for a kthread to
attach to a process and start tearing down VMAs?

> > Uhh .. that seems like it needs a lot more understanding of binfmt_elf
> > than I currently possess.  I'd rather spend my time working on folios
> > than learning much more about binfmt_elf.  I was just trying to fix an
> > assertion failure with the maple tree patches (we now assert that you're
> > holding a lock when walking the list of VMAs).
> 
> Fair enough.  I will put it on my list of things to address.

Thanks.  Now that I've disclosed it's a UAF, I hope you're able to
get to it soon.  Otherwise we should put this band-aid in for now
and you can address it properly in the fullness of time.

Re: [PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

[Jann Horn]

On Mon, Jan 31, 2022 at 5:35 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Jan 31, 2022 at 10:26:11AM -0600, Eric W. Biederman wrote:
> > Matthew Wilcox <willy@infradead.org> writes:
> >
> > > On Mon, Jan 31, 2022 at 10:03:31AM -0600, Eric W. Biederman wrote:
> > >> "Matthew Wilcox (Oracle)" <willy@infradead.org> writes:
> > >>
> > >> > I'm not sure if the VMA list can change under us, but dump_vma_snapshot()
> > >> > is very careful to take the mmap_lock in write mode.  We only need to
> > >> > take it in read mode here as we do not care if the size of the stack
> > >> > VMA changes underneath us.
> > >> >
> > >> > If it can be changed underneath us, this is a potential use-after-free
> > >> > for a multithreaded process which is dumping core.
> > >>
> > >> The problem is not multi-threaded process so much as processes that
> > >> share their mm.
> > >
> > > I don't understand the difference.  I appreciate that another process can
> > > get read access to an mm through, eg, /proc, but how can another process
> > > (that isn't a thread of this process) modify the VMAs?
> >
> > There are a couple of ways.
> >
> > A classic way is a multi-threads process can call vfork, and the
> > mm_struct is shared with the child until exec is called.
>
> While true, I thought the semantics of vfork() were that the parent
> was suspended.  Given that, it can't core dump until the child execs
> ... right?
>
> > A process can do this more deliberately by forking a child using
> > clone(CLONE_VM) and not including CLONE_THREAD.   Supporting this case
> > is a hold over from before CLONE_THREAD was supported in the kernel and
> > such processes were used to simulate threads.

The syscall clone() is kind of the generalized version of fork() and
vfork(), and it lets you create fun combinations of these things (some
of which might be useful, others which make little sense), and e.g.
vfork() is basically just clone() with CLONE_VM (for sharing address
space) plus CLONE_VFORK (block until child exits or execs) plus
SIGCHLD (child should send SIGCHLD to parent when it terminates).

(Some combinations are disallowed, but you can IIRC make things like
threads with separate FD tables, or processes that share virtual
memory and signal handler tables but aren't actual threads.)


Note that until commit 0258b5fd7c71 ("coredump: Limit coredumps to a
single thread group", first in 5.16), coredumps would always tear down
every process that shares the MM before dumping, and the coredumping
code was trying to rely on that to protect against concurrency. (Which
at some point didn't actually work anymore, see below...)

> That is a multithreaded process then!  Maybe not in the strict POSIX
> compliance sense, but the intent is to be a multithreaded process.
> ie multiple threads of execution, sharing an address space.

current_is_single_threaded() agrees with you. :P

> > It also happens that there are subsystems in the kernel that do things
> > like kthread_use_mm that can also be modifying the mm during a coredump.
>
> Yikes.  That's terrifying.  It's really legitimate for a kthread to
> attach to a process and start tearing down VMAs?

I don't know anything about kthreads doing this, but a fun way to
remotely mess with VMAs is userfaultfd. See
https://bugs.chromium.org/p/project-zero/issues/detail?id=1790 ("Issue
1790: Linux: missing locking between ELF coredump code and userfaultfd
VMA modification") for the long version - but the gist is that
userfaultfd can set/clear flags on virtual address ranges (implemented
as flags on VMAs), and that may involve splitting VMAs (when the
boundaries of the specified range don't correspond to existing VMA
boundaries) or merging VMAs (when the flags of adjacent VMAs become
equal). And userfaultfd can by design be used remotely on another
process (so long as that process first created the userfaultfd and
then handed it over).

That's why I added that locking in the coredumping code.

> > > Uhh .. that seems like it needs a lot more understanding of binfmt_elf
> > > than I currently possess.  I'd rather spend my time working on folios
> > > than learning much more about binfmt_elf.  I was just trying to fix an
> > > assertion failure with the maple tree patches (we now assert that you're
> > > holding a lock when walking the list of VMAs).
> >
> > Fair enough.  I will put it on my list of things to address.
>
> Thanks.  Now that I've disclosed it's a UAF, I hope you're able to
> get to it soon.  Otherwise we should put this band-aid in for now
> and you can address it properly in the fullness of time.

Re: [PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

[Eric W. Biederman]

Matthew Wilcox <willy@infradead.org> writes:

> On Mon, Jan 31, 2022 at 10:26:11AM -0600, Eric W. Biederman wrote:
>> Matthew Wilcox <willy@infradead.org> writes:
>> 
>> > On Mon, Jan 31, 2022 at 10:03:31AM -0600, Eric W. Biederman wrote:
>> >> "Matthew Wilcox (Oracle)" <willy@infradead.org> writes:
>> >> 
>> >> > I'm not sure if the VMA list can change under us, but dump_vma_snapshot()
>> >> > is very careful to take the mmap_lock in write mode.  We only need to
>> >> > take it in read mode here as we do not care if the size of the stack
>> >> > VMA changes underneath us.
>> >> >
>> >> > If it can be changed underneath us, this is a potential use-after-free
>> >> > for a multithreaded process which is dumping core.
>> >> 
>> >> The problem is not multi-threaded process so much as processes that
>> >> share their mm.
>> >
>> > I don't understand the difference.  I appreciate that another process can
>> > get read access to an mm through, eg, /proc, but how can another process
>> > (that isn't a thread of this process) modify the VMAs?
>> 
>> There are a couple of ways.
>> 
>> A classic way is a multi-threads process can call vfork, and the
>> mm_struct is shared with the child until exec is called.
>
> While true, I thought the semantics of vfork() were that the parent
> was suspended.  Given that, it can't core dump until the child execs
> ... right?

The thread that called vfork is suspended.  The other threads can
continue to execute.

>> A process can do this more deliberately by forking a child using
>> clone(CLONE_VM) and not including CLONE_THREAD.   Supporting this case
>> is a hold over from before CLONE_THREAD was supported in the kernel and
>> such processes were used to simulate threads.
>
> That is a multithreaded process then!  Maybe not in the strict POSIX
> compliance sense, but the intent is to be a multithreaded process.
> ie multiple threads of execution, sharing an address space.

Sometimes.  From a coredump perspective it is just another process
that happens to share the mm.  Like the vfork process.

For a while the coredump code was trying to kill and possibly dump all
of these ``threads'' that shared a vm.  The practical problem was that
a failing exec after vfork could trigger a coredump that would kill
it's parent process.

So when I look at these from a coredump or signal perspective I just
treat them as weird processes that happen to share an mm_struct.

>> It also happens that there are subsystems in the kernel that do things
>> like kthread_use_mm that can also be modifying the mm during a coredump.
>
> Yikes.  That's terrifying.  It's really legitimate for a kthread to
> attach to a process and start tearing down VMAs?

I don't know how much VMA manipulation makes sense but it is legitimate
to attach to an mm and do those things as Jann pointed out.

> Thanks.  Now that I've disclosed it's a UAF, I hope you're able to
> get to it soon.  Otherwise we should put this band-aid in for now
> and you can address it properly in the fullness of time.

Working on it now.

Eric

[PATCH 0/5] Fix fill_files_note

[Eric W. Biederman]

Matthew Wilcox has reported that a missing mmap_lock in file_files_note,
which could cause trouble.

Refactor the code and clean it up so that the vma snapshot makes
it to fill_files_note, and then use the vma snapshot in fill_files_note.

Folks please review this as this looks correct to me but I haven't done
anything beyond compile testing this yet.

Eric W. Biederman (5):
      coredump: Move definition of struct coredump_params into coredump.h
      coredump: Snapshot the vmas in do_coredump
      coredump: Remove the WARN_ON in dump_vma_snapshot
      coredump/elf: Pass coredump_params into fill_note_info
      coredump: Use the vma snapshot in fill_files_note

 fs/binfmt_elf.c          | 61 ++++++++++++++++++++++--------------------------
 fs/binfmt_elf_fdpic.c    | 18 +++++---------
 fs/coredump.c            | 55 +++++++++++++++++++++++++++++--------------
 include/linux/binfmts.h  | 13 +----------
 include/linux/coredump.h | 20 ++++++++++++----
 5 files changed, 88 insertions(+), 79 deletions(-)


Eric

[PATCH 1/5] coredump: Move definition of struct coredump_params into coredump.h

[Eric W. Biederman]

Move the definition of struct coredump_params into coredump.h where
it belongs.

Remove the slightly errorneous comment explaining why struct
coredump_params was declared in binfmts.h.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/linux/binfmts.h  | 13 +------------
 include/linux/coredump.h | 12 +++++++++++-
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 049cf9421d83..05a91f5499ba 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -8,6 +8,7 @@
 #include <uapi/linux/binfmts.h>
 
 struct filename;
+struct coredump_params;
 
 #define CORENAME_MAX_SIZE 128
 
@@ -77,18 +78,6 @@ struct linux_binprm {
 #define BINPRM_FLAGS_PRESERVE_ARGV0_BIT 3
 #define BINPRM_FLAGS_PRESERVE_ARGV0 (1 << BINPRM_FLAGS_PRESERVE_ARGV0_BIT)
 
-/* Function parameter for binfmt->coredump */
-struct coredump_params {
-	const kernel_siginfo_t *siginfo;
-	struct pt_regs *regs;
-	struct file *file;
-	unsigned long limit;
-	unsigned long mm_flags;
-	loff_t written;
-	loff_t pos;
-	loff_t to_skip;
-};
-
 /*
  * This structure defines the functions that are used to load the binary formats that
  * linux accepts.
diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index 248a68c668b4..2ee1460a1d66 100644
--- a/include/linux/coredump.h
+++ b/include/linux/coredump.h
@@ -14,11 +14,21 @@ struct core_vma_metadata {
 	unsigned long dump_size;
 };
 
+struct coredump_params {
+	const kernel_siginfo_t *siginfo;
+	struct pt_regs *regs;
+	struct file *file;
+	unsigned long limit;
+	unsigned long mm_flags;
+	loff_t written;
+	loff_t pos;
+	loff_t to_skip;
+};
+
 /*
  * These are the only things you should do on a core-file: use only these
  * functions to write out all the necessary info.
  */
-struct coredump_params;
 extern void dump_skip_to(struct coredump_params *cprm, unsigned long to);
 extern void dump_skip(struct coredump_params *cprm, size_t nr);
 extern int dump_emit(struct coredump_params *cprm, const void *addr, int nr);
-- 
2.29.2

[PATCH 2/5] coredump: Snapshot the vmas in do_coredump

[Eric W. Biederman]

Move the call of dump_vma_snapshot and kvfree(vma_meta) out of the
individual coredump routines into do_coredump itself.  This makes
the code less error prone and easier to maintain.

Make the vma snapshot available to the coredump routines
in struct coredump_params.  This makes it easier to
change and update what is captures in the vma snapshot
and will be needed for fixing fill_file_notes.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/binfmt_elf.c          | 20 +++++++-------------
 fs/binfmt_elf_fdpic.c    | 18 ++++++------------
 fs/coredump.c            | 36 ++++++++++++++++++++----------------
 include/linux/coredump.h |  6 +++---
 4 files changed, 36 insertions(+), 44 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 605017eb9349..4822b04154e1 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -2169,8 +2169,7 @@ static void fill_extnum_info(struct elfhdr *elf, struct elf_shdr *shdr4extnum,
 static int elf_core_dump(struct coredump_params *cprm)
 {
 	int has_dumped = 0;
-	int vma_count, segs, i;
-	size_t vma_data_size;
+	int segs, i;
 	struct elfhdr elf;
 	loff_t offset = 0, dataoff;
 	struct elf_note_info info = { };
@@ -2178,16 +2177,12 @@ static int elf_core_dump(struct coredump_params *cprm)
 	struct elf_shdr *shdr4extnum = NULL;
 	Elf_Half e_phnum;
 	elf_addr_t e_shoff;
-	struct core_vma_metadata *vma_meta;
-
-	if (dump_vma_snapshot(cprm, &vma_count, &vma_meta, &vma_data_size))
-		return 0;
 
 	/*
 	 * The number of segs are recored into ELF header as 16bit value.
 	 * Please check DEFAULT_MAX_MAP_COUNT definition when you modify here.
 	 */
-	segs = vma_count + elf_core_extra_phdrs();
+	segs = cprm->vma_count + elf_core_extra_phdrs();
 
 	/* for notes section */
 	segs++;
@@ -2226,7 +2221,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	offset += vma_data_size;
+	offset += cprm->vma_data_size;
 	offset += elf_core_extra_data_size();
 	e_shoff = offset;
 
@@ -2246,8 +2241,8 @@ static int elf_core_dump(struct coredump_params *cprm)
 		goto end_coredump;
 
 	/* Write program headers for segments dump */
-	for (i = 0; i < vma_count; i++) {
-		struct core_vma_metadata *meta = vma_meta + i;
+	for (i = 0; i < cprm->vma_count; i++) {
+		struct core_vma_metadata *meta = cprm->vma_meta + i;
 		struct elf_phdr phdr;
 
 		phdr.p_type = PT_LOAD;
@@ -2284,8 +2279,8 @@ static int elf_core_dump(struct coredump_params *cprm)
 	/* Align to page */
 	dump_skip_to(cprm, dataoff);
 
-	for (i = 0; i < vma_count; i++) {
-		struct core_vma_metadata *meta = vma_meta + i;
+	for (i = 0; i < cprm->vma_count; i++) {
+		struct core_vma_metadata *meta = cprm->vma_meta + i;
 
 		if (!dump_user_range(cprm, meta->start, meta->dump_size))
 			goto end_coredump;
@@ -2302,7 +2297,6 @@ static int elf_core_dump(struct coredump_params *cprm)
 end_coredump:
 	free_note_info(&info);
 	kfree(shdr4extnum);
-	kvfree(vma_meta);
 	kfree(phdr4note);
 	return has_dumped;
 }
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index c6f588dc4a9d..1a25536b0120 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1465,7 +1465,7 @@ static bool elf_fdpic_dump_segments(struct coredump_params *cprm,
 static int elf_fdpic_core_dump(struct coredump_params *cprm)
 {
 	int has_dumped = 0;
-	int vma_count, segs;
+	int segs;
 	int i;
 	struct elfhdr *elf = NULL;
 	loff_t offset = 0, dataoff;
@@ -1480,8 +1480,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 	elf_addr_t e_shoff;
 	struct core_thread *ct;
 	struct elf_thread_status *tmp;
-	struct core_vma_metadata *vma_meta = NULL;
-	size_t vma_data_size;
 
 	/* alloc memory for large data structures: too large to be on stack */
 	elf = kmalloc(sizeof(*elf), GFP_KERNEL);
@@ -1491,9 +1489,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 	if (!psinfo)
 		goto end_coredump;
 
-	if (dump_vma_snapshot(cprm, &vma_count, &vma_meta, &vma_data_size))
-		goto end_coredump;
-
 	for (ct = current->signal->core_state->dumper.next;
 					ct; ct = ct->next) {
 		tmp = elf_dump_thread_status(cprm->siginfo->si_signo,
@@ -1513,7 +1508,7 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 	tmp->next = thread_list;
 	thread_list = tmp;
 
-	segs = vma_count + elf_core_extra_phdrs();
+	segs = cprm->vma_count + elf_core_extra_phdrs();
 
 	/* for notes section */
 	segs++;
@@ -1558,7 +1553,7 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 	/* Page-align dumped data */
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	offset += vma_data_size;
+	offset += cprm->vma_data_size;
 	offset += elf_core_extra_data_size();
 	e_shoff = offset;
 
@@ -1578,8 +1573,8 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 		goto end_coredump;
 
 	/* write program headers for segments dump */
-	for (i = 0; i < vma_count; i++) {
-		struct core_vma_metadata *meta = vma_meta + i;
+	for (i = 0; i < cprm->vma_count; i++) {
+		struct core_vma_metadata *meta = cprm->vma_meta + i;
 		struct elf_phdr phdr;
 		size_t sz;
 
@@ -1628,7 +1623,7 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 
 	dump_skip_to(cprm, dataoff);
 
-	if (!elf_fdpic_dump_segments(cprm, vma_meta, vma_count))
+	if (!elf_fdpic_dump_segments(cprm, cprm->vma_meta, cprm->vma_count))
 		goto end_coredump;
 
 	if (!elf_core_write_extra_data(cprm))
@@ -1652,7 +1647,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 		thread_list = thread_list->next;
 		kfree(tmp);
 	}
-	kvfree(vma_meta);
 	kfree(phdr4note);
 	kfree(elf);
 	kfree(psinfo);
diff --git a/fs/coredump.c b/fs/coredump.c
index 1c060c0a2d72..def2a0c9cb14 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -53,6 +53,8 @@
 
 #include <trace/events/sched.h>
 
+static bool dump_vma_snapshot(struct coredump_params *cprm);
+
 static int core_uses_pid;
 static unsigned int core_pipe_limit;
 static char core_pattern[CORENAME_MAX_SIZE] = "core";
@@ -531,6 +533,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		 * by any locks.
 		 */
 		.mm_flags = mm->flags,
+		.vma_meta = NULL,
 	};
 
 	audit_core_dumps(siginfo->si_signo);
@@ -745,6 +748,9 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			pr_info("Core dump to |%s disabled\n", cn.corename);
 			goto close_fail;
 		}
+		if (!dump_vma_snapshot(&cprm))
+			goto close_fail;
+
 		file_start_write(cprm.file);
 		core_dumped = binfmt->core_dump(&cprm);
 		/*
@@ -758,6 +764,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			dump_emit(&cprm, "", 1);
 		}
 		file_end_write(cprm.file);
+		kvfree(cprm.vma_meta);
 	}
 	if (ispipe && core_pipe_limit)
 		wait_for_dump_helpers(cprm.file);
@@ -1082,14 +1089,11 @@ static struct vm_area_struct *next_vma(struct vm_area_struct *this_vma,
  * Under the mmap_lock, take a snapshot of relevant information about the task's
  * VMAs.
  */
-int dump_vma_snapshot(struct coredump_params *cprm, int *vma_count,
-		      struct core_vma_metadata **vma_meta,
-		      size_t *vma_data_size_ptr)
+static bool dump_vma_snapshot(struct coredump_params *cprm)
 {
 	struct vm_area_struct *vma, *gate_vma;
 	struct mm_struct *mm = current->mm;
 	int i;
-	size_t vma_data_size = 0;
 
 	/*
 	 * Once the stack expansion code is fixed to not change VMA bounds
@@ -1097,36 +1101,36 @@ int dump_vma_snapshot(struct coredump_params *cprm, int *vma_count,
 	 * mmap_lock in read mode.
 	 */
 	if (mmap_write_lock_killable(mm))
-		return -EINTR;
+		return false;
 
+	cprm->vma_data_size = 0;
 	gate_vma = get_gate_vma(mm);
-	*vma_count = mm->map_count + (gate_vma ? 1 : 0);
+	cprm->vma_count = mm->map_count + (gate_vma ? 1 : 0);
 
-	*vma_meta = kvmalloc_array(*vma_count, sizeof(**vma_meta), GFP_KERNEL);
-	if (!*vma_meta) {
+	cprm->vma_meta = kvmalloc_array(cprm->vma_count, sizeof(*cprm->vma_meta), GFP_KERNEL);
+	if (!cprm->vma_meta) {
 		mmap_write_unlock(mm);
-		return -ENOMEM;
+		return false;
 	}
 
 	for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
 			vma = next_vma(vma, gate_vma), i++) {
-		struct core_vma_metadata *m = (*vma_meta) + i;
+		struct core_vma_metadata *m = cprm->vma_meta + i;
 
 		m->start = vma->vm_start;
 		m->end = vma->vm_end;
 		m->flags = vma->vm_flags;
 		m->dump_size = vma_dump_size(vma, cprm->mm_flags);
 
-		vma_data_size += m->dump_size;
+		cprm->vma_data_size += m->dump_size;
 	}
 
 	mmap_write_unlock(mm);
 
-	if (WARN_ON(i != *vma_count)) {
-		kvfree(*vma_meta);
-		return -EFAULT;
+	if (WARN_ON(i != cprm->vma_count)) {
+		kvfree(cprm->vma_meta);
+		return false;
 	}
 
-	*vma_data_size_ptr = vma_data_size;
-	return 0;
+	return true;
 }
diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index 2ee1460a1d66..7d05370e555e 100644
--- a/include/linux/coredump.h
+++ b/include/linux/coredump.h
@@ -23,6 +23,9 @@ struct coredump_params {
 	loff_t written;
 	loff_t pos;
 	loff_t to_skip;
+	int vma_count;
+	size_t vma_data_size;
+	struct core_vma_metadata *vma_meta;
 };
 
 /*
@@ -35,9 +38,6 @@ extern int dump_emit(struct coredump_params *cprm, const void *addr, int nr);
 extern int dump_align(struct coredump_params *cprm, int align);
 int dump_user_range(struct coredump_params *cprm, unsigned long start,
 		    unsigned long len);
-int dump_vma_snapshot(struct coredump_params *cprm, int *vma_count,
-		      struct core_vma_metadata **vma_meta,
-		      size_t *vma_data_size_ptr);
 extern void do_coredump(const kernel_siginfo_t *siginfo);
 #else
 static inline void do_coredump(const kernel_siginfo_t *siginfo) {}
-- 
2.29.2

[PATCH 3/5] coredump: Remove the WARN_ON in dump_vma_snapshot

[Eric W. Biederman]

The condition is impossible and to the best of my knowledge has never
triggered.

We are in deep trouble if that conditions happens and we walk past
the end of our allocated array.

So delete the WARN_ON and the code that makes it look like the kernel
can handle the case of walking past the end of it's vma_meta array.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/coredump.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index def2a0c9cb14..c5e7d63525c6 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -1127,10 +1127,5 @@ static bool dump_vma_snapshot(struct coredump_params *cprm)
 
 	mmap_write_unlock(mm);
 
-	if (WARN_ON(i != cprm->vma_count)) {
-		kvfree(cprm->vma_meta);
-		return false;
-	}
-
 	return true;
 }
-- 
2.29.2

[PATCH 4/5] coredump/elf: Pass coredump_params into fill_note_info

[Eric W. Biederman]

Instead of individually passing cprm->siginfo and cprm->regs
into fill_note_info pass all of struct coredump_params.

This is preparation to allow fill_files_note to use the existing
vma snapshot.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/binfmt_elf.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 4822b04154e1..272032b1f9a2 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1800,7 +1800,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 
 static int fill_note_info(struct elfhdr *elf, int phdrs,
 			  struct elf_note_info *info,
-			  const kernel_siginfo_t *siginfo, struct pt_regs *regs)
+			  struct coredump_params *cprm)
 {
 	struct task_struct *dump_task = current;
 	const struct user_regset_view *view = task_user_regset_view(dump_task);
@@ -1872,7 +1872,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	 * Now fill in each thread's information.
 	 */
 	for (t = info->thread; t != NULL; t = t->next)
-		if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size))
+		if (!fill_thread_core_info(t, view, cprm->siginfo->si_signo, &info->size))
 			return 0;
 
 	/*
@@ -1881,7 +1881,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	fill_psinfo(psinfo, dump_task->group_leader, dump_task->mm);
 	info->size += notesize(&info->psinfo);
 
-	fill_siginfo_note(&info->signote, &info->csigdata, siginfo);
+	fill_siginfo_note(&info->signote, &info->csigdata, cprm->siginfo);
 	info->size += notesize(&info->signote);
 
 	fill_auxv_note(&info->auxv, current->mm);
@@ -2029,7 +2029,7 @@ static int elf_note_info_init(struct elf_note_info *info)
 
 static int fill_note_info(struct elfhdr *elf, int phdrs,
 			  struct elf_note_info *info,
-			  const kernel_siginfo_t *siginfo, struct pt_regs *regs)
+			  struct coredump_params *cprm)
 {
 	struct core_thread *ct;
 	struct elf_thread_status *ets;
@@ -2050,13 +2050,13 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	list_for_each_entry(ets, &info->thread_list, list) {
 		int sz;
 
-		sz = elf_dump_thread_status(siginfo->si_signo, ets);
+		sz = elf_dump_thread_status(cprm->siginfo->si_signo, ets);
 		info->thread_status_size += sz;
 	}
 	/* now collect the dump for the current */
 	memset(info->prstatus, 0, sizeof(*info->prstatus));
-	fill_prstatus(&info->prstatus->common, current, siginfo->si_signo);
-	elf_core_copy_regs(&info->prstatus->pr_reg, regs);
+	fill_prstatus(&info->prstatus->common, current, cprm->siginfo->si_signo);
+	elf_core_copy_regs(&info->prstatus->pr_reg, cprm->regs);
 
 	/* Set up header */
 	fill_elf_header(elf, phdrs, ELF_ARCH, ELF_CORE_EFLAGS);
@@ -2072,7 +2072,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	fill_note(info->notes + 1, "CORE", NT_PRPSINFO,
 		  sizeof(*info->psinfo), info->psinfo);
 
-	fill_siginfo_note(info->notes + 2, &info->csigdata, siginfo);
+	fill_siginfo_note(info->notes + 2, &info->csigdata, cprm->siginfo);
 	fill_auxv_note(info->notes + 3, current->mm);
 	info->numnote = 4;
 
@@ -2082,8 +2082,8 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	}
 
 	/* Try to dump the FPU. */
-	info->prstatus->pr_fpvalid = elf_core_copy_task_fpregs(current, regs,
-							       info->fpu);
+	info->prstatus->pr_fpvalid =
+		elf_core_copy_task_fpregs(current, cprm->regs, info->fpu);
 	if (info->prstatus->pr_fpvalid)
 		fill_note(info->notes + info->numnote++,
 			  "CORE", NT_PRFPREG, sizeof(*info->fpu), info->fpu);
@@ -2196,7 +2196,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 	 * Collect all the non-memory information about the process for the
 	 * notes.  This also sets up the file header.
 	 */
-	if (!fill_note_info(&elf, e_phnum, &info, cprm->siginfo, cprm->regs))
+	if (!fill_note_info(&elf, e_phnum, &info, cprm))
 		goto end_coredump;
 
 	has_dumped = 1;
-- 
2.29.2

[PATCH 5/5] coredump: Use the vma snapshot in fill_files_note

[Eric W. Biederman]

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

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

Re: [PATCH 0/5] Fix fill_files_note

[Kees Cook]

On Mon, Jan 31, 2022 at 12:44:53PM -0600, Eric W. Biederman wrote:
> 
> Matthew Wilcox has reported that a missing mmap_lock in file_files_note,
> which could cause trouble.
> 
> Refactor the code and clean it up so that the vma snapshot makes
> it to fill_files_note, and then use the vma snapshot in fill_files_note.
> 
> Folks please review this as this looks correct to me but I haven't done
> anything beyond compile testing this yet.
> 
> Eric W. Biederman (5):
>       coredump: Move definition of struct coredump_params into coredump.h
>       coredump: Snapshot the vmas in do_coredump
>       coredump: Remove the WARN_ON in dump_vma_snapshot
>       coredump/elf: Pass coredump_params into fill_note_info
>       coredump: Use the vma snapshot in fill_files_note
> 
>  fs/binfmt_elf.c          | 61 ++++++++++++++++++++++--------------------------
>  fs/binfmt_elf_fdpic.c    | 18 +++++---------
>  fs/coredump.c            | 55 +++++++++++++++++++++++++++++--------------
>  include/linux/binfmts.h  | 13 +----------
>  include/linux/coredump.h | 20 ++++++++++++----
>  5 files changed, 88 insertions(+), 79 deletions(-)
> 
> 
> Eric

This looks like a good clean-up to me. For the series:

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 1/5] coredump: Move definition of struct coredump_params into coredump.h

[kernel test robot]

Hi "Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Eric-W-Biederman/coredump-Move-definition-of-struct-coredump_params-into-coredump-h/20220201-034653
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 26291c54e111ff6ba87a164d85d4a4e134b7315c
config: m68k-m5208evb_defconfig (https://download.01.org/0day-ci/archive/20220201/202202010957.uywEsvch-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/51661b08028418cf7e46f97d7e7dbee927cd61e0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-W-Biederman/coredump-Move-definition-of-struct-coredump_params-into-coredump-h/20220201-034653
        git checkout 51661b08028418cf7e46f97d7e7dbee927cd61e0
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:29,
                    from fs/binfmt_flat.c:21:
   fs/binfmt_flat.c: In function 'flat_core_dump':
>> fs/binfmt_flat.c:118:50: error: invalid use of undefined type 'struct coredump_params'
     118 |                 current->comm, current->pid, cprm->siginfo->si_signo);
         |                                                  ^~
   include/linux/printk.h:418:33: note: in definition of macro 'printk_index_wrap'
     418 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                                 ^~~~~~~~~~~
   include/linux/printk.h:499:9: note: in expansion of macro 'printk'
     499 |         printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~
   fs/binfmt_flat.c:117:9: note: in expansion of macro 'pr_warn'
     117 |         pr_warn("Process %s:%d received signr %d and should have core dumped\n",
         |         ^~~~~~~


vim +118 fs/binfmt_flat.c

^1da177e4c3f41 Linus Torvalds   2005-04-16  108  
^1da177e4c3f41 Linus Torvalds   2005-04-16  109  /****************************************************************************/
^1da177e4c3f41 Linus Torvalds   2005-04-16  110  /*
^1da177e4c3f41 Linus Torvalds   2005-04-16  111   * Routine writes a core dump image in the current directory.
^1da177e4c3f41 Linus Torvalds   2005-04-16  112   * Currently only a stub-function.
^1da177e4c3f41 Linus Torvalds   2005-04-16  113   */
^1da177e4c3f41 Linus Torvalds   2005-04-16  114  
f6151dfea21496 Masami Hiramatsu 2009-12-17  115  static int flat_core_dump(struct coredump_params *cprm)
^1da177e4c3f41 Linus Torvalds   2005-04-16  116  {
4adbb6ac4b807e Nicolas Pitre    2016-07-24  117  	pr_warn("Process %s:%d received signr %d and should have core dumped\n",
13c3f50c914e6a Nicolas Pitre    2016-07-24 @118  		current->comm, current->pid, cprm->siginfo->si_signo);
13c3f50c914e6a Nicolas Pitre    2016-07-24  119  	return 1;
^1da177e4c3f41 Linus Torvalds   2005-04-16  120  }
^1da177e4c3f41 Linus Torvalds   2005-04-16  121  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/5] coredump: Move definition of struct coredump_params into coredump.h

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3881 bytes --]

Hi "Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Eric-W-Biederman/coredump-Move-definition-of-struct-coredump_params-into-coredump-h/20220201-034653
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 26291c54e111ff6ba87a164d85d4a4e134b7315c
config: m68k-m5208evb_defconfig (https://download.01.org/0day-ci/archive/20220201/202202010957.uywEsvch-lkp(a)intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/51661b08028418cf7e46f97d7e7dbee927cd61e0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-W-Biederman/coredump-Move-definition-of-struct-coredump_params-into-coredump-h/20220201-034653
        git checkout 51661b08028418cf7e46f97d7e7dbee927cd61e0
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:29,
                    from fs/binfmt_flat.c:21:
   fs/binfmt_flat.c: In function 'flat_core_dump':
>> fs/binfmt_flat.c:118:50: error: invalid use of undefined type 'struct coredump_params'
     118 |                 current->comm, current->pid, cprm->siginfo->si_signo);
         |                                                  ^~
   include/linux/printk.h:418:33: note: in definition of macro 'printk_index_wrap'
     418 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                                 ^~~~~~~~~~~
   include/linux/printk.h:499:9: note: in expansion of macro 'printk'
     499 |         printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~
   fs/binfmt_flat.c:117:9: note: in expansion of macro 'pr_warn'
     117 |         pr_warn("Process %s:%d received signr %d and should have core dumped\n",
         |         ^~~~~~~


vim +118 fs/binfmt_flat.c

^1da177e4c3f41 Linus Torvalds   2005-04-16  108  
^1da177e4c3f41 Linus Torvalds   2005-04-16  109  /****************************************************************************/
^1da177e4c3f41 Linus Torvalds   2005-04-16  110  /*
^1da177e4c3f41 Linus Torvalds   2005-04-16  111   * Routine writes a core dump image in the current directory.
^1da177e4c3f41 Linus Torvalds   2005-04-16  112   * Currently only a stub-function.
^1da177e4c3f41 Linus Torvalds   2005-04-16  113   */
^1da177e4c3f41 Linus Torvalds   2005-04-16  114  
f6151dfea21496 Masami Hiramatsu 2009-12-17  115  static int flat_core_dump(struct coredump_params *cprm)
^1da177e4c3f41 Linus Torvalds   2005-04-16  116  {
4adbb6ac4b807e Nicolas Pitre    2016-07-24  117  	pr_warn("Process %s:%d received signr %d and should have core dumped\n",
13c3f50c914e6a Nicolas Pitre    2016-07-24 @118  		current->comm, current->pid, cprm->siginfo->si_signo);
13c3f50c914e6a Nicolas Pitre    2016-07-24  119  	return 1;
^1da177e4c3f41 Linus Torvalds   2005-04-16  120  }
^1da177e4c3f41 Linus Torvalds   2005-04-16  121  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 1/5] coredump: Move definition of struct coredump_params into coredump.h

[kernel test robot]

Hi "Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Eric-W-Biederman/coredump-Move-definition-of-struct-coredump_params-into-coredump-h/20220201-034653
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 26291c54e111ff6ba87a164d85d4a4e134b7315c
config: riscv-buildonly-randconfig-r005-20220131 (https://download.01.org/0day-ci/archive/20220201/202202011230.eerL23uM-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 2cdbaca3943a4d6259119f185656328bd3805b68)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/51661b08028418cf7e46f97d7e7dbee927cd61e0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-W-Biederman/coredump-Move-definition-of-struct-coredump_params-into-coredump-h/20220201-034653
        git checkout 51661b08028418cf7e46f97d7e7dbee927cd61e0
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/binfmt_flat.c:118:36: error: incomplete definition of type 'struct coredump_params'
                   current->comm, current->pid, cprm->siginfo->si_signo);
                                                ~~~~^
   include/linux/printk.h:499:37: note: expanded from macro 'pr_warn'
           printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                                              ^~~~~~~~~~~
   include/linux/printk.h:446:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                              ^~~~~~~~~~~
   include/linux/printk.h:418:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                                   ^~~~~~~~~~~
   include/linux/binfmts.h:11:8: note: forward declaration of 'struct coredump_params'
   struct coredump_params;
          ^
   1 error generated.


vim +118 fs/binfmt_flat.c

^1da177e4c3f41 Linus Torvalds   2005-04-16  108  
^1da177e4c3f41 Linus Torvalds   2005-04-16  109  /****************************************************************************/
^1da177e4c3f41 Linus Torvalds   2005-04-16  110  /*
^1da177e4c3f41 Linus Torvalds   2005-04-16  111   * Routine writes a core dump image in the current directory.
^1da177e4c3f41 Linus Torvalds   2005-04-16  112   * Currently only a stub-function.
^1da177e4c3f41 Linus Torvalds   2005-04-16  113   */
^1da177e4c3f41 Linus Torvalds   2005-04-16  114  
f6151dfea21496 Masami Hiramatsu 2009-12-17  115  static int flat_core_dump(struct coredump_params *cprm)
^1da177e4c3f41 Linus Torvalds   2005-04-16  116  {
4adbb6ac4b807e Nicolas Pitre    2016-07-24  117  	pr_warn("Process %s:%d received signr %d and should have core dumped\n",
13c3f50c914e6a Nicolas Pitre    2016-07-24 @118  		current->comm, current->pid, cprm->siginfo->si_signo);
13c3f50c914e6a Nicolas Pitre    2016-07-24  119  	return 1;
^1da177e4c3f41 Linus Torvalds   2005-04-16  120  }
^1da177e4c3f41 Linus Torvalds   2005-04-16  121  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/5] coredump: Move definition of struct coredump_params into coredump.h

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4109 bytes --]

Hi "Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Eric-W-Biederman/coredump-Move-definition-of-struct-coredump_params-into-coredump-h/20220201-034653
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 26291c54e111ff6ba87a164d85d4a4e134b7315c
config: riscv-buildonly-randconfig-r005-20220131 (https://download.01.org/0day-ci/archive/20220201/202202011230.eerL23uM-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 2cdbaca3943a4d6259119f185656328bd3805b68)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/51661b08028418cf7e46f97d7e7dbee927cd61e0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-W-Biederman/coredump-Move-definition-of-struct-coredump_params-into-coredump-h/20220201-034653
        git checkout 51661b08028418cf7e46f97d7e7dbee927cd61e0
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/binfmt_flat.c:118:36: error: incomplete definition of type 'struct coredump_params'
                   current->comm, current->pid, cprm->siginfo->si_signo);
                                                ~~~~^
   include/linux/printk.h:499:37: note: expanded from macro 'pr_warn'
           printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                                              ^~~~~~~~~~~
   include/linux/printk.h:446:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                              ^~~~~~~~~~~
   include/linux/printk.h:418:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                                   ^~~~~~~~~~~
   include/linux/binfmts.h:11:8: note: forward declaration of 'struct coredump_params'
   struct coredump_params;
          ^
   1 error generated.


vim +118 fs/binfmt_flat.c

^1da177e4c3f41 Linus Torvalds   2005-04-16  108  
^1da177e4c3f41 Linus Torvalds   2005-04-16  109  /****************************************************************************/
^1da177e4c3f41 Linus Torvalds   2005-04-16  110  /*
^1da177e4c3f41 Linus Torvalds   2005-04-16  111   * Routine writes a core dump image in the current directory.
^1da177e4c3f41 Linus Torvalds   2005-04-16  112   * Currently only a stub-function.
^1da177e4c3f41 Linus Torvalds   2005-04-16  113   */
^1da177e4c3f41 Linus Torvalds   2005-04-16  114  
f6151dfea21496 Masami Hiramatsu 2009-12-17  115  static int flat_core_dump(struct coredump_params *cprm)
^1da177e4c3f41 Linus Torvalds   2005-04-16  116  {
4adbb6ac4b807e Nicolas Pitre    2016-07-24  117  	pr_warn("Process %s:%d received signr %d and should have core dumped\n",
13c3f50c914e6a Nicolas Pitre    2016-07-24 @118  		current->comm, current->pid, cprm->siginfo->si_signo);
13c3f50c914e6a Nicolas Pitre    2016-07-24  119  	return 1;
^1da177e4c3f41 Linus Torvalds   2005-04-16  120  }
^1da177e4c3f41 Linus Torvalds   2005-04-16  121  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 2/5] coredump: Snapshot the vmas in do_coredump

[Jann Horn]

On Mon, Jan 31, 2022 at 7:46 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Move the call of dump_vma_snapshot and kvfree(vma_meta) out of the
> individual coredump routines into do_coredump itself.  This makes
> the code less error prone and easier to maintain.
>
> Make the vma snapshot available to the coredump routines
> in struct coredump_params.  This makes it easier to
> change and update what is captures in the vma snapshot
> and will be needed for fixing fill_file_notes.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Jann Horn <jannh@google.com>

>         for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
>                         vma = next_vma(vma, gate_vma), i++) {
> -               struct core_vma_metadata *m = (*vma_meta) + i;
> +               struct core_vma_metadata *m = cprm->vma_meta + i;
>
>                 m->start = vma->vm_start;
>                 m->end = vma->vm_end;
>                 m->flags = vma->vm_flags;
>                 m->dump_size = vma_dump_size(vma, cprm->mm_flags);
>
> -               vma_data_size += m->dump_size;
> +               cprm->vma_data_size += m->dump_size;

FYI, this part is probably going to cause a merge conflict with the
fix https://www.ozlabs.org/~akpm/mmotm/broken-out/coredump-also-dump-first-pages-of-non-executable-elf-libraries.patch
in akpm's tree. I don't know what the right way to handle that is,
just thought I'd point it out.

Re: [PATCH 3/5] coredump: Remove the WARN_ON in dump_vma_snapshot

[Jann Horn]

On Mon, Jan 31, 2022 at 7:46 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> The condition is impossible and to the best of my knowledge has never
> triggered.
>
> We are in deep trouble if that conditions happens and we walk past
> the end of our allocated array.
>
> So delete the WARN_ON and the code that makes it look like the kernel
> can handle the case of walking past the end of it's vma_meta array.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Jann Horn <jannh@google.com>

Re: [PATCH 4/5] coredump/elf: Pass coredump_params into fill_note_info

[Jann Horn]

On Mon, Jan 31, 2022 at 7:47 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Instead of individually passing cprm->siginfo and cprm->regs
> into fill_note_info pass all of struct coredump_params.
>
> This is preparation to allow fill_files_note to use the existing
> vma snapshot.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Jann Horn <jannh@google.com>

Re: [PATCH 5/5] coredump: Use the vma snapshot in fill_files_note

[Jann Horn]

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)

> +       }
> +}

Re: [PATCH 5/5] coredump: Use the vma snapshot in fill_files_note

[Eric W. Biederman]

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

Re: [PATCH 2/5] coredump: Snapshot the vmas in do_coredump

[Eric W. Biederman]

Jann Horn <jannh@google.com> writes:

> On Mon, Jan 31, 2022 at 7:46 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Move the call of dump_vma_snapshot and kvfree(vma_meta) out of the
>> individual coredump routines into do_coredump itself.  This makes
>> the code less error prone and easier to maintain.
>>
>> Make the vma snapshot available to the coredump routines
>> in struct coredump_params.  This makes it easier to
>> change and update what is captures in the vma snapshot
>> and will be needed for fixing fill_file_notes.
>>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Reviewed-by: Jann Horn <jannh@google.com>
>
>>         for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
>>                         vma = next_vma(vma, gate_vma), i++) {
>> -               struct core_vma_metadata *m = (*vma_meta) + i;
>> +               struct core_vma_metadata *m = cprm->vma_meta + i;
>>
>>                 m->start = vma->vm_start;
>>                 m->end = vma->vm_end;
>>                 m->flags = vma->vm_flags;
>>                 m->dump_size = vma_dump_size(vma, cprm->mm_flags);
>>
>> -               vma_data_size += m->dump_size;
>> +               cprm->vma_data_size += m->dump_size;
>
> FYI, this part is probably going to cause a merge conflict with the
> fix https://www.ozlabs.org/~akpm/mmotm/broken-out/coredump-also-dump-first-pages-of-non-executable-elf-libraries.patch
> in akpm's tree. I don't know what the right way to handle that is,
> just thought I'd point it out.

There are not any conflicts in principle we could just let resolution
handle it.  Unfortunately both are candidates for backporting.

Either we replace your fix with a simple deletion of the executable
check, or I need to base mine on yours.

Since I need to repost mine anyway I will look at the latter.

Eric

[GIT PULL] Fix fill_files_note

[Eric W. Biederman]

Kees,

Please pull the coredump-vma-snapshot-fix branch from the git tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git coredump-vma-snapshot-fix

  HEAD: 390031c942116d4733310f0684beb8db19885fe6 coredump: Use the vma snapshot in fill_files_note

Matthew Wilcox has reported that a missing mmap_lock in file_files_note,
which could cause trouble.

Refactor the code and clean it up so that the vma snapshot makes
it to fill_files_note, and then use the vma snapshot in fill_files_note.

Eric W. Biederman (5):
      coredump: Move definition of struct coredump_params into coredump.h
      coredump: Snapshot the vmas in do_coredump
      coredump: Remove the WARN_ON in dump_vma_snapshot
      coredump/elf: Pass coredump_params into fill_note_info
      coredump: Use the vma snapshot in fill_files_note

 fs/binfmt_elf.c          | 66 ++++++++++++++++++++++--------------------------
 fs/binfmt_elf_fdpic.c    | 18 +++++--------
 fs/binfmt_flat.c         |  1 +
 fs/coredump.c            | 59 ++++++++++++++++++++++++++++---------------
 include/linux/binfmts.h  | 13 +---------
 include/linux/coredump.h | 20 ++++++++++++---
 6 files changed, 93 insertions(+), 84 deletions(-)

---

Kees I realized I needed to rebase this on Jann Horn's commit
84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF
libraries").  Unfortunately before I got that done I got distracted and
these changes have been sitting in limbo for most of the development
cycle.  Since you are running a tree that is including changes like this
including Jann's can you please pull these changes into your tree.

Thank you,
Eric

Re: [GIT PULL] Fix fill_files_note

[Kees Cook]

On Tue, Mar 08, 2022 at 01:35:03PM -0600, Eric W. Biederman wrote:
> 
> Kees,
> 
> Please pull the coredump-vma-snapshot-fix branch from the git tree:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git coredump-vma-snapshot-fix
> 
>   HEAD: 390031c942116d4733310f0684beb8db19885fe6 coredump: Use the vma snapshot in fill_files_note
> 
> Matthew Wilcox has reported that a missing mmap_lock in file_files_note,
> which could cause trouble.
> 
> Refactor the code and clean it up so that the vma snapshot makes
> it to fill_files_note, and then use the vma snapshot in fill_files_note.
> 
> Eric W. Biederman (5):
>       coredump: Move definition of struct coredump_params into coredump.h
>       coredump: Snapshot the vmas in do_coredump
>       coredump: Remove the WARN_ON in dump_vma_snapshot
>       coredump/elf: Pass coredump_params into fill_note_info
>       coredump: Use the vma snapshot in fill_files_note
> 
>  fs/binfmt_elf.c          | 66 ++++++++++++++++++++++--------------------------
>  fs/binfmt_elf_fdpic.c    | 18 +++++--------
>  fs/binfmt_flat.c         |  1 +
>  fs/coredump.c            | 59 ++++++++++++++++++++++++++++---------------
>  include/linux/binfmts.h  | 13 +---------
>  include/linux/coredump.h | 20 ++++++++++++---
>  6 files changed, 93 insertions(+), 84 deletions(-)
> 
> ---
> 
> Kees I realized I needed to rebase this on Jann Horn's commit
> 84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF
> libraries").  Unfortunately before I got that done I got distracted and
> these changes have been sitting in limbo for most of the development
> cycle.  Since you are running a tree that is including changes like this
> including Jann's can you please pull these changes into your tree.

Sure! Can you make a signed tag for this pull?


If it helps, my workflow look like this, though I assume there might be
better ways. (tl;dr: "git tag -s TAG BRANCH")


PULL_BRANCH=name-of-branch
BASE=sha-of-base
FOR=someone
TOPIC=topic-name

TAG="for-$FOR/$TOPIC"
SIGNED=~/.pull-request-signed-"$TAG"
echo "$TOPIC update" > "$SIGNED"
git request-pull "$BASE" git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git "$PULL_BRANCH" | awk '{print "# " $0}' >> "$SIGNED"
vi "$SIGNED"

git tag -sF "$SIGNED" "$TAG" "$PULL_BRANCH"
git push origin "$PULL_BRANCH"
git push origin +"$TAG"


-- 
Kees Cook

Re: [GIT PULL] Fix fill_files_note

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> On Tue, Mar 08, 2022 at 01:35:03PM -0600, Eric W. Biederman wrote:
>> 
>> Kees,
>> 
>> Please pull the coredump-vma-snapshot-fix branch from the git tree:
>> 
>>   git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git coredump-vma-snapshot-fix
>> 
>>   HEAD: 390031c942116d4733310f0684beb8db19885fe6 coredump: Use the vma snapshot in fill_files_note
>> 
>> Matthew Wilcox has reported that a missing mmap_lock in file_files_note,
>> which could cause trouble.
>> 
>> Refactor the code and clean it up so that the vma snapshot makes
>> it to fill_files_note, and then use the vma snapshot in fill_files_note.
>> 
>> Eric W. Biederman (5):
>>       coredump: Move definition of struct coredump_params into coredump.h
>>       coredump: Snapshot the vmas in do_coredump
>>       coredump: Remove the WARN_ON in dump_vma_snapshot
>>       coredump/elf: Pass coredump_params into fill_note_info
>>       coredump: Use the vma snapshot in fill_files_note
>> 
>>  fs/binfmt_elf.c          | 66 ++++++++++++++++++++++--------------------------
>>  fs/binfmt_elf_fdpic.c    | 18 +++++--------
>>  fs/binfmt_flat.c         |  1 +
>>  fs/coredump.c            | 59 ++++++++++++++++++++++++++++---------------
>>  include/linux/binfmts.h  | 13 +---------
>>  include/linux/coredump.h | 20 ++++++++++++---
>>  6 files changed, 93 insertions(+), 84 deletions(-)
>> 
>> ---
>> 
>> Kees I realized I needed to rebase this on Jann Horn's commit
>> 84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF
>> libraries").  Unfortunately before I got that done I got distracted and
>> these changes have been sitting in limbo for most of the development
>> cycle.  Since you are running a tree that is including changes like this
>> including Jann's can you please pull these changes into your tree.
>
> Sure! Can you make a signed tag for this pull?

Not yet.

Hopefully I will get the time to set that up soon, but I am not at all
setup to do signed tags at this point.

> If it helps, my workflow look like this, though I assume there might be
> better ways. (tl;dr: "git tag -s TAG BRANCH")
>
>
> PULL_BRANCH=name-of-branch
> BASE=sha-of-base
> FOR=someone
> TOPIC=topic-name
>
> TAG="for-$FOR/$TOPIC"
> SIGNED=~/.pull-request-signed-"$TAG"
> echo "$TOPIC update" > "$SIGNED"
> git request-pull "$BASE" git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git "$PULL_BRANCH" | awk '{print "# " $0}' >> "$SIGNED"
> vi "$SIGNED"
>
> git tag -sF "$SIGNED" "$TAG" "$PULL_BRANCH"
> git push origin "$PULL_BRANCH"
> git push origin +"$TAG"

Thanks.  That looks like a good place to start.

Eric

Re: [GIT PULL] Fix fill_files_note

[Kees Cook]

On Wed, Mar 09, 2022 at 10:29:10AM -0600, Eric W. Biederman wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > On Tue, Mar 08, 2022 at 01:35:03PM -0600, Eric W. Biederman wrote:
> >> 
> >> Kees,
> >> 
> >> Please pull the coredump-vma-snapshot-fix branch from the git tree:
> >> 
> >>   git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git coredump-vma-snapshot-fix
> >> 
> >>   HEAD: 390031c942116d4733310f0684beb8db19885fe6 coredump: Use the vma snapshot in fill_files_note
> >> 
> >> Matthew Wilcox has reported that a missing mmap_lock in file_files_note,
> >> which could cause trouble.
> >> 
> >> Refactor the code and clean it up so that the vma snapshot makes
> >> it to fill_files_note, and then use the vma snapshot in fill_files_note.
> >> 
> >> Eric W. Biederman (5):
> >>       coredump: Move definition of struct coredump_params into coredump.h
> >>       coredump: Snapshot the vmas in do_coredump
> >>       coredump: Remove the WARN_ON in dump_vma_snapshot
> >>       coredump/elf: Pass coredump_params into fill_note_info
> >>       coredump: Use the vma snapshot in fill_files_note
> >> 
> >>  fs/binfmt_elf.c          | 66 ++++++++++++++++++++++--------------------------
> >>  fs/binfmt_elf_fdpic.c    | 18 +++++--------
> >>  fs/binfmt_flat.c         |  1 +
> >>  fs/coredump.c            | 59 ++++++++++++++++++++++++++++---------------
> >>  include/linux/binfmts.h  | 13 +---------
> >>  include/linux/coredump.h | 20 ++++++++++++---
> >>  6 files changed, 93 insertions(+), 84 deletions(-)
> >> 
> >> ---
> >> 
> >> Kees I realized I needed to rebase this on Jann Horn's commit
> >> 84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF
> >> libraries").  Unfortunately before I got that done I got distracted and
> >> these changes have been sitting in limbo for most of the development
> >> cycle.  Since you are running a tree that is including changes like this
> >> including Jann's can you please pull these changes into your tree.
> >
> > Sure! Can you make a signed tag for this pull?
> 
> Not yet.
> 
> Hopefully I will get the time to set that up soon, but I am not at all
> setup to do signed tags at this point.

Okay, cool. Since I'd already review these before, I've pulled and it
should be in -next now.

> [...]
> Thanks.  That looks like a good place to start.

I will try to clean up that work-flow and stuff it into my kernel-tools
repo.

-- 
Kees Cook

Re: [GIT PULL] Fix fill_files_note

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> On Wed, Mar 09, 2022 at 10:29:10AM -0600, Eric W. Biederman wrote:
>> Kees Cook <keescook@chromium.org> writes:
>> 
>> > On Tue, Mar 08, 2022 at 01:35:03PM -0600, Eric W. Biederman wrote:
>> >> 
>> >> Kees,
>> >> 
>> >> Please pull the coredump-vma-snapshot-fix branch from the git tree:
>> >> 
>> >>   git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git coredump-vma-snapshot-fix
>> >> 
>> >>   HEAD: 390031c942116d4733310f0684beb8db19885fe6 coredump: Use the vma snapshot in fill_files_note
>> >> 
>> >> Matthew Wilcox has reported that a missing mmap_lock in file_files_note,
>> >> which could cause trouble.
>> >> 
>> >> Refactor the code and clean it up so that the vma snapshot makes
>> >> it to fill_files_note, and then use the vma snapshot in fill_files_note.
>> >> 
>> >> Eric W. Biederman (5):
>> >>       coredump: Move definition of struct coredump_params into coredump.h
>> >>       coredump: Snapshot the vmas in do_coredump
>> >>       coredump: Remove the WARN_ON in dump_vma_snapshot
>> >>       coredump/elf: Pass coredump_params into fill_note_info
>> >>       coredump: Use the vma snapshot in fill_files_note
>> >> 
>> >>  fs/binfmt_elf.c          | 66 ++++++++++++++++++++++--------------------------
>> >>  fs/binfmt_elf_fdpic.c    | 18 +++++--------
>> >>  fs/binfmt_flat.c         |  1 +
>> >>  fs/coredump.c            | 59 ++++++++++++++++++++++++++++---------------
>> >>  include/linux/binfmts.h  | 13 +---------
>> >>  include/linux/coredump.h | 20 ++++++++++++---
>> >>  6 files changed, 93 insertions(+), 84 deletions(-)
>> >> 
>> >> ---
>> >> 
>> >> Kees I realized I needed to rebase this on Jann Horn's commit
>> >> 84158b7f6a06 ("coredump: Also dump first pages of non-executable ELF
>> >> libraries").  Unfortunately before I got that done I got distracted and
>> >> these changes have been sitting in limbo for most of the development
>> >> cycle.  Since you are running a tree that is including changes like this
>> >> including Jann's can you please pull these changes into your tree.
>> >
>> > Sure! Can you make a signed tag for this pull?
>> 
>> Not yet.
>> 
>> Hopefully I will get the time to set that up soon, but I am not at all
>> setup to do signed tags at this point.
>
> Okay, cool. Since I'd already review these before, I've pulled and it
> should be in -next now.


>
>> [...]
>> Thanks.  That looks like a good place to start.
>
> I will try to clean up that work-flow and stuff it into my kernel-tools
> repo.

It turns out I missed a crazy corner case of binfmt_flat, when coredumps
are disabled.  This fixes a compile error that was reported.

   git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git coredump-vma-snapshot-fix-for-v5.18
   HEAD: f833116ad2c3eabf9c739946170e07825cca67ed coredump: Don't compile flat_core_dump when coredumps are disabled

Can you include this as well.

Thank you,
Eric

This is the entire patch.

From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Wed, 9 Mar 2022 10:37:07 -0600
Subject: [PATCH] coredump: Don't compile flat_core_dump when coredumps are disabled

Recently the kernel test robot reported:
> In file included from include/linux/kernel.h:29,
>                     from fs/binfmt_flat.c:21:
>    fs/binfmt_flat.c: In function 'flat_core_dump':
> >> fs/binfmt_flat.c:121:50: error: invalid use of undefined type 'struct coredump_params'
>      121 |                 current->comm, current->pid, cprm->siginfo->si_signo);
>          |                                                  ^~
>    include/linux/printk.h:418:33: note: in definition of macro 'printk_index_wrap'
>      418 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
>          |                                 ^~~~~~~~~~~
>    include/linux/printk.h:499:9: note: in expansion of macro 'printk'
>      499 |         printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
>          |         ^~~~~~
>    fs/binfmt_flat.c:120:9: note: in expansion of macro 'pr_warn'
>      120 |         pr_warn("Process %s:%d received signr %d and should have core dumped\n",
>          |         ^~~~~~~
>    At top level:
>    fs/binfmt_flat.c:118:12: warning: 'flat_core_dump' defined but not used [-Wunused-function]
>      118 | static int flat_core_dump(struct coredump_params *cprm)
>          |            ^~~~~~~~~~~~~~

The little dinky do nothing function flat_core_dump has always been
compiled unconditionally.  With my change to move coredump_params into
coredump.h coredump_params reasonably becomes unavailable when
coredump support is not compiled in.  Fix this old issue by simply not
compiling flat_core_dump when coredump support is not supported.

Fixes: a99a3e2efaf1 ("coredump: Move definition of struct coredump_params into coredump.h")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/binfmt_flat.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 208cdce16de1..626898150011 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -98,7 +98,9 @@ static int load_flat_shared_library(int id, struct lib_info *p);
 #endif
 
 static int load_flat_binary(struct linux_binprm *);
+#ifdef CONFIG_COREDUMP
 static int flat_core_dump(struct coredump_params *cprm);
+#endif
 
 static struct linux_binfmt flat_format = {
 	.module		= THIS_MODULE,
@@ -115,12 +117,14 @@ static struct linux_binfmt flat_format = {
  * Currently only a stub-function.
  */
 
+#ifdef CONFIG_COREDUMP
 static int flat_core_dump(struct coredump_params *cprm)
 {
 	pr_warn("Process %s:%d received signr %d and should have core dumped\n",
 		current->comm, current->pid, cprm->siginfo->si_signo);
 	return 1;
 }
+#endif
 
 /****************************************************************************/
 /*
-- 
2.29.2

Re: [GIT PULL] Fix fill_files_note

[Kees Cook]

On Wed, Mar 09, 2022 at 02:27:07PM -0600, Eric W. Biederman wrote:
> It turns out I missed a crazy corner case of binfmt_flat, when coredumps
> are disabled.  This fixes a compile error that was reported.
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git coredump-vma-snapshot-fix-for-v5.18
>    HEAD: f833116ad2c3eabf9c739946170e07825cca67ed coredump: Don't compile flat_core_dump when coredumps are disabled
> 
> Can you include this as well.

Thanks! Pulled and pushed out.

-- 
Kees Cook