[PATCH] x86/fpu/xstate: Clear uninitialized xstate areas in core dump

[PATCH] x86/fpu/xstate: Clear uninitialized xstate areas in core dump

[Yu-cheng Yu]

In a core dump, copy_xstate_to_kernel() copies only enabled user xfeatures
to a kernel buffer without touching areas for disabled xfeatures.  However,
those uninitialized areas may contain random data, which is then written to
the core dump file and can be read by a non-privileged user.

Fix it by clearing uninitialized areas.

Link: https://github.com/google/kmsan/issues/76
Link: https://lore.kernel.org/lkml/20200419100848.63472-1-glider@google.com/
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reported-by: sam <sunhaoyl@outlook.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Jann Horn <jannh@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Alexander Potapenko <glider@google.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: <stable@vger.kernel.org>
---
 arch/x86/kernel/fpu/xstate.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 32b153d38748..0856daa29be7 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -983,6 +983,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 {
 	unsigned int offset, size;
 	struct xstate_header header;
+	int last_off;
 	int i;
 
 	/*
@@ -1006,7 +1007,17 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 
 	__copy_xstate_to_kernel(kbuf, &header, offset, size, size_total);
 
+	last_off = 0;
+
 	for (i = 0; i < XFEATURE_MAX; i++) {
+		/*
+		 * Clear uninitialized area before XSAVE header.
+		 */
+		if (i == FIRST_EXTENDED_XFEATURE) {
+			memset(kbuf + last_off, 0, XSAVE_HDR_OFFSET - last_off);
+			last_off = XSAVE_HDR_OFFSET + XSAVE_HDR_SIZE;
+		}
+
 		/*
 		 * Copy only in-use xstates:
 		 */
@@ -1020,11 +1031,16 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 			if (offset + size > size_total)
 				break;
 
+			memset(kbuf + last_off, 0, offset - last_off);
+			last_off = offset + size;
+
 			__copy_xstate_to_kernel(kbuf, src, offset, size, size_total);
 		}
 
 	}
 
+	memset(kbuf + last_off, 0, size_total - last_off);
+
 	if (xfeatures_mxcsr_quirk(header.xfeatures)) {
 		offset = offsetof(struct fxregs_state, mxcsr);
 		size = MXCSR_AND_FLAGS_SIZE;
-- 
2.21.0

Re: [PATCH] x86/fpu/xstate: Clear uninitialized xstate areas in core dump

[Dave Hansen]

On 5/7/20 9:49 AM, Yu-cheng Yu wrote:
> In a core dump, copy_xstate_to_kernel() copies only enabled user xfeatures
> to a kernel buffer without touching areas for disabled xfeatures.  However,
> those uninitialized areas may contain random data, which is then written to
> the core dump file and can be read by a non-privileged user.
> 
> Fix it by clearing uninitialized areas.

Do you have a Fixes: tag for this, or some background on where this
issue originated that might be helpful for backports?

Re: [PATCH] x86/fpu/xstate: Clear uninitialized xstate areas in core dump

[Yu-cheng Yu]

On Thu, 2020-05-07 at 09:52 -0700, Dave Hansen wrote:
> On 5/7/20 9:49 AM, Yu-cheng Yu wrote:
> > In a core dump, copy_xstate_to_kernel() copies only enabled user xfeatures
> > to a kernel buffer without touching areas for disabled xfeatures.  However,
> > those uninitialized areas may contain random data, which is then written to
> > the core dump file and can be read by a non-privileged user.
> > 
> > Fix it by clearing uninitialized areas.
> 
> Do you have a Fixes: tag for this, or some background on where this
> issue originated that might be helpful for backports?

I will add that.

Re: [PATCH] x86/fpu/xstate: Clear uninitialized xstate areas in core dump

[Sebastian Andrzej Siewior]

On 2020-05-07 09:49:04 [-0700], Yu-cheng Yu wrote:
> In a core dump, copy_xstate_to_kernel() copies only enabled user xfeatures
> to a kernel buffer without touching areas for disabled xfeatures.  However,
> those uninitialized areas may contain random data, which is then written to
> the core dump file and can be read by a non-privileged user.
> 
> Fix it by clearing uninitialized areas.

Is the problem that copy_xstate_to_kernel() gets `kbuf' passed which
isn't zeroed? If so, would it work clean that upfront?

Sebastian

Re: [PATCH] x86/fpu/xstate: Clear uninitialized xstate areas in core dump

[Yu-cheng Yu]

On Thu, 2020-05-07 at 18:56 +0200, Sebastian Andrzej Siewior wrote:
> On 2020-05-07 09:49:04 [-0700], Yu-cheng Yu wrote:
> > In a core dump, copy_xstate_to_kernel() copies only enabled user xfeatures
> > to a kernel buffer without touching areas for disabled xfeatures.  However,
> > those uninitialized areas may contain random data, which is then written to
> > the core dump file and can be read by a non-privileged user.
> > 
> > Fix it by clearing uninitialized areas.
> 
> Is the problem that copy_xstate_to_kernel() gets `kbuf' passed which
> isn't zeroed? If so, would it work clean that upfront?

Alexander Potapenko's patch (in the Link:) fixes the buffer in
fill_thread_core_info().  My patch prevents the same issue if this function is
called from somewhere else in the future.

Yu-cheng

Re: [PATCH] x86/fpu/xstate: Clear uninitialized xstate areas in core dump

[Thomas Gleixner]

Yu-cheng Yu <yu-cheng.yu@intel.com> writes:
> @@ -983,6 +983,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
>  {
>  	unsigned int offset, size;
>  	struct xstate_header header;
> +	int last_off;
>  	int i;
>  
>  	/*
> @@ -1006,7 +1007,17 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
>  
>  	__copy_xstate_to_kernel(kbuf, &header, offset, size, size_total);
>  
> +	last_off = 0;
> +
>  	for (i = 0; i < XFEATURE_MAX; i++) {
> +		/*
> +		 * Clear uninitialized area before XSAVE header.
> +		 */
> +		if (i == FIRST_EXTENDED_XFEATURE) {
> +			memset(kbuf + last_off, 0, XSAVE_HDR_OFFSET - last_off);
> +			last_off = XSAVE_HDR_OFFSET + XSAVE_HDR_SIZE;
> +		}
> +
>  		/*
>  		 * Copy only in-use xstates:
>  		 */
> @@ -1020,11 +1031,16 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
>  			if (offset + size > size_total)
>  				break;
>  
> +			memset(kbuf + last_off, 0, offset - last_off);
> +			last_off = offset + size;
> +
>  			__copy_xstate_to_kernel(kbuf, src, offset, size, size_total);
>  		}
>  
>  	}
>  
> +	memset(kbuf + last_off, 0, size_total - last_off);

