[git pull] coredump infoleak fix

[git pull] coredump infoleak fix

[Al Viro]

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

Re: [git pull] coredump infoleak fix

[Ingo Molnar]

* 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,

Re: [git pull] coredump infoleak fix

[Al Viro]

On Thu, May 28, 2020 at 09:02:55AM +0200, Ingo Molnar wrote:

> Looks good to me.
> 
> I'm wondering, shouldn't we also zero-initialize the dump data to 
> begin with? See the patch below (untested).

Note that this hides the bug from KASAN, though ;-)  And the bug
is not just infoleak - not all components are "all zeroes" in the
init state.

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

[Ingo Molnar]

* 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);
 		}
 	}
 

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

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.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@kernel.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);

Re: [git pull] coredump infoleak fix

[Ingo Molnar]

* Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Thu, May 28, 2020 at 09:02:55AM +0200, Ingo Molnar wrote:
> 
> > Looks good to me.
> > 
> > I'm wondering, shouldn't we also zero-initialize the dump data to 
> > begin with? See the patch below (untested).
> 
> Note that this hides the bug from KASAN, though ;-)  And the bug
> is not just infoleak - not all components are "all zeroes" in the
> init state.

Yeah, but is zero-init really a problem though? Wouldn't it be 
'better' to have all zeroes if the dump doesn't fit? But I might be 
missing something ...

Thanks,

	Ingo

Re: [git pull] coredump infoleak fix

[Al Viro]

On Thu, May 28, 2020 at 09:44:42AM +0200, Ingo Molnar wrote:
> 
> * Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> > On Thu, May 28, 2020 at 09:02:55AM +0200, Ingo Molnar wrote:
> > 
> > > Looks good to me.
> > > 
> > > I'm wondering, shouldn't we also zero-initialize the dump data to 
> > > begin with? See the patch below (untested).
> > 
> > Note that this hides the bug from KASAN, though ;-)  And the bug
> > is not just infoleak - not all components are "all zeroes" in the
> > init state.
> 
> Yeah, but is zero-init really a problem though? Wouldn't it be 
> 'better' to have all zeroes if the dump doesn't fit? But I might be 
> missing something ...

"Doesn't fit" case is irrelevant for coredump - it gets a full-sized
buffer there.  And you do not copy the components in init state into
it.  Note that the set actually written will be different for different
threads; it's _not_ "everything in xcr0".

See the comments in front of fpstate_sanitize_xstate():

 * When executing XSAVEOPT (or other optimized XSAVE instructions), if
 * a processor implementation detects that an FPU state component is still
 * (or is again) in its initialized state, it may clear the corresponding
 * bit in the header.xfeatures field, and can skip the writeout of registers
 * to the corresponding memory layout.

The part about cleared bits in header.xfeatures, that is.  That, BTW,
is why you need xfeatures_mxcsr_quirk() thing - XFEATURES_FP in xcr0 is
always set (xsetbv would throw a GPF if you tried to clear it there),
but the same bit in the header might very well be clear.  For the
threads that have FPU registers in init state, which is _not_ all-zeroes.
If not for that, xfeatures_mxcsr_quirk() would always return true.

============================================
13.11         OPERATION OF XSAVES
The operation of XSAVES is similar to that of XSAVEC. The main differences
are (1) XSAVES can be executed only if CPL = 0; (2) XSAVES can operate on
the state components whose bits are set in XCR0 | IA32_XSS and can thus
operate on supervisor state components; and (3) XSAVES uses the modified
optimization (see Section 13.6). See Section 13.2 for details of how to
determine whether XSAVES is supported.

The XSAVES instruction takes a single memory operand, which is an XSAVE
area. In addition, the register pair EDX:EAX is an implicit operand used
as a state-component bitmap (see Section 13.1) called the instruction mask.
EDX:EAX & (XCR0 | IA32_XSS) (the logical AND the instruction mask with the
logical OR of XCR0 and IA32_XSS) is the requested-feature bitmap (RFBM)
of the state components to be saved.

