Groups keyboard shortcuts have been updated
Dismiss
See shortcuts

[PATCH 0/3] KASAN fix for arch_dup_task_struct (x86, um)

1 view
Skip to first unread message

Benjamin Berg

unread,
Dec 17, 2024, 3:30:06 PM12/17/24
to linux...@vger.kernel.org, linu...@lists.infradead.org, x...@kernel.org, brian...@chromium.org, linux-...@vger.kernel.org, kasa...@googlegroups.com, Benjamin Berg
From: Benjamin Berg <benjam...@intel.com>

On the x86 and um architectures struct task_struct is dynamically
sized depending on the size required to store the floating point
registers. After adding this feature to UML it sometimes triggered
KASAN errors as the memcpy in arch_dup_task_struct read past
init_task.

In my own testing, the reported KASAN error was for a read into the
redzone of the next global variable (init_sighand). Due to padding,
the reported area was already far past the size of init_task.

Note that on x86 the dynamically allocated area of struct task_struct
is quite a bit smaller and may not even exist. This might explain why
this error has not been noticed before.

This problem was reported by Brian Norris <brian...@chromium.org>.

Benjamin

Benjamin Berg (3):
vmlinux.lds.h: remove entry to place init_task onto init_stack
um: avoid copying FP state from init_task
x86: avoid copying dynamic FP state from init_task

arch/um/kernel/process.c | 10 +++++++++-
arch/x86/kernel/process.c | 10 +++++++++-
include/asm-generic/vmlinux.lds.h | 1 -
3 files changed, 18 insertions(+), 3 deletions(-)

--
2.47.1

Benjamin Berg

unread,
Dec 17, 2024, 3:30:07 PM12/17/24
to linux...@vger.kernel.org, linu...@lists.infradead.org, x...@kernel.org, brian...@chromium.org, linux-...@vger.kernel.org, kasa...@googlegroups.com, Benjamin Berg
From: Benjamin Berg <benjam...@intel.com>

Since commit 0eb5085c3874 ("arch: remove ARCH_TASK_STRUCT_ON_STACK")
there is no option that would allow placing task_struct on the stack.
Remove the unused linker script entry.

