* [tip: x86/fpu] x86/fpu: Restore fpu_thread_struct_whitelist() to fix CONFIG_HARDENED_USERCOPY=y crash
[not found] <202505041418.F47130C4C8@keescook # Discussion #2>
@ 2025-05-05 11:43 ` tip-bot2 for Kees Cook
0 siblings, 0 replies; only message in thread
From: tip-bot2 for Kees Cook @ 2025-05-05 11:43 UTC (permalink / raw)
To: linux-tip-commits
Cc: Borislav Petkov (AMD),
Kees Cook, Ingo Molnar, Chang S. Bae, Gustavo A. R. Silva,
H. Peter Anvin, Andy Lutomirski, Brian Gerst, Linus Torvalds,
linux-hardening, x86, linux-kernel
The following commit has been merged into the x86/fpu branch of tip:
Commit-ID: 960bc2bcba5987a82530b9756e1f602a894cffa4
Gitweb: https://git.kernel.org/tip/960bc2bcba5987a82530b9756e1f602a894cffa4
Author: Kees Cook <kees@kernel.org>
AuthorDate: Sun, 04 May 2025 15:30:38 -07:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 05 May 2025 13:24:32 +02:00
x86/fpu: Restore fpu_thread_struct_whitelist() to fix CONFIG_HARDENED_USERCOPY=y crash
Borislav Petkov reported the following boot crash on x86-32,
with CONFIG_HARDENED_USERCOPY=y:
| usercopy: Kernel memory overwrite attempt detected to SLUB object 'task_struct' (offset 2112, size 160)!
| ...
| kernel BUG at mm/usercopy.c:102!
So the useroffset and usersize arguments are what control the allowed
window of copying in/out of the "task_struct" kmem cache:
/* create a slab on which task_structs can be allocated */
task_struct_whitelist(&useroffset, &usersize);
task_struct_cachep = kmem_cache_create_usercopy("task_struct",
arch_task_struct_size, align,
SLAB_PANIC|SLAB_ACCOUNT,
useroffset, usersize, NULL);
task_struct_whitelist() positions this window based on the location of
the thread_struct within task_struct, and gets the arch-specific details
via arch_thread_struct_whitelist(offset, size):
static void __init task_struct_whitelist(unsigned long *offset, unsigned long *size)
{
/* Fetch thread_struct whitelist for the architecture. */
arch_thread_struct_whitelist(offset, size);
/*
* Handle zero-sized whitelist or empty thread_struct, otherwise
* adjust offset to position of thread_struct in task_struct.
*/
if (unlikely(*size == 0))
*offset = 0;
else
*offset += offsetof(struct task_struct, thread);
}
Commit cb7ca40a3882 ("x86/fpu: Make task_struct::thread constant size")
removed the logic for the window, leaving:
static inline void
arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
{
*offset = 0;
*size = 0;
}
So now there is no window that usercopy hardening will allow to be copied
in/out of task_struct.
But as reported above, there *is* a copy in copy_uabi_to_xstate(). (It
seems there are several, actually.)
int copy_sigframe_from_user_to_xstate(struct task_struct *tsk,
const void __user *ubuf)
{
return copy_uabi_to_xstate(x86_task_fpu(tsk)->fpstate, NULL, ubuf, &tsk->thread.pkru);
}
This appears to be writing into x86_task_fpu(tsk)->fpstate. With or
without CONFIG_X86_DEBUG_FPU, this resolves to:
((struct fpu *)((void *)(task) + sizeof(*(task))))
i.e. the memory "after task_struct" is cast to "struct fpu", and the
uses the "fpstate" pointer. How that pointer gets set looks to be
variable, but I think the one we care about here is:
fpu->fpstate = &fpu->__fpstate;
And struct fpu::__fpstate says:
struct fpstate __fpstate;
/*
* WARNING: '__fpstate' is dynamically-sized. Do not put
* anything after it here.
*/
So we're still dealing with a dynamically sized thing, even if it's not
within the literal struct task_struct -- it's still in the kmem cache,
though.
Looking at the kmem cache size, it has allocated "arch_task_struct_size"
bytes, which is calculated in fpu__init_task_struct_size():
int task_size = sizeof(struct task_struct);
task_size += sizeof(struct fpu);
/*
* Subtract off the static size of the register state.
* It potentially has a bunch of padding.
*/
task_size -= sizeof(union fpregs_state);
/*
* Add back the dynamically-calculated register state
* size.
*/
task_size += fpu_kernel_cfg.default_size;
/*
* We dynamically size 'struct fpu', so we require that
* 'state' be at the end of 'it:
*/
CHECK_MEMBER_AT_END_OF(struct fpu, __fpstate);
arch_task_struct_size = task_size;
So, this is still copying out of the kmem cache for task_struct, and the
window seems unchanged (still fpu regs). This is what the window was
before:
void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
{
*offset = offsetof(struct thread_struct, fpu.__fpstate.regs);
*size = fpu_kernel_cfg.default_size;
}
And the same commit I mentioned above removed it.
I think the misunderstanding is here:
| The fpu_thread_struct_whitelist() quirk to hardened usercopy can be removed,
| now that the FPU structure is not embedded in the task struct anymore, which
| reduces text footprint a bit.
Yes, FPU is no longer in task_struct, but it IS in the kmem cache named
"task_struct", since the fpstate is still being allocated there.
Partially revert the earlier mentioned commit, along with a
recalculation of the fpstate regs location.
Fixes: cb7ca40a3882 ("x86/fpu: Make task_struct::thread constant size")
Reported-by: Borislav Petkov (AMD) <bp@alien8.de>
Tested-by: Borislav Petkov (AMD) <bp@alien8.de>
Signed-off-by: Kees Cook <kees@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-hardening@vger.kernel.org
Link: https://lore.kernel.org/all/20250409211127.3544993-1-mingo@kernel.org/ # Discussion #1
Link: https://lore.kernel.org/r/202505041418.F47130C4C8@keescook # Discussion #2
---
arch/x86/include/asm/processor.h | 12 +++++-------
arch/x86/kernel/fpu/core.c | 14 ++++++++++++++
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index eaa7214..ad33903 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -522,14 +522,12 @@ extern struct fpu *x86_task_fpu(struct task_struct *task);
# define x86_task_fpu(task) ((struct fpu *)((void *)(task) + sizeof(*(task))))
#endif
-/*
- * X86 doesn't need any embedded-FPU-struct quirks:
- */
-static inline void
-arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
+extern void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size);
+
+static inline void arch_thread_struct_whitelist(unsigned long *offset,
+ unsigned long *size)
{
- *offset = 0;
- *size = 0;
+ fpu_thread_struct_whitelist(offset, size);
}
static inline void
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index fa13129..105b1b8 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -681,6 +681,20 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal,
}
/*
+ * While struct fpu is no longer part of struct thread_struct, it is still
+ * allocated after struct task_struct in the "task_struct" kmem cache. But
+ * since FPU is expected to be part of struct thread_struct, we have to
+ * adjust for it here.
+ */
+void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
+{
+ /* The allocation follows struct task_struct. */
+ *offset = sizeof(struct task_struct) - offsetof(struct task_struct, thread);
+ *offset += offsetof(struct fpu, __fpstate.regs);
+ *size = fpu_kernel_cfg.default_size;
+}
+
+/*
* Drops current FPU state: deactivates the fpregs and
* the fpstate. NOTE: it still leaves previous contents
* in the fpregs in the eager-FPU case.
^ permalink raw reply [flat|nested] only message in thread