The following conditions cause execution of the XSAVES instruction to
generate a fault:

*   If the XSAVE feature set is not enabled (CR4.OSXSAVE = 0), an
    invalid-opcode exception (#UD) occurs.
*   If CR0.TS[bit 3] is 1, a device-not-available exception (#NM) occurs.
*   If CPL > 0 or if the address of the XSAVE area is not 64-byte aligned,
    a general-protection exception (#GP) occurs.

If none of these conditions cause a fault, execution of XSAVES writes the
XSTATE_BV field of the XSAVE header (see Section 13.4.2), setting
XSTATE_BV[i] (0 <= i <= 63) as follows:
*   If RFBM[i] = 0, XSTATE_BV[i] is written as 0.
*   If RFBM[i] = 1, XSTATE_BV[i] is set to the value of XINUSE[i] (see below
    for an exception made for XSTATE_BV[1]). Section 13.6 defines XINUSE to
    describe the processor init optimization and specifies the initial
    configuration of each state component. The nature of that optimization
    implies the following:
    -- If state component i is in its initial configuration, XSTATE_BV[i]
       may be written with either 0 or 1.
    -- If state component i is not in its initial configuration, XSTATE_BV[i]
       is written with 1.
    XINUSE[1] pertains only to the state of the XMM registers and not to MXCSR.
    However, if RFBM[1] = 1 and MXCSR does not have the value 1F80H, XSAVES
    writes XSTATE_BV[1] as 1 even if XINUSE[1] = 0.
    (As explained in Section 13.6, the initial configurations of some state
    components may depend on whether the processor is in 64-bit mode.)
============================================

IOW, copy_xstate_to_kernel()/copy_xstate_to_user() needs not only to map
from compacted format to standard one; it also needs to compensate for
that "we might skip saving the components that are in init state; we'll
report which ones got skipped by way of ->header.xfeatures" thing.

Again, those leaked uninit chunks are *not* in the same places for all
threads.  Without any overflows, etc. involved.  And at least for
the set 0 (x87 registers) the init state is not all-zeroes, so blanket
memset() done first is not going to give the right results.

Re: [git pull] coredump infoleak fix

[Linus Torvalds]

On Thu, May 28, 2020 at 12:03 AM Ingo Molnar <mingo@kernel.org> wrote:
>
> I'm wondering, shouldn't we also zero-initialize the dump data to
> begin with? See the patch below (untested).

I actually got this patch from Andrew today independently due to a
KMSAN report. Which I'm applying.

             Linus

Re: [git pull] coredump infoleak fix

[Al Viro]

On Thu, May 28, 2020 at 11:34:38AM -0700, Linus Torvalds wrote:
> On Thu, May 28, 2020 at 12:03 AM Ingo Molnar <mingo@kernel.org> wrote:
> >
> > I'm wondering, shouldn't we also zero-initialize the dump data to
> > begin with? See the patch below (untested).
> 
> I actually got this patch from Andrew today independently due to a
> KMSAN report. Which I'm applying.

It doesn't fix all problems, though - you don't get an infoleak, but
you do get incorrect data...

Re: [git pull] coredump infoleak fix

[Linus Torvalds]

On Thu, May 28, 2020 at 12:06 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> It doesn't fix all problems, though - you don't get an infoleak, but
> you do get incorrect data...

Oh, I'm not saying it should replace any fix to regset->get(). I'm
just saying it is in addition to.

So if a regset has a reason to return less than the asked-for data, it
can do so and there's no leak.

               Linus

Re: [git pull] coredump infoleak fix

[Al Viro]

On Thu, May 28, 2020 at 12:09:48PM -0700, Linus Torvalds wrote:
> On Thu, May 28, 2020 at 12:06 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > It doesn't fix all problems, though - you don't get an infoleak, but
> > you do get incorrect data...
> 
> Oh, I'm not saying it should replace any fix to regset->get(). I'm
> just saying it is in addition to.
> 
> So if a regset has a reason to return less than the asked-for data, it
> can do so and there's no leak.

Might make sense to change the summary of that pull request to something
like
	make sure we don't forget to report the xstate components that happen
to be in init state - both for coredump and for PTRACE_GETREGSET

Re: [git pull] coredump infoleak fix

[Linus Torvalds]

On Thu, May 28, 2020 at 12:17 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Might make sense to change the summary of that pull request to something
> like
>         make sure we don't forget to report the xstate components that happen
> to be in init state - both for coredump and for PTRACE_GETREGSET

Note that this has nothing to do with x86 per se.

It's more about ->getregs() being a horrid interface, and being easy
to get wrong in general. The fact that xstate is complex is just one
such trigger.

              Linus

Re: [git pull] coredump infoleak fix

[Al Viro]

On Thu, May 28, 2020 at 12:19:32PM -0700, Linus Torvalds wrote:
> On Thu, May 28, 2020 at 12:17 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Might make sense to change the summary of that pull request to something
> > like
> >         make sure we don't forget to report the xstate components that happen
> > to be in init state - both for coredump and for PTRACE_GETREGSET
> 
> Note that this has nothing to do with x86 per se.
> 
> It's more about ->getregs() being a horrid interface, and being easy
> to get wrong in general. The fact that xstate is complex is just one
> such trigger.

The only one I've ran into so far, fortunately...  Almost all instances
write sequentially; the only exceptions are this one (buggy) and ia64
horrors with unwind.

I certainly agree that ->get() is an atrocity...

Re: [git pull] coredump infoleak fix

[Ingo Molnar]

* Al Viro <viro@zeniv.linux.org.uk> wrote:

> IOW, copy_xstate_to_kernel()/copy_xstate_to_user() needs not only to map
> from compacted format to standard one; it also needs to compensate for
> that "we might skip saving the components that are in init state; we'll
> report which ones got skipped by way of ->header.xfeatures" thing.
> 
> Again, those leaked uninit chunks are *not* in the same places for all
> threads.  Without any overflows, etc. involved.  And at least for
> the set 0 (x87 registers) the init state is not all-zeroes, so blanket
> memset() done first is not going to give the right results.

I'm not arguing against your fix (at all!) and I'll pull it in for 
v5.8 if Linus doesn't beat me to it.

I was arguing:

  >> shouldn't we also zero-initialize the dump data

with emphasis on the 'also'. :-)

To me the biggest practical fail here was the exposure of genuinely 
uninitialized data - not that the dumped information might not be a 
100% semantically correct FPU dump.

[ Unless I'm missing some non-coredump aspect of all this (such as the 
  wrong saving done in signal handling context for example?), which is 
  always a possibility. ]

Thanks,

	Ingo

Re: [git pull] coredump infoleak fix

[Ingo Molnar]

* Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Thu, May 28, 2020 at 11:34:38AM -0700, Linus Torvalds wrote:
> > On Thu, May 28, 2020 at 12:03 AM Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > > I'm wondering, shouldn't we also zero-initialize the dump data to
> > > begin with? See the patch below (untested).
> > 
> > I actually got this patch from Andrew today independently due to a
> > KMSAN report. Which I'm applying.
> 
> It doesn't fix all problems, though - you don't get an infoleak, but
> you do get incorrect data...

BTW., I pulled this into x86/urgent with a potential v5.7 merge, 
although it's getting tight.

I think the ptrace impact qualifies it for v5.7?

Thanks,

	Ingo

Re: [git pull] coredump infoleak fix

[pr-tracker-bot]

The pull request you sent on Wed, 27 May 2020 22:34:47 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git fixes

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/aa61b7bb00f7c0738823237a06161d568442b93c

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker