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

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

[Benjamin Berg]

From: Benjamin Berg <benjamin.berg@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 <briannorris@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

[PATCH 1/3] vmlinux.lds.h: remove entry to place init_task onto init_stack

[Benjamin Berg]

From: Benjamin Berg <benjamin.berg@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 <benjamin.berg@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

[PATCH 2/3] um: avoid copying FP state from init_task

[Benjamin Berg]

From: Benjamin Berg <benjamin.berg@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 <briannorris@chromium.org>
Closes: https://lore.kernel.org/Z1ySXmjZm-xOqk90@google.com
Fixes: 3f17fed21491 ("um: switch to regset API and depend on XSTATE")
Signed-off-by: Benjamin Berg <benjamin.berg@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

[PATCH 3/3] x86: avoid copying dynamic FP state from init_task

[Benjamin Berg]

From: Benjamin Berg <benjamin.berg@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 <benjamin.berg@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

Re: [PATCH 2/3] um: avoid copying FP state from init_task

[Thomas Weißschuh]

On Tue, Dec 17, 2024 at 09:27:44PM +0100, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.berg@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 <briannorris@chromium.org>
> Closes: https://lore.kernel.org/Z1ySXmjZm-xOqk90@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 <benjamin.berg@intel.com>

Tested-by: Thomas Weißschuh <thomas.weissschuh@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.

> +
>  	return 0;
>  }
>  
> -- 
> 2.47.1
> 

Re: [PATCH 3/3] x86: avoid copying dynamic FP state from init_task

[Ingo Molnar]

* Benjamin Berg <benjamin@sipsolutions.net> wrote:

> From: Benjamin Berg <benjamin.berg@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 <benjamin.berg@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);

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

[Benjamin Berg]

On Wed, 2025-02-26 at 14:08 +0100, Ingo Molnar wrote:
> 
> * Benjamin Berg <benjamin@sipsolutions.net> wrote:
> 
> > From: Benjamin Berg <benjamin.berg@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 <benjamin.berg@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);
> 
> 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

[Ingo Molnar]

* Benjamin Berg <benjamin@sipsolutions.net> wrote:

> > 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.

Thank you!

	Ingo