Why doing all this partial zeroing? There is absolutely no point.

Either the caller clears the buffer or this function clears it right at
the beginning with:

    memset(kbuf, 0, min(size_total, XSAVE_MAX_SIZE));

Thanks,

        tglx

Re: [PATCH] x86/fpu/xstate: Clear uninitialized xstate areas in core dump

[Yu-cheng Yu]

On Thu, 2020-05-07 at 20:22 +0200, Thomas Gleixner wrote:
> Yu-cheng Yu <yu-cheng.yu@intel.com> writes:
> > @@ -983,6 +983,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
> >  {
> >  	unsigned int offset, size;
> >  	struct xstate_header header;
> > +	int last_off;
> >  	int i;
> >  
> >  	/*
> > @@ -1006,7 +1007,17 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
> >  
> >  	__copy_xstate_to_kernel(kbuf, &header, offset, size, size_total);
> >  
> > +	last_off = 0;
> > +
> >  	for (i = 0; i < XFEATURE_MAX; i++) {
> > +		/*
> > +		 * Clear uninitialized area before XSAVE header.
> > +		 */
> > +		if (i == FIRST_EXTENDED_XFEATURE) {
> > +			memset(kbuf + last_off, 0, XSAVE_HDR_OFFSET - last_off);
> > +			last_off = XSAVE_HDR_OFFSET + XSAVE_HDR_SIZE;
> > +		}
> > +
> >  		/*
> >  		 * Copy only in-use xstates:
> >  		 */
> > @@ -1020,11 +1031,16 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
> >  			if (offset + size > size_total)
> >  				break;
> >  
> > +			memset(kbuf + last_off, 0, offset - last_off);
> > +			last_off = offset + size;
> > +
> >  			__copy_xstate_to_kernel(kbuf, src, offset, size, size_total);
> >  		}
> >  
> >  	}
> >  
> > +	memset(kbuf + last_off, 0, size_total - last_off);
> 
> Why doing all this partial zeroing? There is absolutely no point.
> 
> Either the caller clears the buffer or this function clears it right at
> the beginning with:
> 
>     memset(kbuf, 0, min(size_total, XSAVE_MAX_SIZE));

I was concerned that the XSAVES buffer can be large, but this is not in a
performance-critical path.  Yes, clear it in the beginning is simpler.

Yu-cheng