* [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3)
[not found] <20210904060908.1310204-1-keithp@keithp.com>
@ 2021-09-07 22:00 ` Keith Packard
2021-09-07 22:00 ` [PATCH 1/7] ARM: Pass cpu number to secondary_start_kernel Keith Packard
` (7 more replies)
0 siblings, 8 replies; 14+ messages in thread
From: Keith Packard @ 2021-09-07 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Keith Packard, Krzysztof Kozlowski,
Linus Walleij, linux-arm-kernel, linux-mm, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
Placing thread_info in the kernel stack leaves it vulnerable to stack
overflow attacks. This short series addresses that by using the
existing THREAD_INFO_IN_TASK infrastructure.
This is the third version of this series, in this version the changes
are restricted to hardware which provides the TPIDRPRW register. This
register is repurposed from holding the per_cpu_offset value to
holding the 'current' value as that allows fetching this value
atomically so that it can be used in a preemptable context.
The series is broken into seven pieces:
1) Change the secondary_start_kernel API to receive the cpu
number. This avoids needing to be able to find this value independently in
future patches.
2) Change the secondary_start_kernel API to also receive the 'task'
value. Passing the value to this function also avoids needing to
be able to discover it independently.
3) A cleanup which avoids assuming that THREAD_INFO_IN_TASK is not set.
4) A hack, borrowed from the powerpc arch, which allows locating the 'cpu'
field in either thread_info or task_struct, without requiring linux/sched.h
to be included in asm/smp.h
5) Disable the optimization storing per_cpu_offset in TPIDRPRW. This leaves
the register free to hold 'current' instead.
6) Use TPIDRPRW for 'current'. This is enabled for either CPU_V6K or CPU_V7,
but not if CPU_V6 is also enabled.
7) Enable THREAD_INFO_IN_TASK whenever TPIDRPRW is used to hold 'current'.
Signed-off-by: Keith Packard <keithpac@amazon.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/7] ARM: Pass cpu number to secondary_start_kernel
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
@ 2021-09-07 22:00 ` Keith Packard
2021-09-07 22:00 ` [PATCH 2/7] ARM: Pass task " Keith Packard
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Keith Packard @ 2021-09-07 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Keith Packard, Krzysztof Kozlowski,
Linus Walleij, linux-arm-kernel, linux-mm, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
Instead of pulling the CPU value out of the thread_info struct, pass
it as an argument. When first initializing secondary processors, this
is done by stashing the value in the secondary_data struct. When
restarting idle processors, the previous CPU value is passed.
Because the cpu is now known at the top of secondary_start_kernel, the
per_cpu_offset value can now be set at this point, instead of in
cpu_init where it was also incorrectly setting the per_cpu_offset for
the boot processor before that value had been computed.
Signed-off-by: Keith Packard <keithpac@amazon.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/arm/include/asm/smp.h | 3 ++-
arch/arm/kernel/head-nommu.S | 1 +
arch/arm/kernel/head.S | 1 +
arch/arm/kernel/setup.c | 6 ------
arch/arm/kernel/smp.c | 14 +++++++++-----
5 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 5d508f5d56c4..86a7fd721556 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -48,7 +48,7 @@ extern void set_smp_ipi_range(int ipi_base, int nr_ipi);
* Called from platform specific assembly code, this is the
* secondary CPU entry point.
*/
-asmlinkage void secondary_start_kernel(void);
+asmlinkage void secondary_start_kernel(unsigned int cpu);
/*
@@ -61,6 +61,7 @@ struct secondary_data {
};
unsigned long swapper_pg_dir;
void *stack;
+ unsigned int cpu;
};
extern struct secondary_data secondary_data;
extern void secondary_startup(void);
diff --git a/arch/arm/kernel/head-nommu.S b/arch/arm/kernel/head-nommu.S
index 0fc814bbc34b..5aa8ef42717f 100644
--- a/arch/arm/kernel/head-nommu.S
+++ b/arch/arm/kernel/head-nommu.S
@@ -114,6 +114,7 @@ ENTRY(secondary_startup)
add r12, r12, r10
ret r12
1: bl __after_proc_init
+ ldr r0, [r7, #16] @ set up cpu number
ldr sp, [r7, #12] @ set up the stack pointer
mov fp, #0
b secondary_start_kernel
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 7f62c5eccdf3..0e541af738e2 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -394,6 +394,7 @@ ENDPROC(secondary_startup_arm)
ENTRY(__secondary_switched)
ldr_l r7, secondary_data + 12 @ get secondary_data.stack
+ ldr_l r0, secondary_data + 16 @ get secondary_data.cpu
mov sp, r7
mov fp, #0
b secondary_start_kernel
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 73ca7797b92f..ca0201635fac 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -532,12 +532,6 @@ void notrace cpu_init(void)
BUG();
}
- /*
- * This only works on resume and secondary cores. For booting on the
- * boot cpu, smp_prepare_boot_cpu is called after percpu area setup.
- */
- set_my_cpu_offset(per_cpu_offset(cpu));
-
cpu_proc_init();
/*
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 74679240a9d8..55cb1689a4b3 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -153,6 +153,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
secondary_data.pgdir = virt_to_phys(idmap_pgd);
secondary_data.swapper_pg_dir = get_arch_pgd(swapper_pg_dir);
#endif
+ secondary_data.cpu = cpu;
sync_cache_w(&secondary_data);
/*
@@ -373,11 +374,14 @@ void arch_cpu_idle_dead(void)
* cpu initialisation. There's some initialisation which needs
* to be repeated to undo the effects of taking the CPU offline.
*/
- __asm__("mov sp, %0\n"
+ __asm__("mov r0, %1\n"
+ " mov sp, %0\n"
" mov fp, #0\n"
" b secondary_start_kernel"
:
- : "r" (task_stack_page(current) + THREAD_SIZE - 8));
+ : "r" (task_stack_page(current) + THREAD_SIZE - 8),
+ "r" (cpu)
+ : "r0");
}
#endif /* CONFIG_HOTPLUG_CPU */
@@ -400,10 +404,11 @@ static void smp_store_cpu_info(unsigned int cpuid)
* This is the secondary CPU boot entry. We're using this CPUs
* idle thread stack, but a set of temporary page tables.
*/
-asmlinkage void secondary_start_kernel(void)
+asmlinkage void secondary_start_kernel(unsigned int cpu)
{
struct mm_struct *mm = &init_mm;
- unsigned int cpu;
+
+ set_my_cpu_offset(per_cpu_offset(cpu));
secondary_biglittle_init();
@@ -420,7 +425,6 @@ asmlinkage void secondary_start_kernel(void)
* All kernel threads share the same mm context; grab a
* reference and switch to it.
*/
- cpu = smp_processor_id();
mmgrab(mm);
current->active_mm = mm;
cpumask_set_cpu(cpu, mm_cpumask(mm));
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/7] ARM: Pass task to secondary_start_kernel
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
2021-09-07 22:00 ` [PATCH 1/7] ARM: Pass cpu number to secondary_start_kernel Keith Packard
@ 2021-09-07 22:00 ` Keith Packard
2021-09-07 22:00 ` [PATCH 3/7] ARM: Use smp_processor_id() in vfp_pm_suspend instead of ti->cpu Keith Packard
` (5 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Keith Packard @ 2021-09-07 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Keith Packard, Krzysztof Kozlowski,
Linus Walleij, linux-arm-kernel, linux-mm, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
This avoids needing to compute the task pointer in this function,
allowing it to be used as the source of identification in the future.
Signed-off-by: Keith Packard <keithpac@amazon.com>
---
arch/arm/include/asm/smp.h | 3 ++-
arch/arm/kernel/head-nommu.S | 1 +
arch/arm/kernel/head.S | 1 +
arch/arm/kernel/smp.c | 8 +++++---
4 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 86a7fd721556..d43b64635d77 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -48,7 +48,7 @@ extern void set_smp_ipi_range(int ipi_base, int nr_ipi);
* Called from platform specific assembly code, this is the
* secondary CPU entry point.
*/
-asmlinkage void secondary_start_kernel(unsigned int cpu);
+asmlinkage void secondary_start_kernel(unsigned int cpu, struct task_struct *task);
/*
@@ -62,6 +62,7 @@ struct secondary_data {
unsigned long swapper_pg_dir;
void *stack;
unsigned int cpu;
+ struct task_struct *task;
};
extern struct secondary_data secondary_data;
extern void secondary_startup(void);
diff --git a/arch/arm/kernel/head-nommu.S b/arch/arm/kernel/head-nommu.S
index 5aa8ef42717f..218715c135ed 100644
--- a/arch/arm/kernel/head-nommu.S
+++ b/arch/arm/kernel/head-nommu.S
@@ -115,6 +115,7 @@ ENTRY(secondary_startup)
ret r12
1: bl __after_proc_init
ldr r0, [r7, #16] @ set up cpu number
+ ldr r1, [r7, #20] @ set up task pointer
ldr sp, [r7, #12] @ set up the stack pointer
mov fp, #0
b secondary_start_kernel
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 0e541af738e2..4a6cb0b0808b 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -395,6 +395,7 @@ ENDPROC(secondary_startup_arm)
ENTRY(__secondary_switched)
ldr_l r7, secondary_data + 12 @ get secondary_data.stack
ldr_l r0, secondary_data + 16 @ get secondary_data.cpu
+ ldr_l r1, secondary_data + 20 @ get secondary_data.task
mov sp, r7
mov fp, #0
b secondary_start_kernel
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 55cb1689a4b3..5e999f1f1aea 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -154,6 +154,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
secondary_data.swapper_pg_dir = get_arch_pgd(swapper_pg_dir);
#endif
secondary_data.cpu = cpu;
+ secondary_data.task = idle;
sync_cache_w(&secondary_data);
/*
@@ -375,13 +376,14 @@ void arch_cpu_idle_dead(void)
* to be repeated to undo the effects of taking the CPU offline.
*/
__asm__("mov r0, %1\n"
+ " mov r1, %2\n"
" mov sp, %0\n"
" mov fp, #0\n"
" b secondary_start_kernel"
:
: "r" (task_stack_page(current) + THREAD_SIZE - 8),
- "r" (cpu)
- : "r0");
+ "r" (cpu), "r" (current)
+ : "r0", "r1");
}
#endif /* CONFIG_HOTPLUG_CPU */
@@ -404,7 +406,7 @@ static void smp_store_cpu_info(unsigned int cpuid)
* This is the secondary CPU boot entry. We're using this CPUs
* idle thread stack, but a set of temporary page tables.
*/
-asmlinkage void secondary_start_kernel(unsigned int cpu)
+asmlinkage void secondary_start_kernel(unsigned int cpu, struct task_struct *task)
{
struct mm_struct *mm = &init_mm;
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/7] ARM: Use smp_processor_id() in vfp_pm_suspend instead of ti->cpu
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
2021-09-07 22:00 ` [PATCH 1/7] ARM: Pass cpu number to secondary_start_kernel Keith Packard
2021-09-07 22:00 ` [PATCH 2/7] ARM: Pass task " Keith Packard
@ 2021-09-07 22:00 ` Keith Packard
2021-09-07 22:00 ` [PATCH 4/7] ARM: Use hack from powerpc to get current cpu number Keith Packard
` (4 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Keith Packard @ 2021-09-07 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Keith Packard, Krzysztof Kozlowski,
Linus Walleij, linux-arm-kernel, linux-mm, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
These are equivalent when thread_info contains the CPU value, but when
THREAD_INFO_IN_TASK is enabled, cpu moves to task_struct. Using the macro
allows either.
Signed-off-by: Keith Packard <keithpac@amazon.com>
---
arch/arm/vfp/vfpmodule.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 2cb355c1b5b7..d7a3818da671 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -458,16 +458,16 @@ static int vfp_pm_suspend(void)
/* disable, just in case */
fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
- } else if (vfp_current_hw_state[ti->cpu]) {
+ } else if (vfp_current_hw_state[smp_processor_id()]) {
#ifndef CONFIG_SMP
fmxr(FPEXC, fpexc | FPEXC_EN);
- vfp_save_state(vfp_current_hw_state[ti->cpu], fpexc);
+ vfp_save_state(vfp_current_hw_state[smp_processor_id()], fpexc);
fmxr(FPEXC, fpexc);
#endif
}
/* clear any information we had about last context state */
- vfp_current_hw_state[ti->cpu] = NULL;
+ vfp_current_hw_state[smp_processor_id()] = NULL;
return 0;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/7] ARM: Use hack from powerpc to get current cpu number
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
` (2 preceding siblings ...)
2021-09-07 22:00 ` [PATCH 3/7] ARM: Use smp_processor_id() in vfp_pm_suspend instead of ti->cpu Keith Packard
@ 2021-09-07 22:00 ` Keith Packard
2021-09-08 7:45 ` Ard Biesheuvel
2021-09-07 22:00 ` [PATCH 5/7] ARM: Stop using TPIDRPRW to hold per_cpu_offset Keith Packard
` (3 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Keith Packard @ 2021-09-07 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Keith Packard, Krzysztof Kozlowski,
Linus Walleij, linux-arm-kernel, linux-mm, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
When we enable THREAD_INFO_IN_TASK, the cpu number will disappear from
thread_info and reappear in task_struct. As we cannot include
linux/sched.h in asm/smp.h, there's no way to use that struct type in
the raw_smp_processor_id macro. Instead, a hack from the powerpc code
is used. This pulls the TI_CPU offset out of asm-offsets.h and uses
that to find the cpu value.
Signed-off-by: Keith Packard <keithpac@amazon.com>
---
arch/arm/Makefile | 8 ++++++++
arch/arm/include/asm/smp.h | 18 +++++++++++++++++-
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 415c3514573a..6752995d2914 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -284,6 +284,14 @@ stack_protector_prepare: prepare0
$(eval GCC_PLUGINS_CFLAGS += $(SSP_PLUGIN_CFLAGS))
endif
+ifdef CONFIG_SMP
+prepare: task_cpu_prepare
+
+PHONY += task_cpu_prepare
+task_cpu_prepare: prepare0
+ $(eval KBUILD_CFLAGS += -D_TI_CPU=$(shell awk '{if ($$2 == "TI_CPU") print $$3;}' include/generated/asm-offsets.h))
+endif
+
all: $(notdir $(KBUILD_IMAGE))
diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index d43b64635d77..f77ba3753bc4 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -15,7 +15,23 @@
# error "<asm/smp.h> included in non-SMP build"
#endif
-#define raw_smp_processor_id() (current_thread_info()->cpu)
+/*
+ * This is particularly ugly: it appears we can't actually get the
+ * definition of task_struct here, but we need access to the CPU this
+ * task is running on, which is stored in task_struct when
+ * THREAD_INFO_IN_TASK is set. Instead of using task_struct we're
+ * using TI_CPU which is extracted from asm-offsets.h by kbuild to get
+ * the current processor ID.
+ *
+ * This also needs to be safeguarded when building asm-offsets.s
+ * because at that time TI_CPU is not defined yet.
+ */
+#ifndef _TI_CPU
+#define raw_smp_processor_id() (0)
+#else
+#define raw_smp_processor_id() \
+ (*(unsigned int *)((void *)current_thread_info() + _TI_CPU))
+#endif
struct seq_file;
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/7] ARM: Stop using TPIDRPRW to hold per_cpu_offset
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
` (3 preceding siblings ...)
2021-09-07 22:00 ` [PATCH 4/7] ARM: Use hack from powerpc to get current cpu number Keith Packard
@ 2021-09-07 22:00 ` Keith Packard
2021-09-09 13:54 ` Ard Biesheuvel
2021-09-07 22:00 ` [PATCH 6/7] ARM: Use TPIDRPRW for current Keith Packard
` (2 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Keith Packard @ 2021-09-07 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Keith Packard, Krzysztof Kozlowski,
Linus Walleij, linux-arm-kernel, linux-mm, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
We're going to store TPIDRPRW here instead
Signed-off-by: Keith Packard <keithpac@amazon.com>
---
arch/arm/include/asm/percpu.h | 31 -------------------------------
arch/arm/kernel/setup.c | 7 -------
arch/arm/kernel/smp.c | 3 ---
3 files changed, 41 deletions(-)
diff --git a/arch/arm/include/asm/percpu.h b/arch/arm/include/asm/percpu.h
index e2fcb3cfd3de..eeafcd6a3e01 100644
--- a/arch/arm/include/asm/percpu.h
+++ b/arch/arm/include/asm/percpu.h
@@ -7,37 +7,6 @@
register unsigned long current_stack_pointer asm ("sp");
-/*
- * Same as asm-generic/percpu.h, except that we store the per cpu offset
- * in the TPIDRPRW. TPIDRPRW only exists on V6K and V7
- */
-#if defined(CONFIG_SMP) && !defined(CONFIG_CPU_V6)
-static inline void set_my_cpu_offset(unsigned long off)
-{
- /* Set TPIDRPRW */
- asm volatile("mcr p15, 0, %0, c13, c0, 4" : : "r" (off) : "memory");
-}
-
-static inline unsigned long __my_cpu_offset(void)
-{
- unsigned long off;
-
- /*
- * Read TPIDRPRW.
- * We want to allow caching the value, so avoid using volatile and
- * instead use a fake stack read to hazard against barrier().
- */
- asm("mrc p15, 0, %0, c13, c0, 4" : "=r" (off)
- : "Q" (*(const unsigned long *)current_stack_pointer));
-
- return off;
-}
-#define __my_cpu_offset __my_cpu_offset()
-#else
-#define set_my_cpu_offset(x) do {} while(0)
-
-#endif /* CONFIG_SMP */
-
#include <asm-generic/percpu.h>
#endif /* _ASM_ARM_PERCPU_H_ */
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index ca0201635fac..d0dc60afe54f 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -590,13 +590,6 @@ void __init smp_setup_processor_id(void)
for (i = 1; i < nr_cpu_ids; ++i)
cpu_logical_map(i) = i == cpu ? 0 : i;
- /*
- * clear __my_cpu_offset on boot CPU to avoid hang caused by
- * using percpu variable early, for example, lockdep will
- * access percpu variable inside lock_release
- */
- set_my_cpu_offset(0);
-
pr_info("Booting Linux on physical CPU 0x%x\n", mpidr);
}
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 5e999f1f1aea..8ccf10b34f08 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -410,8 +410,6 @@ asmlinkage void secondary_start_kernel(unsigned int cpu, struct task_struct *tas
{
struct mm_struct *mm = &init_mm;
- set_my_cpu_offset(per_cpu_offset(cpu));
-
secondary_biglittle_init();
/*
@@ -495,7 +493,6 @@ void __init smp_cpus_done(unsigned int max_cpus)
void __init smp_prepare_boot_cpu(void)
{
- set_my_cpu_offset(per_cpu_offset(smp_processor_id()));
}
void __init smp_prepare_cpus(unsigned int max_cpus)
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/7] ARM: Use TPIDRPRW for current
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
` (4 preceding siblings ...)
2021-09-07 22:00 ` [PATCH 5/7] ARM: Stop using TPIDRPRW to hold per_cpu_offset Keith Packard
@ 2021-09-07 22:00 ` Keith Packard
2021-09-09 13:56 ` Ard Biesheuvel
2021-09-07 22:00 ` [PATCH 7/7] ARM: Move thread_info into task_struct (v7 only) Keith Packard
2021-09-08 7:01 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Krzysztof Kozlowski
7 siblings, 1 reply; 14+ messages in thread
From: Keith Packard @ 2021-09-07 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Keith Packard, Krzysztof Kozlowski,
Linus Walleij, linux-arm-kernel, linux-mm, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
Store current task pointer in CPU thread ID register TPIDRPRW so that
accessing it doesn't depend on being able to locate thread_info off of
the kernel stack pointer.
Signed-off-by: Keith Packard <keithpac@amazon.com>
---
arch/arm/Kconfig | 4 +++
arch/arm/include/asm/assembler.h | 8 +++++
arch/arm/include/asm/current.h | 52 ++++++++++++++++++++++++++++++++
arch/arm/kernel/entry-armv.S | 4 +++
arch/arm/kernel/setup.c | 1 +
arch/arm/kernel/smp.c | 1 +
6 files changed, 70 insertions(+)
create mode 100644 arch/arm/include/asm/current.h
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 24804f11302d..414fe23fd5ac 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1172,6 +1172,10 @@ config SMP_ON_UP
If you don't know what to do here, say Y.
+config CURRENT_POINTER_IN_TPIDRPRW
+ def_bool y
+ depends on (CPU_V6K || CPU_V7) && !CPU_V6
+
config ARM_CPU_TOPOLOGY
bool "Support cpu topology definition"
depends on SMP && CPU_V7
diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index e2b1fd558bf3..ea12fe3bb589 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -209,6 +209,14 @@
mov \rd, \rd, lsl #THREAD_SIZE_ORDER + PAGE_SHIFT
.endm
+/*
+ * Set current task_info
+ * @src: Source register containing task_struct pointer
+ */
+ .macro set_current src : req
+ mcr p15, 0, \src, c13, c0, 4
+ .endm
+
/*
* Increment/decrement the preempt count.
*/
diff --git a/arch/arm/include/asm/current.h b/arch/arm/include/asm/current.h
new file mode 100644
index 000000000000..153a2ea18747
--- /dev/null
+++ b/arch/arm/include/asm/current.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright © 2021 Keith Packard <keithp@keithp.com>
+ */
+
+#ifndef _ASM_ARM_CURRENT_H_
+#define _ASM_ARM_CURRENT_H_
+
+#ifndef __ASSEMBLY__
+
+register unsigned long current_stack_pointer asm ("sp");
+
+/*
+ * Same as asm-generic/current.h, except that we store current
+ * in TPIDRPRW. TPIDRPRW only exists on V6K and V7
+ */
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRPRW
+
+struct task_struct;
+
+static inline void set_current(struct task_struct *tsk)
+{
+ /* Set TPIDRPRW */
+ asm volatile("mcr p15, 0, %0, c13, c0, 4" : : "r" (tsk) : "memory");
+}
+
+static __always_inline struct task_struct *get_current(void)
+{
+ struct task_struct *tsk;
+
+ /*
+ * Read TPIDRPRW.
+ * We want to allow caching the value, so avoid using volatile and
+ * instead use a fake stack read to hazard against barrier().
+ */
+ asm("mrc p15, 0, %0, c13, c0, 4" : "=r" (tsk)
+ : "Q" (*(const unsigned long *)current_stack_pointer));
+
+ return tsk;
+}
+#define current get_current()
+#else
+
+#define set_current(tsk) do {} while (0)
+
+#include <asm-generic/current.h>
+
+#endif /* CONFIG_SMP */
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_ARM_CURRENT_H_ */
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 0ea8529a4872..db3947ee9c3e 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -761,6 +761,10 @@ ENTRY(__switch_to)
ldr r6, [r2, #TI_CPU_DOMAIN]
#endif
switch_tls r1, r4, r5, r3, r7
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRPRW
+ ldr r7, [r2, #TI_TASK]
+ set_current r7
+#endif
#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
ldr r7, [r2, #TI_TASK]
ldr r8, =__stack_chk_guard
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index d0dc60afe54f..2fdf8c31d6c9 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -586,6 +586,7 @@ void __init smp_setup_processor_id(void)
u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
u32 cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+ set_current(&init_task);
cpu_logical_map(0) = cpu;
for (i = 1; i < nr_cpu_ids; ++i)
cpu_logical_map(i) = i == cpu ? 0 : i;
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 8ccf10b34f08..09771916442a 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -410,6 +410,7 @@ asmlinkage void secondary_start_kernel(unsigned int cpu, struct task_struct *tas
{
struct mm_struct *mm = &init_mm;
+ set_current(task);
secondary_biglittle_init();
/*
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 7/7] ARM: Move thread_info into task_struct (v7 only)
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
` (5 preceding siblings ...)
2021-09-07 22:00 ` [PATCH 6/7] ARM: Use TPIDRPRW for current Keith Packard
@ 2021-09-07 22:00 ` Keith Packard
2021-09-08 7:01 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Krzysztof Kozlowski
7 siblings, 0 replies; 14+ messages in thread
From: Keith Packard @ 2021-09-07 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Keith Packard, Krzysztof Kozlowski,
Linus Walleij, linux-arm-kernel, linux-mm, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
This avoids many stack overflow attacks which modified the thread_info
structure by moving that into the task_struct as is done is almost all
other architectures.
This depends on having 'current' stored in the TPIDRPRW register as
that allows us to find thread_info and task_struct once the
thread_info cannot be located using the kernel stack pointer.
Signed-off-by: Keith Packard <keithpac@amazon.com>
---
arch/arm/Kconfig | 1 +
arch/arm/include/asm/assembler.h | 4 ++++
arch/arm/include/asm/thread_info.h | 12 +++++++++++-
arch/arm/kernel/asm-offsets.c | 4 ++++
arch/arm/kernel/entry-armv.S | 4 ++++
arch/arm/vfp/vfpmodule.c | 9 +++++++++
6 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 414fe23fd5ac..5846b4f5444b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -128,6 +128,7 @@ config ARM
select RTC_LIB
select SET_FS
select SYS_SUPPORTS_APM_EMULATION
+ select THREAD_INFO_IN_TASK if CURRENT_POINTER_IN_TPIDRPRW
# Above selects are sorted alphabetically; please add new ones
# according to that. Thanks.
help
diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index ea12fe3bb589..b23d2b87184a 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -203,10 +203,14 @@
* Get current thread_info.
*/
.macro get_thread_info, rd
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+ mrc p15, 0, \rd, c13, c0, 4
+#else
ARM( mov \rd, sp, lsr #THREAD_SIZE_ORDER + PAGE_SHIFT )
THUMB( mov \rd, sp )
THUMB( lsr \rd, \rd, #THREAD_SIZE_ORDER + PAGE_SHIFT )
mov \rd, \rd, lsl #THREAD_SIZE_ORDER + PAGE_SHIFT
+#endif
.endm
/*
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 70d4cbc49ae1..6b67703ca16a 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -55,8 +55,10 @@ struct thread_info {
unsigned long flags; /* low level flags */
int preempt_count; /* 0 => preemptable, <0 => bug */
mm_segment_t addr_limit; /* address limit */
+#ifndef CONFIG_THREAD_INFO_IN_TASK
struct task_struct *task; /* main task structure */
__u32 cpu; /* cpu */
+#endif
__u32 cpu_domain; /* cpu domain */
#ifdef CONFIG_STACKPROTECTOR_PER_TASK
unsigned long stack_canary;
@@ -75,14 +77,21 @@ struct thread_info {
#endif
};
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+#define INIT_THREAD_INFO_TASK(tsk)
+#else
+#define INIT_THREAD_INFO_TASK(tsk) .task = &tsk,
+#endif
+
#define INIT_THREAD_INFO(tsk) \
{ \
- .task = &tsk, \
+ INIT_THREAD_INFO_TASK(tsk) \
.flags = 0, \
.preempt_count = INIT_PREEMPT_COUNT, \
.addr_limit = KERNEL_DS, \
}
+#ifndef CONFIG_THREAD_INFO_IN_TASK
/*
* how to get the thread information struct from C
*/
@@ -93,6 +102,7 @@ static inline struct thread_info *current_thread_info(void)
return (struct thread_info *)
(current_stack_pointer & ~(THREAD_SIZE - 1));
}
+#endif
#define thread_saved_pc(tsk) \
((unsigned long)(task_thread_info(tsk)->cpu_context.pc))
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 70993af22d80..2a6745f7423e 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -44,8 +44,12 @@ int main(void)
DEFINE(TI_FLAGS, offsetof(struct thread_info, flags));
DEFINE(TI_PREEMPT, offsetof(struct thread_info, preempt_count));
DEFINE(TI_ADDR_LIMIT, offsetof(struct thread_info, addr_limit));
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+ DEFINE(TI_CPU, offsetof(struct task_struct, cpu));
+#else
DEFINE(TI_TASK, offsetof(struct thread_info, task));
DEFINE(TI_CPU, offsetof(struct thread_info, cpu));
+#endif
DEFINE(TI_CPU_DOMAIN, offsetof(struct thread_info, cpu_domain));
DEFINE(TI_CPU_SAVE, offsetof(struct thread_info, cpu_context));
DEFINE(TI_USED_CP, offsetof(struct thread_info, used_cp));
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index db3947ee9c3e..5ae687c8c7b8 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -762,9 +762,13 @@ ENTRY(__switch_to)
#endif
switch_tls r1, r4, r5, r3, r7
#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRPRW
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+ set_current r2
+#else
ldr r7, [r2, #TI_TASK]
set_current r7
#endif
+#endif
#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
ldr r7, [r2, #TI_TASK]
ldr r8, =__stack_chk_guard
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index d7a3818da671..84a691da59fa 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -158,7 +158,12 @@ static void vfp_thread_copy(struct thread_info *thread)
*/
static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
{
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+ struct task_struct *tsk = v;
+ struct thread_info *thread = &tsk->thread_info;
+#else
struct thread_info *thread = v;
+#endif
u32 fpexc;
#ifdef CONFIG_SMP
unsigned int cpu;
@@ -169,7 +174,11 @@ static int vfp_notifier(struct notifier_block *self, unsigned long cmd, void *v)
fpexc = fmrx(FPEXC);
#ifdef CONFIG_SMP
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+ cpu = tsk->cpu;
+#else
cpu = thread->cpu;
+#endif
/*
* On SMP, if VFP is enabled, save the old state in
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3)
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
` (6 preceding siblings ...)
2021-09-07 22:00 ` [PATCH 7/7] ARM: Move thread_info into task_struct (v7 only) Keith Packard
@ 2021-09-08 7:01 ` Krzysztof Kozlowski
2021-09-08 7:47 ` Ard Biesheuvel
7 siblings, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-08 7:01 UTC (permalink / raw)
To: Keith Packard, linux-kernel
Cc: Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Ard Biesheuvel, Arnd Bergmann, Bjorn Andersson,
Christoph Lameter, Dennis Zhou, Geert Uytterhoeven, Jens Axboe,
Joe Perches, Kees Cook, Linus Walleij, linux-arm-kernel,
linux-mm, Manivannan Sadhasivam, Marc Zyngier, Masahiro Yamada,
Mike Rapoport, Nathan Chancellor, Nick Desaulniers,
Nick Desaulniers, Nicolas Pitre, Russell King, Tejun Heo,
Thomas Gleixner, Uwe Kleine-König, Valentin Schneider,
Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
On 08/09/2021 00:00, Keith Packard wrote:
> Placing thread_info in the kernel stack leaves it vulnerable to stack
> overflow attacks. This short series addresses that by using the
> existing THREAD_INFO_IN_TASK infrastructure.
>
> This is the third version of this series, in this version the changes
> are restricted to hardware which provides the TPIDRPRW register. This
> register is repurposed from holding the per_cpu_offset value to
> holding the 'current' value as that allows fetching this value
> atomically so that it can be used in a preemptable context.
>
> The series is broken into seven pieces:
>
> 1) Change the secondary_start_kernel API to receive the cpu
> number. This avoids needing to be able to find this value independently in
> future patches.
>
> 2) Change the secondary_start_kernel API to also receive the 'task'
> value. Passing the value to this function also avoids needing to
> be able to discover it independently.
>
> 3) A cleanup which avoids assuming that THREAD_INFO_IN_TASK is not set.
>
> 4) A hack, borrowed from the powerpc arch, which allows locating the 'cpu'
> field in either thread_info or task_struct, without requiring linux/sched.h
> to be included in asm/smp.h
>
> 5) Disable the optimization storing per_cpu_offset in TPIDRPRW. This leaves
> the register free to hold 'current' instead.
>
> 6) Use TPIDRPRW for 'current'. This is enabled for either CPU_V6K or CPU_V7,
> but not if CPU_V6 is also enabled.
>
> 7) Enable THREAD_INFO_IN_TASK whenever TPIDRPRW is used to hold 'current'.
Hi,
Thanks for your patches. This seems to be a v3 but the patches are not
marked with it. Use "-v3" in format-patch to get it right.
The email here also lacks diffstat which is useful, for example to check
whether any maintainer's relevant files are touched here. You can get it
with "--cover-letter".
In total the command should look like:
git format-patch --cover-letter -v3 -7 HEAD
Of course you can use any other tools to achieve the same result but as
of now - result is not the same.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/7] ARM: Use hack from powerpc to get current cpu number
2021-09-07 22:00 ` [PATCH 4/7] ARM: Use hack from powerpc to get current cpu number Keith Packard
@ 2021-09-08 7:45 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2021-09-08 7:45 UTC (permalink / raw)
To: Keith Packard
Cc: Linux Kernel Mailing List, Abbott Liu, Andrew Morton,
Andrey Ryabinin, Anshuman Khandual, Arnd Bergmann,
Bjorn Andersson, Christoph Lameter, Dennis Zhou,
Geert Uytterhoeven, Jens Axboe, Joe Perches, Kees Cook,
Krzysztof Kozlowski, Linus Walleij, Linux ARM,
Linux Memory Management List, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
On Wed, 8 Sept 2021 at 00:00, Keith Packard <keithpac@amazon.com> wrote:
>
> When we enable THREAD_INFO_IN_TASK, the cpu number will disappear from
> thread_info and reappear in task_struct. As we cannot include
> linux/sched.h in asm/smp.h, there's no way to use that struct type in
> the raw_smp_processor_id macro. Instead, a hack from the powerpc code
> is used. This pulls the TI_CPU offset out of asm-offsets.h and uses
> that to find the cpu value.
>
> Signed-off-by: Keith Packard <keithpac@amazon.com>
> ---
> arch/arm/Makefile | 8 ++++++++
> arch/arm/include/asm/smp.h | 18 +++++++++++++++++-
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index 415c3514573a..6752995d2914 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -284,6 +284,14 @@ stack_protector_prepare: prepare0
> $(eval GCC_PLUGINS_CFLAGS += $(SSP_PLUGIN_CFLAGS))
> endif
>
> +ifdef CONFIG_SMP
> +prepare: task_cpu_prepare
> +
> +PHONY += task_cpu_prepare
> +task_cpu_prepare: prepare0
> + $(eval KBUILD_CFLAGS += -D_TI_CPU=$(shell awk '{if ($$2 == "TI_CPU") print $$3;}' include/generated/asm-offsets.h))
> +endif
> +
> all: $(notdir $(KBUILD_IMAGE))
>
>
> diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
> index d43b64635d77..f77ba3753bc4 100644
> --- a/arch/arm/include/asm/smp.h
> +++ b/arch/arm/include/asm/smp.h
> @@ -15,7 +15,23 @@
> # error "<asm/smp.h> included in non-SMP build"
> #endif
>
> -#define raw_smp_processor_id() (current_thread_info()->cpu)
> +/*
> + * This is particularly ugly: it appears we can't actually get the
> + * definition of task_struct here, but we need access to the CPU this
> + * task is running on, which is stored in task_struct when
> + * THREAD_INFO_IN_TASK is set. Instead of using task_struct we're
> + * using TI_CPU which is extracted from asm-offsets.h by kbuild to get
> + * the current processor ID.
> + *
> + * This also needs to be safeguarded when building asm-offsets.s
> + * because at that time TI_CPU is not defined yet.
> + */
> +#ifndef _TI_CPU
> +#define raw_smp_processor_id() (0)
> +#else
> +#define raw_smp_processor_id() \
> + (*(unsigned int *)((void *)current_thread_info() + _TI_CPU))
> +#endif
>
> struct seq_file;
>
As I stated before, I would really like to avoid using this hack - I
don't think we need to.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3)
2021-09-08 7:01 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Krzysztof Kozlowski
@ 2021-09-08 7:47 ` Ard Biesheuvel
2021-09-08 7:50 ` Geert Uytterhoeven
0 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2021-09-08 7:47 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Keith Packard, Linux Kernel Mailing List, Abbott Liu,
Andrew Morton, Andrey Ryabinin, Anshuman Khandual, Arnd Bergmann,
Bjorn Andersson, Christoph Lameter, Dennis Zhou,
Geert Uytterhoeven, Jens Axboe, Joe Perches, Kees Cook,
Linus Walleij, Linux ARM, Linux Memory Management List,
Manivannan Sadhasivam, Marc Zyngier, Masahiro Yamada,
Mike Rapoport, Nathan Chancellor, Nick Desaulniers,
Nick Desaulniers, Nicolas Pitre, Russell King, Tejun Heo,
Thomas Gleixner, Uwe Kleine-König, Valentin Schneider,
Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
On Wed, 8 Sept 2021 at 09:01, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 08/09/2021 00:00, Keith Packard wrote:
> > Placing thread_info in the kernel stack leaves it vulnerable to stack
> > overflow attacks. This short series addresses that by using the
> > existing THREAD_INFO_IN_TASK infrastructure.
> >
> > This is the third version of this series, in this version the changes
> > are restricted to hardware which provides the TPIDRPRW register. This
> > register is repurposed from holding the per_cpu_offset value to
> > holding the 'current' value as that allows fetching this value
> > atomically so that it can be used in a preemptable context.
> >
> > The series is broken into seven pieces:
> >
> > 1) Change the secondary_start_kernel API to receive the cpu
> > number. This avoids needing to be able to find this value independently in
> > future patches.
> >
> > 2) Change the secondary_start_kernel API to also receive the 'task'
> > value. Passing the value to this function also avoids needing to
> > be able to discover it independently.
> >
> > 3) A cleanup which avoids assuming that THREAD_INFO_IN_TASK is not set.
> >
> > 4) A hack, borrowed from the powerpc arch, which allows locating the 'cpu'
> > field in either thread_info or task_struct, without requiring linux/sched.h
> > to be included in asm/smp.h
> >
> > 5) Disable the optimization storing per_cpu_offset in TPIDRPRW. This leaves
> > the register free to hold 'current' instead.
> >
> > 6) Use TPIDRPRW for 'current'. This is enabled for either CPU_V6K or CPU_V7,
> > but not if CPU_V6 is also enabled.
> >
> > 7) Enable THREAD_INFO_IN_TASK whenever TPIDRPRW is used to hold 'current'.
>
> Hi,
>
> Thanks for your patches. This seems to be a v3 but the patches are not
> marked with it. Use "-v3" in format-patch to get it right.
>
> The email here also lacks diffstat which is useful, for example to check
> whether any maintainer's relevant files are touched here. You can get it
> with "--cover-letter".
>
> In total the command should look like:
> git format-patch --cover-letter -v3 -7 HEAD
>
> Of course you can use any other tools to achieve the same result but as
> of now - result is not the same.
>
Also, this ended up in my GMail spam folder, likely due to some
antispam ID header being set incorrectly?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3)
2021-09-08 7:47 ` Ard Biesheuvel
@ 2021-09-08 7:50 ` Geert Uytterhoeven
0 siblings, 0 replies; 14+ messages in thread
From: Geert Uytterhoeven @ 2021-09-08 7:50 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Krzysztof Kozlowski, Keith Packard, Linux Kernel Mailing List,
Abbott Liu, Andrew Morton, Andrey Ryabinin, Anshuman Khandual,
Arnd Bergmann, Bjorn Andersson, Christoph Lameter, Dennis Zhou,
Geert Uytterhoeven, Jens Axboe, Joe Perches, Kees Cook,
Linus Walleij, Linux ARM, Linux Memory Management List,
Manivannan Sadhasivam, Marc Zyngier, Masahiro Yamada,
Mike Rapoport, Nathan Chancellor, Nick Desaulniers,
Nick Desaulniers, Nicolas Pitre, Russell King, Tejun Heo,
Thomas Gleixner, Uwe Kleine-König, Valentin Schneider,
Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
Hi Ard,
On Wed, Sep 8, 2021 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> Also, this ended up in my GMail spam folder, likely due to some
> antispam ID header being set incorrectly?
I have a rule to never mark as spam email with "patch" in the subject.
So far only one false positive in all these years ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/7] ARM: Stop using TPIDRPRW to hold per_cpu_offset
2021-09-07 22:00 ` [PATCH 5/7] ARM: Stop using TPIDRPRW to hold per_cpu_offset Keith Packard
@ 2021-09-09 13:54 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2021-09-09 13:54 UTC (permalink / raw)
To: Keith Packard
Cc: Linux Kernel Mailing List, Abbott Liu, Andrew Morton,
Andrey Ryabinin, Anshuman Khandual, Arnd Bergmann,
Bjorn Andersson, Christoph Lameter, Dennis Zhou,
Geert Uytterhoeven, Jens Axboe, Joe Perches, Kees Cook,
Krzysztof Kozlowski, Linus Walleij, Linux ARM,
Linux Memory Management List, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
On Wed, 8 Sept 2021 at 00:00, Keith Packard <keithpac@amazon.com> wrote:
>
> We're going to store TPIDRPRW here instead
>
?
> Signed-off-by: Keith Packard <keithpac@amazon.com>
I'd much prefer to keep using TPIDIRPRW for the per-CPU offsets, and
use the user space TLS register for current.
There are several reasons for this:
- arm64 does the same - as someone who still cares about ARM while
many have moved on to arm64 or RISC-V, I am still trying to maintain
parity between ARM and arm64 where possible.
- efficiency: loading the per-CPU offset using a CPU id stored in
memory, which is then used to index the per-CPU offsets array in
memory adds two additional loads to every load/store of a per-CPU
variable
- 'current' usually does not change value under the code's feet,
whereas per-CPU offsets might change at any time. Given the fact that
the CPU offset load is visible to the compiler as a memory access, I
suppose this should be safe, but I would still prefer per-CPU access
to avoid going via current if possible.
> ---
> arch/arm/include/asm/percpu.h | 31 -------------------------------
> arch/arm/kernel/setup.c | 7 -------
> arch/arm/kernel/smp.c | 3 ---
> 3 files changed, 41 deletions(-)
>
> diff --git a/arch/arm/include/asm/percpu.h b/arch/arm/include/asm/percpu.h
> index e2fcb3cfd3de..eeafcd6a3e01 100644
> --- a/arch/arm/include/asm/percpu.h
> +++ b/arch/arm/include/asm/percpu.h
> @@ -7,37 +7,6 @@
>
> register unsigned long current_stack_pointer asm ("sp");
>
> -/*
> - * Same as asm-generic/percpu.h, except that we store the per cpu offset
> - * in the TPIDRPRW. TPIDRPRW only exists on V6K and V7
> - */
> -#if defined(CONFIG_SMP) && !defined(CONFIG_CPU_V6)
> -static inline void set_my_cpu_offset(unsigned long off)
> -{
> - /* Set TPIDRPRW */
> - asm volatile("mcr p15, 0, %0, c13, c0, 4" : : "r" (off) : "memory");
> -}
> -
> -static inline unsigned long __my_cpu_offset(void)
> -{
> - unsigned long off;
> -
> - /*
> - * Read TPIDRPRW.
> - * We want to allow caching the value, so avoid using volatile and
> - * instead use a fake stack read to hazard against barrier().
> - */
> - asm("mrc p15, 0, %0, c13, c0, 4" : "=r" (off)
> - : "Q" (*(const unsigned long *)current_stack_pointer));
> -
> - return off;
> -}
> -#define __my_cpu_offset __my_cpu_offset()
> -#else
> -#define set_my_cpu_offset(x) do {} while(0)
> -
> -#endif /* CONFIG_SMP */
> -
> #include <asm-generic/percpu.h>
>
> #endif /* _ASM_ARM_PERCPU_H_ */
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index ca0201635fac..d0dc60afe54f 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -590,13 +590,6 @@ void __init smp_setup_processor_id(void)
> for (i = 1; i < nr_cpu_ids; ++i)
> cpu_logical_map(i) = i == cpu ? 0 : i;
>
> - /*
> - * clear __my_cpu_offset on boot CPU to avoid hang caused by
> - * using percpu variable early, for example, lockdep will
> - * access percpu variable inside lock_release
> - */
> - set_my_cpu_offset(0);
> -
> pr_info("Booting Linux on physical CPU 0x%x\n", mpidr);
> }
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 5e999f1f1aea..8ccf10b34f08 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -410,8 +410,6 @@ asmlinkage void secondary_start_kernel(unsigned int cpu, struct task_struct *tas
> {
> struct mm_struct *mm = &init_mm;
>
> - set_my_cpu_offset(per_cpu_offset(cpu));
> -
> secondary_biglittle_init();
>
> /*
> @@ -495,7 +493,6 @@ void __init smp_cpus_done(unsigned int max_cpus)
>
> void __init smp_prepare_boot_cpu(void)
> {
> - set_my_cpu_offset(per_cpu_offset(smp_processor_id()));
> }
>
> void __init smp_prepare_cpus(unsigned int max_cpus)
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 6/7] ARM: Use TPIDRPRW for current
2021-09-07 22:00 ` [PATCH 6/7] ARM: Use TPIDRPRW for current Keith Packard
@ 2021-09-09 13:56 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2021-09-09 13:56 UTC (permalink / raw)
To: Keith Packard
Cc: Linux Kernel Mailing List, Abbott Liu, Andrew Morton,
Andrey Ryabinin, Anshuman Khandual, Arnd Bergmann,
Bjorn Andersson, Christoph Lameter, Dennis Zhou,
Geert Uytterhoeven, Jens Axboe, Joe Perches, Kees Cook,
Krzysztof Kozlowski, Linus Walleij, Linux ARM,
Linux Memory Management List, Manivannan Sadhasivam,
Marc Zyngier, Masahiro Yamada, Mike Rapoport, Nathan Chancellor,
Nick Desaulniers, Nick Desaulniers, Nicolas Pitre, Russell King,
Tejun Heo, Thomas Gleixner, Uwe Kleine-König,
Valentin Schneider, Viresh Kumar, Wolfram Sang (Renesas),
YiFei Zhu
On Wed, 8 Sept 2021 at 00:00, Keith Packard <keithpac@amazon.com> wrote:
>
> Store current task pointer in CPU thread ID register TPIDRPRW so that
> accessing it doesn't depend on being able to locate thread_info off of
> the kernel stack pointer.
>
> Signed-off-by: Keith Packard <keithpac@amazon.com>
> ---
> arch/arm/Kconfig | 4 +++
> arch/arm/include/asm/assembler.h | 8 +++++
> arch/arm/include/asm/current.h | 52 ++++++++++++++++++++++++++++++++
> arch/arm/kernel/entry-armv.S | 4 +++
> arch/arm/kernel/setup.c | 1 +
> arch/arm/kernel/smp.c | 1 +
> 6 files changed, 70 insertions(+)
> create mode 100644 arch/arm/include/asm/current.h
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 24804f11302d..414fe23fd5ac 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1172,6 +1172,10 @@ config SMP_ON_UP
>
> If you don't know what to do here, say Y.
>
> +config CURRENT_POINTER_IN_TPIDRPRW
> + def_bool y
> + depends on (CPU_V6K || CPU_V7) && !CPU_V6
> +
> config ARM_CPU_TOPOLOGY
> bool "Support cpu topology definition"
> depends on SMP && CPU_V7
> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
> index e2b1fd558bf3..ea12fe3bb589 100644
> --- a/arch/arm/include/asm/assembler.h
> +++ b/arch/arm/include/asm/assembler.h
> @@ -209,6 +209,14 @@
> mov \rd, \rd, lsl #THREAD_SIZE_ORDER + PAGE_SHIFT
> .endm
>
> +/*
> + * Set current task_info
> + * @src: Source register containing task_struct pointer
> + */
> + .macro set_current src : req
> + mcr p15, 0, \src, c13, c0, 4
> + .endm
> +
> /*
> * Increment/decrement the preempt count.
> */
> diff --git a/arch/arm/include/asm/current.h b/arch/arm/include/asm/current.h
> new file mode 100644
> index 000000000000..153a2ea18747
> --- /dev/null
> +++ b/arch/arm/include/asm/current.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright © 2021 Keith Packard <keithp@keithp.com>
> + */
> +
> +#ifndef _ASM_ARM_CURRENT_H_
> +#define _ASM_ARM_CURRENT_H_
> +
> +#ifndef __ASSEMBLY__
> +
> +register unsigned long current_stack_pointer asm ("sp");
> +
> +/*
> + * Same as asm-generic/current.h, except that we store current
> + * in TPIDRPRW. TPIDRPRW only exists on V6K and V7
> + */
> +#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRPRW
> +
> +struct task_struct;
> +
> +static inline void set_current(struct task_struct *tsk)
> +{
> + /* Set TPIDRPRW */
> + asm volatile("mcr p15, 0, %0, c13, c0, 4" : : "r" (tsk) : "memory");
> +}
> +
> +static __always_inline struct task_struct *get_current(void)
> +{
> + struct task_struct *tsk;
> +
> + /*
> + * Read TPIDRPRW.
> + * We want to allow caching the value, so avoid using volatile and
> + * instead use a fake stack read to hazard against barrier().
> + */
> + asm("mrc p15, 0, %0, c13, c0, 4" : "=r" (tsk)
> + : "Q" (*(const unsigned long *)current_stack_pointer));
> +
> + return tsk;
> +}
> +#define current get_current()
> +#else
> +
> +#define set_current(tsk) do {} while (0)
> +
> +#include <asm-generic/current.h>
> +
> +#endif /* CONFIG_SMP */
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* _ASM_ARM_CURRENT_H_ */
> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> index 0ea8529a4872..db3947ee9c3e 100644
> --- a/arch/arm/kernel/entry-armv.S
> +++ b/arch/arm/kernel/entry-armv.S
> @@ -761,6 +761,10 @@ ENTRY(__switch_to)
> ldr r6, [r2, #TI_CPU_DOMAIN]
> #endif
> switch_tls r1, r4, r5, r3, r7
> +#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRPRW
> + ldr r7, [r2, #TI_TASK]
> + set_current r7
> +#endif
This is too early: this will cause the thread notification hooks to be
called with current pointing to the new task instead of the old one.
> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
> ldr r7, [r2, #TI_TASK]
> ldr r8, =__stack_chk_guard
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index d0dc60afe54f..2fdf8c31d6c9 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -586,6 +586,7 @@ void __init smp_setup_processor_id(void)
> u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
> u32 cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>
> + set_current(&init_task);
> cpu_logical_map(0) = cpu;
> for (i = 1; i < nr_cpu_ids; ++i)
> cpu_logical_map(i) = i == cpu ? 0 : i;
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 8ccf10b34f08..09771916442a 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -410,6 +410,7 @@ asmlinkage void secondary_start_kernel(unsigned int cpu, struct task_struct *tas
> {
> struct mm_struct *mm = &init_mm;
>
> + set_current(task);
> secondary_biglittle_init();
>
> /*
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-09-09 13:57 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210904060908.1310204-1-keithp@keithp.com>
2021-09-07 22:00 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Keith Packard
2021-09-07 22:00 ` [PATCH 1/7] ARM: Pass cpu number to secondary_start_kernel Keith Packard
2021-09-07 22:00 ` [PATCH 2/7] ARM: Pass task " Keith Packard
2021-09-07 22:00 ` [PATCH 3/7] ARM: Use smp_processor_id() in vfp_pm_suspend instead of ti->cpu Keith Packard
2021-09-07 22:00 ` [PATCH 4/7] ARM: Use hack from powerpc to get current cpu number Keith Packard
2021-09-08 7:45 ` Ard Biesheuvel
2021-09-07 22:00 ` [PATCH 5/7] ARM: Stop using TPIDRPRW to hold per_cpu_offset Keith Packard
2021-09-09 13:54 ` Ard Biesheuvel
2021-09-07 22:00 ` [PATCH 6/7] ARM: Use TPIDRPRW for current Keith Packard
2021-09-09 13:56 ` Ard Biesheuvel
2021-09-07 22:00 ` [PATCH 7/7] ARM: Move thread_info into task_struct (v7 only) Keith Packard
2021-09-08 7:01 ` [PATCH 0/7] ARM: support THREAD_INFO_IN_TASK (v3) Krzysztof Kozlowski
2021-09-08 7:47 ` Ard Biesheuvel
2021-09-08 7:50 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).