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;
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?
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.
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
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
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
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
linux-stable-mirror@lists.linaro.org