[PATCH] fs/coredump/elf: Clean up fill_thread_core_info()
From: Ingo Molnar
Date: Thu May 28 2020 - 03:29:41 EST
* Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
---
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);
}
}