linux-kernel - [PATCH v2] fs/coredump/elf: Clean up fill_thread_core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200528074055.GA111020@gmail.com>
Date:   Thu, 28 May 2020 09:40:55 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, x86@...nel.org
Subject: [PATCH v2] fs/coredump/elf: Clean up fill_thread_core_info()


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

> > 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@...nel.org>

> +				SET_PR_FPVALID(&t->prstatus, 1, regset0_size);

Meh, I broke the x86-32 build with this, in part because on 64-bit 
SET_PR_FPVALID() silently ignores the third argument.

The patch below, folded into the cleanup patch, does the following:

 - fixes the bug I introduced.

 - makes SET_PR_FPVALID() use all three arguments on 64-bit systems 
   too, to keep dorks like me from breaking the code.

 - fixes a minor macro assumption in arch/x86/include/asm/compat.h

Still an overall win, if we compare it without the WARN_ON():

  # 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

Thanks,

	Ingo

---
 arch/x86/include/asm/compat.h |  2 +-
 fs/binfmt_elf.c               | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
index 52e9f3480f69..2f5ff3c3416b 100644
--- a/arch/x86/include/asm/compat.h
+++ b/arch/x86/include/asm/compat.h
@@ -169,7 +169,7 @@ typedef struct user_regs_struct compat_elf_gregset_t;
 /* Full regset -- prstatus on x32, otherwise on ia32 */
 #define PRSTATUS_SIZE(S, R) (R != sizeof(S.pr_reg) ? 144 : 296)
 #define SET_PR_FPVALID(S, V, R) \
-  do { *(int *) (((void *) &((S)->pr_reg)) + R) = (V); } \
+  do { *(int *) (((void *) &((S)->pr_reg)) + (R)) = (V); } \
   while (0)
 
 #ifdef CONFIG_X86_X32_ABI
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 3f9f299169fd..bc347179df0f 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1695,7 +1695,7 @@ static void do_thread_regset_writeback(struct task_struct *task,
 #endif
 
 #ifndef SET_PR_FPVALID
-#define SET_PR_FPVALID(S, V, R) ((S)->pr_fpvalid = (V))
+#define SET_PR_FPVALID(S, V, R) ((void)(R), (S)->pr_fpvalid = (V))
 #endif
 
 static int fill_thread_core_info(struct elf_thread_core_info *t,
@@ -1705,7 +1705,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 	unsigned int i;
 	const struct user_regset *regset = &view->regsets[0];
 	struct memelfnote *note = &t->notes[0];
-	unsigned int size = regset_size(t->task, regset);
+	unsigned int size, size0 = regset_size(t->task, regset);
 	int ret;
 
 	/*
@@ -1715,11 +1715,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 	 * We assume that regset 0 is NT_PRSTATUS.
 	 */
 	fill_prstatus(&t->prstatus, t->task, signr);
-	ret = regset->get(t->task, regset, 0, size, &t->prstatus.pr_reg, NULL);
+	ret = regset->get(t->task, regset, 0, size0, &t->prstatus.pr_reg, NULL);
 	/* NT_PRSTATUS is not supposed to fail: */
 	WARN_ON_ONCE(ret);
 
-	fill_note(note, "CORE", NT_PRSTATUS, PRSTATUS_SIZE(t->prstatus, size), &t->prstatus);
+	fill_note(note, "CORE", NT_PRSTATUS, PRSTATUS_SIZE(t->prstatus, size0), &t->prstatus);
 	*total += notesize(note);
 
 	do_thread_regset_writeback(t->task, &view->regsets[0]);
@@ -1754,7 +1754,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 			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);
+				SET_PR_FPVALID(&t->prstatus, 1, size0);
 				fill_note(note, "CORE", NT_PRFPREG, size, data);
 			}
 			*total += notesize(note);