KASAN fix for arch_dup_task_struct (x86, um)
Subsystems (3)
cover KASAN fix for arch_dup_task_struct (x86, um)
From: Benjamin Berg <[email protected]>
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 <[email protected]>.
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(-)
Signature · 1 line
2.47.1
PATCH 1/3 vmlinux.lds.h: remove entry to place init_task onto init_stack
From: Benjamin Berg <[email protected]>
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.
--- include/asm-generic/vmlinux.lds.h | 1 - 1 file changed, 1 deletion(-)
include/asm-generic/vmlinux.lds.h +0 -1
@@ -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 = .;
Signature · 1 line
2.47.1
[tip: x86/fpu] vmlinux.lds.h: Remove entry to place init_task onto init_stack
The following commit has been merged into the x86/fpu branch of tip:
The following commit has been merged into the x86/fpu branch of tip:
Commit-ID: 2ec01bd7152f80795eab7b48706aa5db8d4a286a Gitweb: https://git.kernel.org/tip/2ec01bd7152f80795eab7b48706aa5db8d4a286a Author: Benjamin Berg <[email protected]> AuthorDate: Tue, 17 Dec 2024 21:27:43 +01:00 Committer: Ingo Molnar <[email protected]> CommitterDate: Wed, 26 Feb 2025 14:02:21 +01:00
vmlinux.lds.h: Remove entry to place init_task onto init_stack
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.
--- include/asm-generic/vmlinux.lds.h | 1 - 1 file changed, 1 deletion(-)
include/asm-generic/vmlinux.lds.h +0 -1
@@ -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 = .;
PATCH 2/3 um: avoid copying FP state from init_task
In reply to Benjamin BergFrom: Benjamin Berg <[email protected]>
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.
--- arch/um/kernel/process.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
arch/um/kernel/process.c +9 -1
@@ -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 ; }
Signature · 1 line
2.47.1
Re: [PATCH 2/3] um: avoid copying FP state from init_task
Quoted 7 lines · Benjamin Berg
From: Benjamin Berg <[email protected]>
The
init_taskinstance of structtask_structis statically allocated and does not contain the dynamic area for the userspace FP registers. As such, limit the copy to the valid area ofinit_taskand fill the rest with zero.
Quoted 6 lines
Note that the FP state is only needed for userspace, and as such it is entirely reasonable for
init_taskto not contain it.Reported-by Brian Norris <[email protected]>Fixes 3f17fed21491 ("um: switch to regset API and depend on XSTATE")
No stable backport? The broken commit is now in the 6.13 release.
Quoted 1 line
Signed-off-by Benjamin Berg <[email protected]>
Quoted 11 lines
--- arch/um/kernel/process.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)arch/um/kernel/process.c +0
-0@@ -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)
arch/um/kernel/process.c void initial_thread_cb(void (*proc)(void *), void *arg) · line 201
Quoted 10 lines
arch/um/kernel/process.c +8
-1{ -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.
Quoted 7 lines
arch/um/kernel/process.c +1
-0+return 0 ; }Signature · 1 line
2.47.1
PATCH 3/3 x86: avoid copying dynamic FP state from init_task
In reply to Benjamin BergFrom: Benjamin Berg <[email protected]>
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.
--- arch/x86/kernel/process.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
arch/x86/kernel/process.c +9 -1
@@ -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
Signature · 1 line
2.47.1
Re: [PATCH 3/3] x86: avoid copying dynamic FP state from init_task
Quoted 21 lines · Benjamin Berg
From: Benjamin Berg <[email protected]>
The
init_taskinstance of structtask_structis statically allocated and may not contain the full FP state for userspace. As such, limit the copy to the valid area ofinit_taskand fill the rest with zero.Note that the FP state is only needed for userspace, and as such it is entirely reasonable for
init_taskto not contain parts of it.Signed-off-by Benjamin Berg <[email protected]>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(-)arch/x86/kernel/process.c +0
-0@@ -92,7 +92,15 @@ EXPORT_PER_CPU_SYMBOL_GPL(__tss_limit_invalid);*/
arch/x86/kernel/process.c EXPORT_PER_CPU_SYMBOL_GPL(__tss_limit_invalid); · line 101
Quoted 10 lines
arch/x86/kernel/process.c +7
-1int 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);
Note that this patch, while it still applies cleanly, crashes/hangs the x86-64 defconfig kernel bootup in the early boot phase in a KVM guest bootup.
Thanks,
Ingo
Re: [PATCH 3/3] x86: avoid copying dynamic FP state from init_task
Quoted 30 lines · Ingo Molnar
Quoted 27 lines · Benjamin Berg
From: Benjamin Berg <[email protected]>
The
init_taskinstance of structtask_structis statically allocated and may not contain the full FP state for userspace. As such, limit the copy to the valid area ofinit_taskand fill the rest with zero.Note that the FP state is only needed for userspace, and as such it is entirely reasonable for
init_taskto not contain parts of it.Signed-off-by Benjamin Berg <[email protected]>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(-)arch/x86/kernel/process.c +3
-1@@ -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));
Quoted 8 lines
arch/x86/kernel/process.c EXPORT_PER_CPU_SYMBOL_GPL(__tss_limit_invalid); · line 101
Quoted 4 lines
arch/x86/kernel/process.c +4
-0+memset ((void *)dst +sizeof (init_task),0 , + arch_task_struct_size - sizeof(init_task)); + }else { +memcpy (dst, src, arch_task_struct_size);Note that this patch, while it still applies cleanly, crashes/hangs the x86-64 defconfig kernel bootup in the early boot phase in a KVM guest bootup.
Oh, outch. It seems that arch_task_struct_size can actually become smaller than sizeof(init_task) if the CPU does not have certain features.
See fpu__init_task_struct_size, which does:
int task_size = sizeof(struct task_struct); task_size -= sizeof(current->thread.fpu.__fpstate.regs); task_size += fpu_kernel_cfg.default_size;
I'll submit a new version of the patch and then also switch to use memcpy_and_pad.
Benjamin
Re: [PATCH 3/3] x86: avoid copying dynamic FP state from init_task
Quoted 8 lines · Benjamin Berg
Quoted 3 lines
Note that this patch, while it still applies cleanly, crashes/hangs the x86-64 defconfig kernel bootup in the early boot phase in a KVM guest bootup.
Oh, outch. It seems that
arch_task_struct_sizecan actually become smaller thansizeof(init_task)if the CPU does not have certain features.
Quoted 8 lines
See
fpu__init_task_struct_size, which does:int task_size = sizeof(struct task_struct); task_size -= sizeof(current->thread.fpu.__fpstate.regs); task_size += fpu_kernel_cfg.default_size;I'll submit a new version of the patch and then also switch to use
memcpy_and_pad.
Thank you!
Ingo