Signed-off-by: Benjamin Berg <benjam...@intel.com>
---
include/asm-generic/vmlinux.lds.h | 1 -
1 file changed, 1 deletion(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 54504013c749..8cd631a95084 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -404,7 +404,6 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
__start_init_stack = .; \
init_thread_union = .; \
init_stack = .; \
- KEEP(*(.data..init_task)) \
KEEP(*(.data..init_thread_info)) \
. = __start_init_stack + THREAD_SIZE; \
__end_init_stack = .;
--
2.47.1

Benjamin Berg

unread,
Dec 17, 2024, 3:30:09 PM12/17/24
to linux...@vger.kernel.org, linu...@lists.infradead.org, x...@kernel.org, brian...@chromium.org, linux-...@vger.kernel.org, kasa...@googlegroups.com, Benjamin Berg
From: Benjamin Berg <benjam...@intel.com>

The init_task instance of struct task_struct is statically allocated and
does not contain the dynamic area for the userspace FP registers. As
such, limit the copy to the valid area of init_task and fill the rest
with zero.

Note that the FP state is only needed for userspace, and as such it is
entirely reasonable for init_task to not contain it.

Reported-by: Brian Norris <brian...@chromium.org>
Closes: https://lore.kernel.org/Z1ySXmjZ...@google.com
Fixes: 3f17fed21491 ("um: switch to regset API and depend on XSTATE")
Signed-off-by: Benjamin Berg <benjam...@intel.com>
---
arch/um/kernel/process.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
index 30bdc0a87dc8..3a67ba8aa62d 100644
--- a/arch/um/kernel/process.c
+++ b/arch/um/kernel/process.c
@@ -191,7 +191,15 @@ void initial_thread_cb(void (*proc)(void *), void *arg)
int arch_dup_task_struct(struct task_struct *dst,
struct task_struct *src)
{
- memcpy(dst, src, arch_task_struct_size);
+ /* init_task is not dynamically sized (missing FPU state) */
+ if (unlikely(src == &init_task)) {
+ memcpy(dst, src, sizeof(init_task));
+ memset((void *)dst + sizeof(init_task), 0,
+ arch_task_struct_size - sizeof(init_task));
+ } else {
+ memcpy(dst, src, arch_task_struct_size);
+ }
+
return 0;
}

--
2.47.1

Benjamin Berg

unread,
Dec 17, 2024, 3:30:11 PM12/17/24
to linux...@vger.kernel.org, linu...@lists.infradead.org, x...@kernel.org, brian...@chromium.org, linux-...@vger.kernel.org, kasa...@googlegroups.com, Benjamin Berg
From: Benjamin Berg <benjam...@intel.com>

The init_task instance of struct task_struct is statically allocated and
may not contain the full FP state for userspace. As such, limit the copy
to the valid area of init_task and fill the rest with zero.

Note that the FP state is only needed for userspace, and as such it is
entirely reasonable for init_task to not contain parts of it.

Signed-off-by: Benjamin Berg <benjam...@intel.com>
Fixes: 5aaeb5c01c5b ("x86/fpu, sched: Introduce CONFIG_ARCH_WANTS_DYNAMIC_TASK_STRUCT and use it on x86")
---
arch/x86/kernel/process.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index f63f8fd00a91..1be45fe70cad 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -92,7 +92,15 @@ EXPORT_PER_CPU_SYMBOL_GPL(__tss_limit_invalid);
*/
int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
{
- memcpy(dst, src, arch_task_struct_size);
+ /* init_task is not dynamically sized (incomplete FPU state) */
+ if (unlikely(src == &init_task)) {
+ memcpy(dst, src, sizeof(init_task));
+ memset((void *)dst + sizeof(init_task), 0,
+ arch_task_struct_size - sizeof(init_task));
+ } else {
+ memcpy(dst, src, arch_task_struct_size);
+ }
+
#ifdef CONFIG_VM86
dst->thread.vm86 = NULL;
#endif
--
2.47.1

Thomas Weißschuh

unread,
Jan 20, 2025, 8:36:16 AMJan 20
to Benjamin Berg, linux...@vger.kernel.org, linu...@lists.infradead.org, x...@kernel.org, brian...@chromium.org, linux-...@vger.kernel.org, kasa...@googlegroups.com, Benjamin Berg
On Tue, Dec 17, 2024 at 09:27:44PM +0100, Benjamin Berg wrote:
> From: Benjamin Berg <benjam...@intel.com>
>
> The init_task instance of struct task_struct is statically allocated and
> does not contain the dynamic area for the userspace FP registers. As
> such, limit the copy to the valid area of init_task and fill the rest
> with zero.
>
> Note that the FP state is only needed for userspace, and as such it is
> entirely reasonable for init_task to not contain it.
>
> Reported-by: Brian Norris <brian...@chromium.org>
> Closes: https://lore.kernel.org/Z1ySXmjZ...@google.com
> Fixes: 3f17fed21491 ("um: switch to regset API and depend on XSTATE")

No stable backport? The broken commit is now in the 6.13 release.

> Signed-off-by: Benjamin Berg <benjam...@intel.com>

Tested-by: Thomas Weißschuh <thomas.w...@linutronix.de>

> ---
> arch/um/kernel/process.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
> index 30bdc0a87dc8..3a67ba8aa62d 100644
> --- a/arch/um/kernel/process.c
> +++ b/arch/um/kernel/process.c
> @@ -191,7 +191,15 @@ void initial_thread_cb(void (*proc)(void *), void *arg)
> int arch_dup_task_struct(struct task_struct *dst,
> struct task_struct *src)
> {
> - memcpy(dst, src, arch_task_struct_size);
> + /* init_task is not dynamically sized (missing FPU state) */
> + if (unlikely(src == &init_task)) {
> + memcpy(dst, src, sizeof(init_task));
> + memset((void *)dst + sizeof(init_task), 0,
> + arch_task_struct_size - sizeof(init_task));
> + } else {
> + memcpy(dst, src, arch_task_struct_size);
> + }

Nitpick:
This could make use of memcpy_and_pad() in various forms.
Reply all
Reply to author
Forward
0 new messages