[PATCH] fs/coredump/elf: Clean up fill_thread_core_info()

[Ingo Molnar <mingo@kernel.org>]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> > 	xstate note on boxes with xsaves support can leak uninitialized data
> > into coredumps
> > 
> > The following changes since commit 4e89b7210403fa4a8acafe7c602b6212b7af6c3b:
> > 
> >   fix multiplication overflow in copy_fdtable() (2020-05-19 18:29:36 -0400)
> > 
> > are available in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git fixes
> > 
> > for you to fetch changes up to 9e4636545933131de15e1ecd06733538ae939b2f:
> > 
> >   copy_xstate_to_kernel(): don't leave parts of destination uninitialized (2020-05-27 17:06:31 -0400)
> > 
> > ----------------------------------------------------------------
> > Al Viro (1):
> >       copy_xstate_to_kernel(): don't leave parts of destination uninitialized
> > 
> >  arch/x86/kernel/fpu/xstate.c | 86 ++++++++++++++++++++++++--------------------
> >  1 file changed, 48 insertions(+), 38 deletions(-)
> 
> Looks good to me.
> 
> I'm wondering, shouldn't we also zero-initialize the dump data to 
> begin with? See the patch below (untested).
> 
> Thanks,
> 
> 	Ingo
> 
>  fs/binfmt_elf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 13f25e241ac4..25d489bc9453 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1733,7 +1733,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
>  		    (!regset->active || regset->active(t->task, regset) > 0)) {
>  			int ret;
>  			size_t size = regset_size(t->task, regset);
> -			void *data = kmalloc(size, GFP_KERNEL);
> +			void *data = kzalloc(size, GFP_KERNEL);
>  			if (unlikely(!data))
>  				return 0;
>  			ret = regset->get(t->task, regset,

The clean-up patch below on top of the zeroing patch above makes 
fill_thread_core_info() readable for me:

 - Use a proper iterator pattern and merge the special case '0' into 
   the 1..n-1 iterator.

 - Clean up the flow of logic in the iterator to more standard 
   patterns, to see the progress of work versus a mix of uncommon 
   failure causes with the typical branch.

 - Add a WARN_ON_ONCE() for a silent assumption about NT_PRSTATUS 
   semantics.

 - Get rid of a copious amount of col80 line breaks created by 
   copy & paste of overly verbose repetitive patterns.

 - Clean up small details like 10 year old "fill the reset" typos in 
   comments, unbalanced curly braces, etc.

Now that the compiler can see what we are doing the code likely got a 
tiny bit faster as well, because the code shrunk a bit if we discount 
the extra WARN_ON_ONCE():

  # fs/binfmt_elf.o:

   text	   data	    bss	    dec	    hex	filename
  14410	    108	      0	  14518	   38b6	binfmt_elf.o.before
  14381	    108	      0	  14489	   3899	binfmt_elf.o.after

(Assuming it's not due to a bug - this is untested.)

Thanks,

	Ingo

Signed-off-by-if-you-first-test-it: Ingo Molnar <mingo@kernel.org>

---
 fs/binfmt_elf.c | 61 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 25d489bc9453..3f9f299169fd 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1703,56 +1703,61 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 				 long signr, size_t *total)
 {
 	unsigned int i;
-	unsigned int regset0_size = regset_size(t->task, &view->regsets[0]);
+	const struct user_regset *regset = &view->regsets[0];
+	struct memelfnote *note = &t->notes[0];
+	unsigned int size = regset_size(t->task, regset);
+	int ret;
 
 	/*
 	 * NT_PRSTATUS is the one special case, because the regset data
 	 * goes into the pr_reg field inside the note contents, rather
-	 * than being the whole note contents.  We fill the reset in here.
+	 * than being the whole note contents.  We fill the regset in here.
 	 * We assume that regset 0 is NT_PRSTATUS.
 	 */
 	fill_prstatus(&t->prstatus, t->task, signr);
-	(void) view->regsets[0].get(t->task, &view->regsets[0], 0, regset0_size,
-				    &t->prstatus.pr_reg, NULL);
+	ret = regset->get(t->task, regset, 0, size, &t->prstatus.pr_reg, NULL);
+	/* NT_PRSTATUS is not supposed to fail: */
+	WARN_ON_ONCE(ret);
 
-	fill_note(&t->notes[0], "CORE", NT_PRSTATUS,
-		  PRSTATUS_SIZE(t->prstatus, regset0_size), &t->prstatus);
-	*total += notesize(&t->notes[0]);
+	fill_note(note, "CORE", NT_PRSTATUS, PRSTATUS_SIZE(t->prstatus, size), &t->prstatus);
+	*total += notesize(note);
 
 	do_thread_regset_writeback(t->task, &view->regsets[0]);
 
 	/*
-	 * Each other regset might generate a note too.  For each regset
-	 * that has no core_note_type or is inactive, we leave t->notes[i]
+	 * Subsequent regsets might generate a note too.  For each regset
+	 * that has no ->core_note_type or is inactive, we leave t->notes[i]
 	 * all zero and we'll know to skip writing it later.
 	 */
 	for (i = 1; i < view->n; ++i) {
-		const struct user_regset *regset = &view->regsets[i];
+		regset++;
+		note++;
+
 		do_thread_regset_writeback(t->task, regset);
+
 		if (regset->core_note_type && regset->get &&
 		    (!regset->active || regset->active(t->task, regset) > 0)) {
-			int ret;
-			size_t size = regset_size(t->task, regset);
-			void *data = kzalloc(size, GFP_KERNEL);
+			void *data;
+
+			size = regset_size(t->task, regset);
+
+			data = kzalloc(size, GFP_KERNEL);
 			if (unlikely(!data))
 				return 0;
-			ret = regset->get(t->task, regset,
-					  0, size, data, NULL);
-			if (unlikely(ret))
+
+			ret = regset->get(t->task, regset, 0, size, data, NULL);
+			if (unlikely(ret)) {
 				kfree(data);
-			else {
-				if (regset->core_note_type != NT_PRFPREG)
-					fill_note(&t->notes[i], "LINUX",
-						  regset->core_note_type,
-						  size, data);
-				else {
-					SET_PR_FPVALID(&t->prstatus,
-							1, regset0_size);
-					fill_note(&t->notes[i], "CORE",
-						  NT_PRFPREG, size, data);
-				}
-				*total += notesize(&t->notes[i]);
+				continue;
+			}
+
+			if (regset->core_note_type != NT_PRFPREG) {
+				fill_note(note, "LINUX", regset->core_note_type, size, data);
+			} else {
+				SET_PR_FPVALID(&t->prstatus, 1, regset0_size);
+				fill_note(note, "CORE", NT_PRFPREG, size, data);
 			}
+			*total += notesize(note);
 		}
 	}