* [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK
@ 2021-09-13 10:39 Ard Biesheuvel
2021-09-13 10:39 ` [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support Ard Biesheuvel
` (7 more replies)
0 siblings, 8 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-13 10:39 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Ard Biesheuvel, Keith Packard, Russell King, Kees Cook,
Arnd Bergmann, Linus Walleij
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 v4 is a follow-up to Keith's v3 [0], which did not address all
concerns I raised in response to v2. After collaborating with Keith
off-list, we decided that I should go ahead and post our joint v4.
Changes since v3:
- Leave the CPU field in thread_info, and keep it in sync at context
switch time. This is by far the easiest and cleanest way to work
around the fact that it is infeasible to implement
raw_smp_processor_id() in terms of task_struct::cpu (for reasons of
header soup).
- Drop the VFP changes, they are no longer necessary given the previous
point.
- Drop the change to pass the CPU number to secondary_start_kernel().
Given that we also need to pass the idle task pointer, which carries
the CPU number, passing the CPU number directly is redundant.
- Use the TPIDRURO register to carry 'current' while running in the
kernel, and keep using TPIDRPRW for the per-CPU offset as before. This
way, there is no need to make any changes to the way the per-CPU offsets
are programmed. It also avoids the concurrency issues that would
result from carrying the 'current' pointer in a per-CPU variable.
- Update the per-task stack protector plugin to pull the stack canary
value directly from the task struct.
Cc: Keith Packard <keithpac@amazon.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
[0] https://lore.kernel.org/all/20210907220038.91021-1-keithpac@amazon.com/
Ard Biesheuvel (4):
gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support
ARM: smp: Free up the TLS register while running in the kernel
ARM: smp: Store current pointer in TPIDRURO register if available
ARM: smp: Enable THREAD_INFO_IN_TASK
Keith Packard (1):
ARM: smp: Pass task to secondary_start_kernel
arch/arm/Kconfig | 8 ++-
arch/arm/Makefile | 5 +-
arch/arm/include/asm/assembler.h | 29 +++++++++++
arch/arm/include/asm/current.h | 51 ++++++++++++++++++++
arch/arm/include/asm/smp.h | 3 +-
arch/arm/include/asm/stackprotector.h | 2 -
arch/arm/include/asm/switch_to.h | 16 ++++++
arch/arm/include/asm/thread_info.h | 15 ++++--
arch/arm/include/asm/tls.h | 10 ++--
arch/arm/kernel/asm-offsets.c | 6 +--
arch/arm/kernel/entry-armv.S | 8 ++-
arch/arm/kernel/entry-common.S | 1 +
arch/arm/kernel/entry-header.S | 8 +++
arch/arm/kernel/head-common.S | 5 ++
arch/arm/kernel/head-nommu.S | 1 +
arch/arm/kernel/head.S | 5 +-
arch/arm/kernel/process.c | 8 +--
arch/arm/kernel/smp.c | 13 ++++-
arch/arm/mm/proc-macros.S | 3 +-
scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 27 +++--------
20 files changed, 173 insertions(+), 51 deletions(-)
create mode 100644 arch/arm/include/asm/current.h
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
@ 2021-09-13 10:39 ` Ard Biesheuvel
2021-09-13 15:40 ` Kees Cook
2021-09-14 22:04 ` Linus Walleij
2021-09-13 10:39 ` [PATCH v4 2/5] ARM: smp: Pass task to secondary_start_kernel Ard Biesheuvel
` (6 subsequent siblings)
7 siblings, 2 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-13 10:39 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Ard Biesheuvel, Keith Packard, Russell King, Kees Cook,
Arnd Bergmann, Linus Walleij
We will be enabling THREAD_INFO_IN_TASK support for ARM, which means
that we can no longer load the stack canary value by masking the stack
pointer and taking the copy that lives in thread_info. Instead, we will
be able to load it from the task_struct directly, by using the TPIDRURO
register which will hold the current task pointer when
THREAD_INFO_IN_TASK is in effect. This is much more straight-forward,
and allows us to declutter this code a bit while at it.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/arm/Kconfig | 2 +-
arch/arm/Makefile | 5 +---
arch/arm/include/asm/stackprotector.h | 2 --
arch/arm/include/asm/thread_info.h | 3 ---
arch/arm/kernel/asm-offsets.c | 4 ---
arch/arm/kernel/process.c | 4 ---
scripts/gcc-plugins/arm_ssp_per_task_plugin.c | 27 +++++---------------
7 files changed, 8 insertions(+), 39 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index fc196421b2ce..ff3e64ae959e 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1600,7 +1600,7 @@ config XEN
config STACKPROTECTOR_PER_TASK
bool "Use a unique stack canary value for each task"
- depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
+ depends on GCC_PLUGINS && STACKPROTECTOR && THREAD_INFO_IN_TASK && !XIP_DEFLATED_DATA
select GCC_PLUGIN_ARM_SSP_PER_TASK
default y
help
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 847c31e7c368..b46e673a0ebe 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -273,11 +273,8 @@ ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
prepare: stack_protector_prepare
stack_protector_prepare: prepare0
$(eval SSP_PLUGIN_CFLAGS := \
- -fplugin-arg-arm_ssp_per_task_plugin-tso=$(shell \
- awk '{if ($$2 == "THREAD_SZ_ORDER") print $$3;}'\
- include/generated/asm-offsets.h) \
-fplugin-arg-arm_ssp_per_task_plugin-offset=$(shell \
- awk '{if ($$2 == "TI_STACK_CANARY") print $$3;}'\
+ awk '{if ($$2 == "TSK_STACK_CANARY") print $$3;}'\
include/generated/asm-offsets.h))
$(eval KBUILD_CFLAGS += $(SSP_PLUGIN_CFLAGS))
$(eval GCC_PLUGINS_CFLAGS += $(SSP_PLUGIN_CFLAGS))
diff --git a/arch/arm/include/asm/stackprotector.h b/arch/arm/include/asm/stackprotector.h
index 72a20c3a0a90..088d03161be5 100644
--- a/arch/arm/include/asm/stackprotector.h
+++ b/arch/arm/include/asm/stackprotector.h
@@ -39,8 +39,6 @@ static __always_inline void boot_init_stack_canary(void)
current->stack_canary = canary;
#ifndef CONFIG_STACKPROTECTOR_PER_TASK
__stack_chk_guard = current->stack_canary;
-#else
- current_thread_info()->stack_canary = current->stack_canary;
#endif
}
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 9a18da3e10cc..f0cacc733231 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -55,9 +55,6 @@ struct thread_info {
struct task_struct *task; /* main task structure */
__u32 cpu; /* cpu */
__u32 cpu_domain; /* cpu domain */
-#ifdef CONFIG_STACKPROTECTOR_PER_TASK
- unsigned long stack_canary;
-#endif
struct cpu_context_save cpu_context; /* cpu context */
__u32 abi_syscall; /* ABI type and syscall nr */
__u8 used_cp[16]; /* thread used copro */
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index a646a3f6440f..9c864ee76107 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -63,10 +63,6 @@ int main(void)
#ifdef CONFIG_IWMMXT
DEFINE(TI_IWMMXT_STATE, offsetof(struct thread_info, fpstate.iwmmxt));
#endif
-#ifdef CONFIG_STACKPROTECTOR_PER_TASK
- DEFINE(TI_STACK_CANARY, offsetof(struct thread_info, stack_canary));
-#endif
- DEFINE(THREAD_SZ_ORDER, THREAD_SIZE_ORDER);
BLANK();
DEFINE(S_R0, offsetof(struct pt_regs, ARM_r0));
DEFINE(S_R1, offsetof(struct pt_regs, ARM_r1));
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 0e2d3051741e..cd73c216b272 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -269,10 +269,6 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
thread_notify(THREAD_NOTIFY_COPY, thread);
-#ifdef CONFIG_STACKPROTECTOR_PER_TASK
- thread->stack_canary = p->stack_canary;
-#endif
-
return 0;
}
diff --git a/scripts/gcc-plugins/arm_ssp_per_task_plugin.c b/scripts/gcc-plugins/arm_ssp_per_task_plugin.c
index 8c1af9bdcb1b..7328d037f975 100644
--- a/scripts/gcc-plugins/arm_ssp_per_task_plugin.c
+++ b/scripts/gcc-plugins/arm_ssp_per_task_plugin.c
@@ -4,7 +4,7 @@
__visible int plugin_is_GPL_compatible;
-static unsigned int sp_mask, canary_offset;
+static unsigned int canary_offset;
static unsigned int arm_pertask_ssp_rtl_execute(void)
{
@@ -13,7 +13,7 @@ static unsigned int arm_pertask_ssp_rtl_execute(void)
for (insn = get_insns(); insn; insn = NEXT_INSN(insn)) {
const char *sym;
rtx body;
- rtx mask, masked_sp;
+ rtx current;
/*
* Find a SET insn involving a SYMBOL_REF to __stack_chk_guard
@@ -30,19 +30,13 @@ static unsigned int arm_pertask_ssp_rtl_execute(void)
/*
* Replace the source of the SET insn with an expression that
- * produces the address of the copy of the stack canary value
- * stored in struct thread_info
+ * produces the address of the current task's stack canary value
*/
- mask = GEN_INT(sext_hwi(sp_mask, GET_MODE_PRECISION(Pmode)));
- masked_sp = gen_reg_rtx(Pmode);
+ current = gen_reg_rtx(Pmode);
- emit_insn_before(gen_rtx_set(masked_sp,
- gen_rtx_AND(Pmode,
- stack_pointer_rtx,
- mask)),
- insn);
+ emit_insn_before(gen_load_tp_hard(current), insn);
- SET_SRC(body) = gen_rtx_PLUS(Pmode, masked_sp,
+ SET_SRC(body) = gen_rtx_PLUS(Pmode, current,
GEN_INT(canary_offset));
}
return 0;
@@ -72,7 +66,6 @@ __visible int plugin_init(struct plugin_name_args *plugin_info,
const char * const plugin_name = plugin_info->base_name;
const int argc = plugin_info->argc;
const struct plugin_argument *argv = plugin_info->argv;
- int tso = 0;
int i;
if (!plugin_default_version_check(version, &gcc_version)) {
@@ -91,11 +84,6 @@ __visible int plugin_init(struct plugin_name_args *plugin_info,
return 1;
}
- if (!strcmp(argv[i].key, "tso")) {
- tso = atoi(argv[i].value);
- continue;
- }
-
if (!strcmp(argv[i].key, "offset")) {
canary_offset = atoi(argv[i].value);
continue;
@@ -105,9 +93,6 @@ __visible int plugin_init(struct plugin_name_args *plugin_info,
return 1;
}
- /* create the mask that produces the base of the stack */
- sp_mask = ~((1U << (12 + tso)) - 1);
-
PASS_INFO(arm_pertask_ssp_rtl, "expand", 1, PASS_POS_INSERT_AFTER);
register_callback(plugin_info->base_name, PLUGIN_PASS_MANAGER_SETUP,
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 2/5] ARM: smp: Pass task to secondary_start_kernel
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
2021-09-13 10:39 ` [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support Ard Biesheuvel
@ 2021-09-13 10:39 ` Ard Biesheuvel
2021-09-13 23:25 ` Linus Walleij
2021-09-13 10:39 ` [PATCH v4 3/5] ARM: smp: Free up the TLS register while running in the kernel Ard Biesheuvel
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-13 10:39 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Ard Biesheuvel, Keith Packard, Russell King, Kees Cook,
Arnd Bergmann, Linus Walleij
From: Keith Packard <keithpac@amazon.com>
This avoids needing to compute the task pointer in this function, which
will no longer be possible once we move thread_info off the stack.
Signed-off-by: Keith Packard <keithpac@amazon.com>
Signed-off-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 | 5 +++--
arch/arm/kernel/smp.c | 8 ++++++--
4 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 5d508f5d56c4..f16cbbd5cda4 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(struct task_struct *task);
/*
@@ -61,6 +61,7 @@ struct secondary_data {
};
unsigned long swapper_pg_dir;
void *stack;
+ 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 0fc814bbc34b..fadfee9e2b45 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 sp, [r7, #12] @ set up the stack pointer
+ ldr r0, [r7, #16] @ set up task pointer
mov fp, #0
b secondary_start_kernel
ENDPROC(secondary_startup)
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 29070eb8df7d..fa44e2d9f0b0 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -424,8 +424,9 @@ ENDPROC(secondary_startup)
ENDPROC(secondary_startup_arm)
ENTRY(__secondary_switched)
- ldr_l r7, secondary_data + 12 @ get secondary_data.stack
- mov sp, r7
+ adr_l r7, secondary_data + 12 @ get secondary_data.stack
+ ldr sp, [r7]
+ ldr r0, [r7, #4] @ get secondary_data.task
mov fp, #0
b secondary_start_kernel
ENDPROC(__secondary_switched)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 842427ff2b3c..8979d548ec17 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.task = idle;
sync_cache_w(&secondary_data);
/*
@@ -375,9 +376,12 @@ void arch_cpu_idle_dead(void)
*/
__asm__("mov sp, %0\n"
" mov fp, #0\n"
+ " mov r0, %1\n"
" b secondary_start_kernel"
:
- : "r" (task_stack_page(current) + THREAD_SIZE - 8));
+ : "r" (task_stack_page(current) + THREAD_SIZE - 8),
+ "r" (current)
+ : "r0");
}
#endif /* CONFIG_HOTPLUG_CPU */
@@ -400,7 +404,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(void)
+asmlinkage void secondary_start_kernel(struct task_struct *task)
{
struct mm_struct *mm = &init_mm;
unsigned int cpu;
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 3/5] ARM: smp: Free up the TLS register while running in the kernel
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
2021-09-13 10:39 ` [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support Ard Biesheuvel
2021-09-13 10:39 ` [PATCH v4 2/5] ARM: smp: Pass task to secondary_start_kernel Ard Biesheuvel
@ 2021-09-13 10:39 ` Ard Biesheuvel
2021-09-13 10:40 ` [PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available Ard Biesheuvel
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-13 10:39 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Ard Biesheuvel, Keith Packard, Russell King, Kees Cook,
Arnd Bergmann, Linus Walleij
To prepare for a subsequent patch that stores the current task pointer
in the user space TLS register while running in the kernel, modify the
set_tls and switch_tls routines not to touch the register directly, and
update the return to user space code to load the correct value.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/arm/include/asm/tls.h | 10 +++++++---
| 8 ++++++++
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/arch/arm/include/asm/tls.h b/arch/arm/include/asm/tls.h
index 5a66c3b13c92..c3296499176c 100644
--- a/arch/arm/include/asm/tls.h
+++ b/arch/arm/include/asm/tls.h
@@ -12,8 +12,8 @@
.macro switch_tls_v6k, base, tp, tpuser, tmp1, tmp2
mrc p15, 0, \tmp2, c13, c0, 2 @ get the user r/w register
- mcr p15, 0, \tp, c13, c0, 3 @ set TLS register
- mcr p15, 0, \tpuser, c13, c0, 2 @ and the user r/w register
+ @ TLS register update is deferred until return to user space
+ mcr p15, 0, \tpuser, c13, c0, 2 @ set the user r/w register
str \tmp2, [\base, #TI_TP_VALUE + 4] @ save it
.endm
@@ -38,18 +38,22 @@
#ifdef CONFIG_TLS_REG_EMUL
#define tls_emu 1
#define has_tls_reg 1
+#define defer_tls_reg_update 0
#define switch_tls switch_tls_none
#elif defined(CONFIG_CPU_V6)
#define tls_emu 0
#define has_tls_reg (elf_hwcap & HWCAP_TLS)
+#define defer_tls_reg_update 0
#define switch_tls switch_tls_v6
#elif defined(CONFIG_CPU_32v6K)
#define tls_emu 0
#define has_tls_reg 1
+#define defer_tls_reg_update 1
#define switch_tls switch_tls_v6k
#else
#define tls_emu 0
#define has_tls_reg 0
+#define defer_tls_reg_update 0
#define switch_tls switch_tls_software
#endif
@@ -77,7 +81,7 @@ static inline void set_tls(unsigned long val)
*/
barrier();
- if (!tls_emu) {
+ if (!tls_emu && !defer_tls_reg_update) {
if (has_tls_reg) {
asm("mcr p15, 0, %0, c13, c0, 3"
: : "r" (val));
--git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
index 40db0f9188b6..d5845cb820f3 100644
--- a/arch/arm/kernel/entry-header.S
+++ b/arch/arm/kernel/entry-header.S
@@ -292,6 +292,14 @@
.macro restore_user_regs, fast = 0, offset = 0
+#ifdef CONFIG_CPU_32v6K
+ @ The TLS register update is deferred until return to user space so we
+ @ can use it for other things while running in the kernel
+ get_thread_info r1
+ ldr r1, [r1, #TI_TP_VALUE]
+ mcr p15, 0, r1, c13, c0, 3 @ set TLS register
+#endif
+
uaccess_enable r1, isb=0
#ifndef CONFIG_THUMB2_KERNEL
@ ARM mode restore
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
` (2 preceding siblings ...)
2021-09-13 10:39 ` [PATCH v4 3/5] ARM: smp: Free up the TLS register while running in the kernel Ard Biesheuvel
@ 2021-09-13 10:40 ` Ard Biesheuvel
2021-09-13 11:22 ` Russell King (Oracle)
2021-09-13 10:40 ` [PATCH v4 5/5] ARM: smp: Enable THREAD_INFO_IN_TASK Ard Biesheuvel
` (3 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-13 10:40 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Ard Biesheuvel, Keith Packard, Russell King, Kees Cook,
Arnd Bergmann, Linus Walleij
Now that the user space TLS register is assigned on every return to user
space, we can use it to keep the 'current' pointer while running in the
kernel. This removes the need to access it via thread_info, which is
located at the base of the stack, but will be moved out of there in a
subsequent patch.
Use the __builtin_thread_pointer() helper when available - this will
help GCC understand that reloading the value within the same function is
not necessary, even when using the per-task stack protector (which also
generates accesses via the TLS register). For example, the generated
code below loads TPIDRURO only once, and uses it to access both the
stack canary and the preempt_count fields.
<do_one_initcall>:
e92d 41f0 stmdb sp!, {r4, r5, r6, r7, r8, lr}
ee1d 4f70 mrc 15, 0, r4, cr13, cr0, {3}
4606 mov r6, r0
b094 sub sp, #80 ; 0x50
f8d4 34e8 ldr.w r3, [r4, #1256] ; 0x4e8 <- stack canary
9313 str r3, [sp, #76] ; 0x4c
f8d4 8004 ldr.w r8, [r4, #4] <- preempt count
Co-developed-by: Keith Packard <keithpac@amazon.com>
Signed-off-by: Keith Packard <keithpac@amazon.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/arm/Kconfig | 5 ++
arch/arm/include/asm/assembler.h | 24 +++++++++
arch/arm/include/asm/current.h | 51 ++++++++++++++++++++
arch/arm/include/asm/switch_to.h | 2 +
arch/arm/include/asm/thread_info.h | 2 +
arch/arm/kernel/entry-armv.S | 9 +++-
arch/arm/kernel/entry-common.S | 1 +
arch/arm/kernel/head-common.S | 5 ++
arch/arm/kernel/process.c | 4 ++
arch/arm/kernel/smp.c | 2 +
arch/arm/mm/proc-macros.S | 3 +-
11 files changed, 105 insertions(+), 3 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index ff3e64ae959e..24c756299d48 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1157,6 +1157,11 @@ config SMP_ON_UP
If you don't know what to do here, say Y.
+
+config CURRENT_POINTER_IN_TPIDRURO
+ def_bool y
+ depends on SMP && (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..c1551dee28be 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -199,6 +199,30 @@
.endm
.endr
+ .macro get_current, rd
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+ mrc p15, 0, \rd, c13, c0, 3 @ get TPIDRURO register
+#else
+ get_thread_info \rd
+ ldr \rd, [\rd, #TI_TASK]
+#endif
+ .endm
+
+ .macro set_current, rn
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+ mcr p15, 0, \rn, c13, c0, 3 @ set TPIDRURO register
+#endif
+ .endm
+
+ .macro reload_current, t1:req, t2:req
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+ adr_l \t1, __entry_task @ get __entry_task base address
+ mrc p15, 0, \t2, c13, c0, 4 @ get per-CPU offset
+ ldr \t1, [\t1, \t2] @ load variable
+ mcr p15, 0, \t1, c13, c0, 3 @ store in TPIDRURO
+#endif
+ .endm
+
/*
* Get current thread_info.
*/
diff --git a/arch/arm/include/asm/current.h b/arch/arm/include/asm/current.h
new file mode 100644
index 000000000000..adce38a4c798
--- /dev/null
+++ b/arch/arm/include/asm/current.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2021 Keith Packard <keithp@keithp.com>
+ * Copyright (c) 2021 Google, LLC <ardb@kernel.org>
+ */
+
+#ifndef _ASM_ARM_CURRENT_H
+#define _ASM_ARM_CURRENT_H
+
+#ifndef __ASSEMBLY__
+
+struct task_struct;
+
+static inline void set_current(struct task_struct *cur)
+{
+ if (!IS_ENABLED(CONFIG_CURRENT_POINTER_IN_TPIDRURO))
+ return;
+
+ /* Set TPIDRURO */
+ asm("mcr p15, 0, %0, c13, c0, 3" :: "r"(cur) : "memory");
+}
+
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+
+static inline struct task_struct *get_current(void)
+{
+ struct task_struct *cur;
+
+#if defined(CONFIG_CC_IS_GCC) && __has_builtin(__builtin_thread_pointer)
+ /*
+ * Use the __builtin helper when available - this results in better
+ * code, especially in combination with the per-task stack protector,
+ * as GCC will recognize that it needs to load the TLS register only
+ * once in every function. Clang would generate a call to
+ * __aeabi_read_tp() here, and does not support the plugin anyway.
+ */
+ cur = __builtin_thread_pointer();
+#else
+ asm("mrc p15, 0, %0, c13, c0, 3" : "=r"(cur));
+#endif
+ return cur;
+}
+
+#define current get_current()
+#else
+#include <asm-generic/current.h>
+#endif /* CONFIG_CURRENT_POINTER_IN_TPIDRURO */
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_ARM_CURRENT_H */
diff --git a/arch/arm/include/asm/switch_to.h b/arch/arm/include/asm/switch_to.h
index 007d8fea7157..61e4a3c4ca6e 100644
--- a/arch/arm/include/asm/switch_to.h
+++ b/arch/arm/include/asm/switch_to.h
@@ -26,6 +26,8 @@ extern struct task_struct *__switch_to(struct task_struct *, struct thread_info
#define switch_to(prev,next,last) \
do { \
__complete_pending_tlbi(); \
+ if (IS_ENABLED(CONFIG_CURRENT_POINTER_IN_TPIDRURO)) \
+ __this_cpu_write(__entry_task, next); \
last = __switch_to(prev,task_thread_info(prev), task_thread_info(next)); \
} while (0)
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index f0cacc733231..76b6fbd5540c 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -29,6 +29,8 @@
struct task_struct;
+DECLARE_PER_CPU(struct task_struct *, __entry_task);
+
#include <asm/types.h>
struct cpu_context_save {
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 241b73d64df7..aedcaa329fc9 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -384,6 +384,8 @@ ENDPROC(__fiq_abt)
ATRAP( teq r8, r7)
ATRAP( mcrne p15, 0, r8, c1, c0, 0)
+ reload_current r7, r8
+
@
@ Clear FP to mark the first stack frame
@
@@ -767,13 +769,18 @@ ENTRY(__switch_to)
mcr p15, 0, r6, c3, c0, 0 @ Set domain register
#endif
mov r5, r0
- add r4, r2, #TI_CPU_SAVE
+ mov r4, r2
ldr r0, =thread_notify_head
mov r1, #THREAD_NOTIFY_SWITCH
bl atomic_notifier_call_chain
#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
str r7, [r8]
#endif
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+ ldr ip, [r4, #TI_TASK]
+ set_current ip
+#endif
+ add r4, r4, #TI_CPU_SAVE
THUMB( mov ip, r4 )
mov r0, r5
ARM( ldmia r4, {r4 - sl, fp, sp, pc} ) @ Load all regs saved previously
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index d9c99db50243..ac86c34682bb 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -170,6 +170,7 @@ ENTRY(vector_swi)
str saved_psr, [sp, #S_PSR] @ Save CPSR
str r0, [sp, #S_OLD_R0] @ Save OLD_R0
#endif
+ reload_current r10, ip
zero_fp
alignment_trap r10, ip, __cr_alignment
asm_trace_hardirqs_on save=0
diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
index 29b2eda136bb..da18e0a17dc2 100644
--- a/arch/arm/kernel/head-common.S
+++ b/arch/arm/kernel/head-common.S
@@ -105,6 +105,11 @@ __mmap_switched:
mov r1, #0
bl __memset @ clear .bss
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+ adr_l r0, init_task @ get swapper task_struct
+ set_current r0
+#endif
+
ldmia r4, {r0, r1, r2, r3}
str r9, [r0] @ Save processor ID
str r7, [r1] @ Save machine type
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index cd73c216b272..30428d756515 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -36,6 +36,10 @@
#include "signal.h"
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+DEFINE_PER_CPU(struct task_struct *, __entry_task);
+#endif
+
#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
#include <linux/stackprotector.h>
unsigned long __stack_chk_guard __read_mostly;
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 8979d548ec17..97ee6b1567e9 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -409,6 +409,8 @@ asmlinkage void secondary_start_kernel(struct task_struct *task)
struct mm_struct *mm = &init_mm;
unsigned int cpu;
+ set_current(task);
+
secondary_biglittle_init();
/*
diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
index e2c743aa2eb2..d48ba99d739c 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -30,8 +30,7 @@
* act_mm - get current->active_mm
*/
.macro act_mm, rd
- get_thread_info \rd
- ldr \rd, [\rd, #TI_TASK]
+ get_current \rd
.if (TSK_ACTIVE_MM > IMM12_MASK)
add \rd, \rd, #TSK_ACTIVE_MM & ~IMM12_MASK
.endif
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 5/5] ARM: smp: Enable THREAD_INFO_IN_TASK
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
` (3 preceding siblings ...)
2021-09-13 10:40 ` [PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available Ard Biesheuvel
@ 2021-09-13 10:40 ` Ard Biesheuvel
2021-09-13 11:23 ` [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Russell King (Oracle)
` (2 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-13 10:40 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Ard Biesheuvel, Keith Packard, Russell King, Kees Cook,
Arnd Bergmann, Linus Walleij
Now that we no longer rely on thread_info living at the base of the task
stack to be able to access the 'current' pointer, we can wire up the
generic support for moving thread_info into the task struct itself.
Note that this requires us to update the cpu field in thread_info
explicitly, now that the core code no longer does so. Ideally, we would
switch the percpu code to access the cpu field in task_struct instead,
but this unleashes #include circular dependency hell.
Co-developed-by: Keith Packard <keithpac@amazon.com>
Signed-off-by: Keith Packard <keithpac@amazon.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/arm/Kconfig | 1 +
arch/arm/include/asm/assembler.h | 5 +++++
arch/arm/include/asm/switch_to.h | 14 ++++++++++++++
arch/arm/include/asm/thread_info.h | 10 +++++++++-
arch/arm/kernel/asm-offsets.c | 2 ++
arch/arm/kernel/entry-armv.S | 3 +--
arch/arm/kernel/smp.c | 3 +++
7 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 24c756299d48..0aa7192b08ea 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -125,6 +125,7 @@ config ARM
select PERF_USE_VMALLOC
select RTC_LIB
select SYS_SUPPORTS_APM_EMULATION
+ select THREAD_INFO_IN_TASK if CURRENT_POINTER_IN_TPIDRURO
select TRACE_IRQFLAGS_SUPPORT if !CPU_V7M
# Above selects are sorted alphabetically; please add new ones
# according to that. Thanks.
diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index c1551dee28be..7d23d4bb2168 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -227,10 +227,15 @@
* Get current thread_info.
*/
.macro get_thread_info, rd
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+ /* thread_info is the first member of struct task_struct */
+ get_current \rd
+#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/switch_to.h b/arch/arm/include/asm/switch_to.h
index 61e4a3c4ca6e..db2be1f6550d 100644
--- a/arch/arm/include/asm/switch_to.h
+++ b/arch/arm/include/asm/switch_to.h
@@ -23,9 +23,23 @@
*/
extern struct task_struct *__switch_to(struct task_struct *, struct thread_info *, struct thread_info *);
+static inline void set_ti_cpu(struct task_struct *p)
+{
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+ /*
+ * The core code no longer maintains the thread_info::cpu field once
+ * CONFIG_THREAD_INFO_IN_TASK is in effect, but we rely on it for
+ * raw_smp_processor_id(), which cannot access struct task_struct*
+ * directly for reasons of circular #inclusion hell.
+ */
+ task_thread_info(p)->cpu = p->cpu;
+#endif
+}
+
#define switch_to(prev,next,last) \
do { \
__complete_pending_tlbi(); \
+ set_ti_cpu(next); \
if (IS_ENABLED(CONFIG_CURRENT_POINTER_IN_TPIDRURO)) \
__this_cpu_write(__entry_task, next); \
last = __switch_to(prev,task_thread_info(prev), task_thread_info(next)); \
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 76b6fbd5540c..787511396f3f 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -54,7 +54,9 @@ struct cpu_context_save {
struct thread_info {
unsigned long flags; /* low level flags */
int preempt_count; /* 0 => preemptable, <0 => bug */
+#ifndef CONFIG_THREAD_INFO_IN_TASK
struct task_struct *task; /* main task structure */
+#endif
__u32 cpu; /* cpu */
__u32 cpu_domain; /* cpu domain */
struct cpu_context_save cpu_context; /* cpu context */
@@ -70,11 +72,16 @@ struct thread_info {
#define INIT_THREAD_INFO(tsk) \
{ \
- .task = &tsk, \
+ INIT_THREAD_INFO_TASK(tsk) \
.flags = 0, \
.preempt_count = INIT_PREEMPT_COUNT, \
}
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+#define INIT_THREAD_INFO_TASK(tsk)
+#else
+#define INIT_THREAD_INFO_TASK(tsk) .task = &(tsk),
+
/*
* how to get the thread information struct from C
*/
@@ -85,6 +92,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 9c864ee76107..645845e4982a 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -43,7 +43,9 @@ int main(void)
BLANK();
DEFINE(TI_FLAGS, offsetof(struct thread_info, flags));
DEFINE(TI_PREEMPT, offsetof(struct thread_info, preempt_count));
+#ifndef CONFIG_THREAD_INFO_IN_TASK
DEFINE(TI_TASK, offsetof(struct thread_info, task));
+#endif
DEFINE(TI_CPU, offsetof(struct thread_info, cpu));
DEFINE(TI_CPU_DOMAIN, offsetof(struct thread_info, cpu_domain));
DEFINE(TI_CPU_SAVE, offsetof(struct thread_info, cpu_context));
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index aedcaa329fc9..75580528f267 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -777,8 +777,7 @@ ENTRY(__switch_to)
str r7, [r8]
#endif
#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
- ldr ip, [r4, #TI_TASK]
- set_current ip
+ set_current r4
#endif
add r4, r4, #TI_CPU_SAVE
THUMB( mov ip, r4 )
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 97ee6b1567e9..cde5b6d8bac5 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -154,6 +154,9 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
secondary_data.swapper_pg_dir = get_arch_pgd(swapper_pg_dir);
#endif
secondary_data.task = idle;
+ if (IS_ENABLED(CONFIG_THREAD_INFO_IN_TASK))
+ task_thread_info(idle)->cpu = cpu;
+
sync_cache_w(&secondary_data);
/*
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available
2021-09-13 10:40 ` [PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available Ard Biesheuvel
@ 2021-09-13 11:22 ` Russell King (Oracle)
2021-09-13 12:52 ` Ard Biesheuvel
0 siblings, 1 reply; 20+ messages in thread
From: Russell King (Oracle) @ 2021-09-13 11:22 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-arm-kernel, Keith Packard, Kees Cook, Arnd Bergmann, Linus Walleij
On Mon, Sep 13, 2021 at 12:40:00PM +0200, Ard Biesheuvel wrote:
> @@ -767,13 +769,18 @@ ENTRY(__switch_to)
> mcr p15, 0, r6, c3, c0, 0 @ Set domain register
> #endif
> mov r5, r0
> - add r4, r2, #TI_CPU_SAVE
> + mov r4, r2
> ldr r0, =thread_notify_head
> mov r1, #THREAD_NOTIFY_SWITCH
> bl atomic_notifier_call_chain
> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
> str r7, [r8]
> #endif
> +#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
> + ldr ip, [r4, #TI_TASK]
> + set_current ip
> +#endif
> + add r4, r4, #TI_CPU_SAVE
> THUMB( mov ip, r4 )
> mov r0, r5
> ARM( ldmia r4, {r4 - sl, fp, sp, pc} ) @ Load all regs saved previously
I would like to keep this as optimal as possible, and the setting of
TPIDRURO to be as close to the stack pointer change too, so I'd
suggest a slightly different approach:
switch_tls r1, r4, r5, r3, r7
+#if (defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)) || \
+ defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
+ ldr r9, [r2, #TI_TASK]
+#endif
#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
- ldr r7, [r2, #TI_TASK]
ldr r8, =__stack_chk_guard
.if (TSK_STACK_CANARY > IMM12_MASK)
- add r7, r7, #TSK_STACK_CANARY & ~IMM12_MASK
- .endif
+ add r7, r9, #TSK_STACK_CANARY & ~IMM12_MASK
ldr r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
+ .else
+ ldr r7, [r9, #TSK_STACK_CANARY & IMM12_MASK]
+ .endif
#endif
...
mov r0, r5
+#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
+ set_current r9
+#endif
ARM( ldmia r4, {r4 - sl, fp, sp, pc} ) @ Load all regs saved previously
It's a slightly more complex change, but one I think we should do.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
` (4 preceding siblings ...)
2021-09-13 10:40 ` [PATCH v4 5/5] ARM: smp: Enable THREAD_INFO_IN_TASK Ard Biesheuvel
@ 2021-09-13 11:23 ` Russell King (Oracle)
2021-09-13 15:40 ` Kees Cook
2021-09-14 9:44 ` Arnd Bergmann
7 siblings, 0 replies; 20+ messages in thread
From: Russell King (Oracle) @ 2021-09-13 11:23 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-arm-kernel, Keith Packard, Kees Cook, Arnd Bergmann, Linus Walleij
On Mon, Sep 13, 2021 at 12:39:56PM +0200, Ard Biesheuvel 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 v4 is a follow-up to Keith's v3 [0], which did not address all
> concerns I raised in response to v2. After collaborating with Keith
> off-list, we decided that I should go ahead and post our joint v4.
>
> Changes since v3:
>
> - Leave the CPU field in thread_info, and keep it in sync at context
> switch time. This is by far the easiest and cleanest way to work
> around the fact that it is infeasible to implement
> raw_smp_processor_id() in terms of task_struct::cpu (for reasons of
> header soup).
>
> - Drop the VFP changes, they are no longer necessary given the previous
> point.
>
> - Drop the change to pass the CPU number to secondary_start_kernel().
> Given that we also need to pass the idle task pointer, which carries
> the CPU number, passing the CPU number directly is redundant.
>
> - Use the TPIDRURO register to carry 'current' while running in the
> kernel, and keep using TPIDRPRW for the per-CPU offset as before. This
> way, there is no need to make any changes to the way the per-CPU offsets
> are programmed. It also avoids the concurrency issues that would
> result from carrying the 'current' pointer in a per-CPU variable.
>
> - Update the per-task stack protector plugin to pull the stack canary
> value directly from the task struct.
Overall, this looks much simpler than Keith's v3 to me, so probably
less likely to run into issues in the future. The only suggestion I
hvae is the comment I've made on patch 4.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available
2021-09-13 11:22 ` Russell King (Oracle)
@ 2021-09-13 12:52 ` Ard Biesheuvel
2021-09-13 13:52 ` Russell King (Oracle)
0 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-13 12:52 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Linux ARM, Keith Packard, Kees Cook, Arnd Bergmann, Linus Walleij
Hi Russell,
Thanks for taking a look.
On Mon, 13 Sept 2021 at 13:22, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Mon, Sep 13, 2021 at 12:40:00PM +0200, Ard Biesheuvel wrote:
> > @@ -767,13 +769,18 @@ ENTRY(__switch_to)
> > mcr p15, 0, r6, c3, c0, 0 @ Set domain register
> > #endif
> > mov r5, r0
> > - add r4, r2, #TI_CPU_SAVE
> > + mov r4, r2
> > ldr r0, =thread_notify_head
> > mov r1, #THREAD_NOTIFY_SWITCH
> > bl atomic_notifier_call_chain
> > #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
> > str r7, [r8]
> > #endif
> > +#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
> > + ldr ip, [r4, #TI_TASK]
> > + set_current ip
> > +#endif
> > + add r4, r4, #TI_CPU_SAVE
> > THUMB( mov ip, r4 )
> > mov r0, r5
> > ARM( ldmia r4, {r4 - sl, fp, sp, pc} ) @ Load all regs saved previously
>
> I would like to keep this as optimal as possible, and the setting of
> TPIDRURO to be as close to the stack pointer change too, so I'd
> suggest a slightly different approach:
>
> switch_tls r1, r4, r5, r3, r7
> +#if (defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)) || \
> + defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
> + ldr r9, [r2, #TI_TASK]
> +#endif
> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
> - ldr r7, [r2, #TI_TASK]
> ldr r8, =__stack_chk_guard
> .if (TSK_STACK_CANARY > IMM12_MASK)
> - add r7, r7, #TSK_STACK_CANARY & ~IMM12_MASK
> - .endif
> + add r7, r9, #TSK_STACK_CANARY & ~IMM12_MASK
> ldr r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
> + .else
> + ldr r7, [r9, #TSK_STACK_CANARY & IMM12_MASK]
> + .endif
> #endif
> ...
> mov r0, r5
> +#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
> + set_current r9
> +#endif
> ARM( ldmia r4, {r4 - sl, fp, sp, pc} ) @ Load all regs saved previously
>
> It's a slightly more complex change, but one I think we should do.
>
Fair enough.
However, the next patch drops the 'ldr ip, [r4, #TI_TASK]' I am adding
here, so I won't be able to use this as-is.
I could just split the ldr/set_current sequence into two, and change
the first one into a mov in the following patch, and move the
set_current to right before where sp is updated.
I.e., in this patch
@@ -762,6 +764,8 @@ ENTRY(__switch_to)
add r7, r7, #TSK_STACK_CANARY & ~IMM12_MASK
.endif
ldr r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
+#elif defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
+ ldr r7, [r2, #TI_TASK]
#endif
#ifdef CONFIG_CPU_USE_DOMAINS
mcr p15, 0, r6, c3, c0, 0 @ Set domain register
@@ -776,6 +780,7 @@ ENTRY(__switch_to)
#endif
THUMB( mov ip, r4 )
mov r0, r5
+ set_current r7
ARM( ldmia r4, {r4 - sl, fp, sp, pc} )
THUMB( ldmia ip!, {r4 - sl, fp} )
THUMB( ldr sp, [ip], #4 )
and in the next one
@@ -765,7 +765,7 @@ ENTRY(__switch_to)
.endif
ldr r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
#elif defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
- ldr r7, [r2, #TI_TASK]
+ mov r7, r2 @ Preserve 'next'
#endif
#ifdef CONFIG_CPU_USE_DOMAINS
mcr p15, 0, r6, c3, c0, 0 @ Set domain register
Would that work for you?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available
2021-09-13 12:52 ` Ard Biesheuvel
@ 2021-09-13 13:52 ` Russell King (Oracle)
0 siblings, 0 replies; 20+ messages in thread
From: Russell King (Oracle) @ 2021-09-13 13:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux ARM, Keith Packard, Kees Cook, Arnd Bergmann, Linus Walleij
On Mon, Sep 13, 2021 at 02:52:46PM +0200, Ard Biesheuvel wrote:
> Hi Russell,
>
> Thanks for taking a look.
>
> However, the next patch drops the 'ldr ip, [r4, #TI_TASK]' I am adding
> here, so I won't be able to use this as-is.
>
> I could just split the ldr/set_current sequence into two, and change
> the first one into a mov in the following patch, and move the
> set_current to right before where sp is updated.
>
> I.e., in this patch
>
> @@ -762,6 +764,8 @@ ENTRY(__switch_to)
> add r7, r7, #TSK_STACK_CANARY & ~IMM12_MASK
> .endif
> ldr r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
> +#elif defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
> + ldr r7, [r2, #TI_TASK]
> #endif
> #ifdef CONFIG_CPU_USE_DOMAINS
> mcr p15, 0, r6, c3, c0, 0 @ Set domain register
> @@ -776,6 +780,7 @@ ENTRY(__switch_to)
> #endif
> THUMB( mov ip, r4 )
> mov r0, r5
> + set_current r7
> ARM( ldmia r4, {r4 - sl, fp, sp, pc} )
> THUMB( ldmia ip!, {r4 - sl, fp} )
> THUMB( ldr sp, [ip], #4 )
>
> and in the next one
>
> @@ -765,7 +765,7 @@ ENTRY(__switch_to)
> .endif
> ldr r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
> #elif defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
> - ldr r7, [r2, #TI_TASK]
> + mov r7, r2 @ Preserve 'next'
> #endif
> #ifdef CONFIG_CPU_USE_DOMAINS
> mcr p15, 0, r6, c3, c0, 0 @ Set domain register
>
> Would that work for you?
Definitely does! Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
` (5 preceding siblings ...)
2021-09-13 11:23 ` [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Russell King (Oracle)
@ 2021-09-13 15:40 ` Kees Cook
2021-09-14 9:44 ` Arnd Bergmann
7 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2021-09-13 15:40 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-arm-kernel, Keith Packard, Russell King, Arnd Bergmann,
Linus Walleij
On Mon, Sep 13, 2021 at 12:39:56PM +0200, Ard Biesheuvel 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 v4 is a follow-up to Keith's v3 [0], which did not address all
> concerns I raised in response to v2. After collaborating with Keith
> off-list, we decided that I should go ahead and post our joint v4.
Great! Thanks for working through this. :)
--
Kees Cook
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support
2021-09-13 10:39 ` [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support Ard Biesheuvel
@ 2021-09-13 15:40 ` Kees Cook
2021-09-14 22:04 ` Linus Walleij
1 sibling, 0 replies; 20+ messages in thread
From: Kees Cook @ 2021-09-13 15:40 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-arm-kernel, Keith Packard, Russell King, Arnd Bergmann,
Linus Walleij
On Mon, Sep 13, 2021 at 12:39:57PM +0200, Ard Biesheuvel wrote:
> We will be enabling THREAD_INFO_IN_TASK support for ARM, which means
> that we can no longer load the stack canary value by masking the stack
> pointer and taking the copy that lives in thread_info. Instead, we will
> be able to load it from the task_struct directly, by using the TPIDRURO
> register which will hold the current task pointer when
> THREAD_INFO_IN_TASK is in effect. This is much more straight-forward,
> and allows us to declutter this code a bit while at it.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/5] ARM: smp: Pass task to secondary_start_kernel
2021-09-13 10:39 ` [PATCH v4 2/5] ARM: smp: Pass task to secondary_start_kernel Ard Biesheuvel
@ 2021-09-13 23:25 ` Linus Walleij
0 siblings, 0 replies; 20+ messages in thread
From: Linus Walleij @ 2021-09-13 23:25 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux ARM, Keith Packard, Russell King, Kees Cook, Arnd Bergmann
On Mon, Sep 13, 2021 at 12:40 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> From: Keith Packard <keithpac@amazon.com>
>
> This avoids needing to compute the task pointer in this function, which
> will no longer be possible once we move thread_info off the stack.
>
> Signed-off-by: Keith Packard <keithpac@amazon.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Looks good and also easier to understand for me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
` (6 preceding siblings ...)
2021-09-13 15:40 ` Kees Cook
@ 2021-09-14 9:44 ` Arnd Bergmann
2021-09-14 9:50 ` Ard Biesheuvel
7 siblings, 1 reply; 20+ messages in thread
From: Arnd Bergmann @ 2021-09-14 9:44 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux ARM, Keith Packard, Russell King, Kees Cook, Arnd Bergmann,
Linus Walleij
On Mon, Sep 13, 2021 at 12:39 PM Ard Biesheuvel <ardb@kernel.org> 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 v4 is a follow-up to Keith's v3 [0], which did not address all
> concerns I raised in response to v2. After collaborating with Keith
> off-list, we decided that I should go ahead and post our joint v4.
>
> Changes since v3:
>
> - Leave the CPU field in thread_info, and keep it in sync at context
> switch time. This is by far the easiest and cleanest way to work
> around the fact that it is infeasible to implement
> raw_smp_processor_id() in terms of task_struct::cpu (for reasons of
> header soup).
>
> - Drop the VFP changes, they are no longer necessary given the previous
> point.
>
> - Drop the change to pass the CPU number to secondary_start_kernel().
> Given that we also need to pass the idle task pointer, which carries
> the CPU number, passing the CPU number directly is redundant.
>
> - Use the TPIDRURO register to carry 'current' while running in the
> kernel, and keep using TPIDRPRW for the per-CPU offset as before. This
> way, there is no need to make any changes to the way the per-CPU offsets
> are programmed. It also avoids the concurrency issues that would
> result from carrying the 'current' pointer in a per-CPU variable.
>
> - Update the per-task stack protector plugin to pull the stack canary
> value directly from the task struct.
I haven't looked in detail, but it all looks great to me so far.
On a related note, this reminds me of the CONFIG_IRQSTACK series
that was posted last year[1] and probably attempted before that.
I'd have to get back to understanding all the details of that discussion,
but I have some hope that adding irqstacks would become much
easier after your series. Any thoughts on that?
Arnd
[1] https://lore.kernel.org/linux-arm-kernel/1602141333-17822-1-git-send-email-maninder1.s@samsung.com/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK
2021-09-14 9:44 ` Arnd Bergmann
@ 2021-09-14 9:50 ` Ard Biesheuvel
2021-09-14 13:07 ` Arnd Bergmann
0 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-14 9:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linux ARM, Keith Packard, Russell King, Kees Cook, Linus Walleij
On Tue, 14 Sept 2021 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Sep 13, 2021 at 12:39 PM Ard Biesheuvel <ardb@kernel.org> 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 v4 is a follow-up to Keith's v3 [0], which did not address all
> > concerns I raised in response to v2. After collaborating with Keith
> > off-list, we decided that I should go ahead and post our joint v4.
> >
> > Changes since v3:
> >
> > - Leave the CPU field in thread_info, and keep it in sync at context
> > switch time. This is by far the easiest and cleanest way to work
> > around the fact that it is infeasible to implement
> > raw_smp_processor_id() in terms of task_struct::cpu (for reasons of
> > header soup).
> >
> > - Drop the VFP changes, they are no longer necessary given the previous
> > point.
> >
> > - Drop the change to pass the CPU number to secondary_start_kernel().
> > Given that we also need to pass the idle task pointer, which carries
> > the CPU number, passing the CPU number directly is redundant.
> >
> > - Use the TPIDRURO register to carry 'current' while running in the
> > kernel, and keep using TPIDRPRW for the per-CPU offset as before. This
> > way, there is no need to make any changes to the way the per-CPU offsets
> > are programmed. It also avoids the concurrency issues that would
> > result from carrying the 'current' pointer in a per-CPU variable.
> >
> > - Update the per-task stack protector plugin to pull the stack canary
> > value directly from the task struct.
>
> I haven't looked in detail, but it all looks great to me so far.
>
> On a related note, this reminds me of the CONFIG_IRQSTACK series
> that was posted last year[1] and probably attempted before that.
> I'd have to get back to understanding all the details of that discussion,
> but I have some hope that adding irqstacks would become much
> easier after your series. Any thoughts on that?
>
Yes, not having to rely on the stack pointer for thread_info/current
seems to make this a lot easier. And eith mentioned that he is also
interested in vmap'ed stacks for ARM, so I have a suspicion that we
will end up picking up some IRQ stack changes in the process as well.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK
2021-09-14 9:50 ` Ard Biesheuvel
@ 2021-09-14 13:07 ` Arnd Bergmann
2021-09-14 17:10 ` Ard Biesheuvel
0 siblings, 1 reply; 20+ messages in thread
From: Arnd Bergmann @ 2021-09-14 13:07 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arnd Bergmann, Linux ARM, Keith Packard, Russell King, Kees Cook,
Linus Walleij
On Tue, Sep 14, 2021 at 11:50 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> On Tue, 14 Sept 2021 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
>
> > I haven't looked in detail, but it all looks great to me so far.
> >
> > On a related note, this reminds me of the CONFIG_IRQSTACK series
> > that was posted last year[1] and probably attempted before that.
> > I'd have to get back to understanding all the details of that discussion,
> > but I have some hope that adding irqstacks would become much
> > easier after your series. Any thoughts on that?
>
> Yes, not having to rely on the stack pointer for thread_info/current
> seems to make this a lot easier. And eith mentioned that he is also
> interested in vmap'ed stacks for ARM, so I have a suspicion that we
> will end up picking up some IRQ stack changes in the process as well.
Ah, perfect! Adding vmap stack also solves ones of the issues for
the 4G:4G split: since vmalloc/vmap space is always mapped in
kernel mode, copy_{from,to}_user() can then just use memcpy()
with TTBR0 switched to used memory if the kernel buffer is on the
stack, or use an intermediate buffer on the stack if it's not.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK
2021-09-14 13:07 ` Arnd Bergmann
@ 2021-09-14 17:10 ` Ard Biesheuvel
0 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-14 17:10 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linux ARM, Keith Packard, Russell King, Kees Cook, Linus Walleij
On Tue, 14 Sept 2021 at 15:07, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Sep 14, 2021 at 11:50 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Tue, 14 Sept 2021 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > > I haven't looked in detail, but it all looks great to me so far.
> > >
> > > On a related note, this reminds me of the CONFIG_IRQSTACK series
> > > that was posted last year[1] and probably attempted before that.
> > > I'd have to get back to understanding all the details of that discussion,
> > > but I have some hope that adding irqstacks would become much
> > > easier after your series. Any thoughts on that?
> >
> > Yes, not having to rely on the stack pointer for thread_info/current
> > seems to make this a lot easier. And eith mentioned that he is also
> > interested in vmap'ed stacks for ARM, so I have a suspicion that we
> > will end up picking up some IRQ stack changes in the process as well.
>
> Ah, perfect! Adding vmap stack also solves ones of the issues for
> the 4G:4G split: since vmalloc/vmap space is always mapped in
> kernel mode, copy_{from,to}_user() can then just use memcpy()
> with TTBR0 switched to used memory if the kernel buffer is on the
> stack, or use an intermediate buffer on the stack if it's not.
>
Yeah I was wondering how that would interact with the 4/4 split work.
In any case, I had a stab at the basic IRQ stacks support here:
https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-irq-stacks
Haven't looked at the backtrace/unwind part yet.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support
2021-09-13 10:39 ` [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support Ard Biesheuvel
2021-09-13 15:40 ` Kees Cook
@ 2021-09-14 22:04 ` Linus Walleij
2021-09-15 6:37 ` Ard Biesheuvel
1 sibling, 1 reply; 20+ messages in thread
From: Linus Walleij @ 2021-09-14 22:04 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux ARM, Keith Packard, Russell King, Kees Cook, Arnd Bergmann,
Neil Armstrong
Hi Ard,
thanks for this patch series!
On Mon, Sep 13, 2021 at 12:40 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> config STACKPROTECTOR_PER_TASK
> bool "Use a unique stack canary value for each task"
> - depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> + depends on GCC_PLUGINS && STACKPROTECTOR && THREAD_INFO_IN_TASK && !XIP_DEFLATED_DATA
> select GCC_PLUGIN_ARM_SSP_PER_TASK
Am I reading this right that STACKPROTECTOR_PER_TASK
was available for ARMv6 with SMP before this
change and after this change it will only be available for
ARMv7 since THREAD_INFO_IN_TASK which is now a dependency
will only be available for ARMv7 (or ARMv6k)?
I suppose this is a reasonable compromise, as I don't use it or need
it on my ARM PB11MPCore and neither do I think the Oxnas router,
but we should write this explicitly in the commit message.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support
2021-09-14 22:04 ` Linus Walleij
@ 2021-09-15 6:37 ` Ard Biesheuvel
2021-09-15 16:26 ` Kees Cook
0 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2021-09-15 6:37 UTC (permalink / raw)
To: Linus Walleij
Cc: Linux ARM, Keith Packard, Russell King, Kees Cook, Arnd Bergmann,
Neil Armstrong
On Wed, 15 Sept 2021 at 00:04, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Ard,
>
> thanks for this patch series!
>
> On Mon, Sep 13, 2021 at 12:40 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > config STACKPROTECTOR_PER_TASK
> > bool "Use a unique stack canary value for each task"
> > - depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> > + depends on GCC_PLUGINS && STACKPROTECTOR && THREAD_INFO_IN_TASK && !XIP_DEFLATED_DATA
> > select GCC_PLUGIN_ARM_SSP_PER_TASK
>
> Am I reading this right that STACKPROTECTOR_PER_TASK
> was available for ARMv6 with SMP before this
> change and after this change it will only be available for
> ARMv7 since THREAD_INFO_IN_TASK which is now a dependency
> will only be available for ARMv7 (or ARMv6k)?
>
Yes. We could potentially keep the old code path in the plugin as
well, but what I would really prefer to do is get
-mstack-protector-guard=tls implemented in GCC, so we can phase out
the plugin entirely.
> I suppose this is a reasonable compromise, as I don't use it or need
> it on my ARM PB11MPCore and neither do I think the Oxnas router,
> but we should write this explicitly in the commit message.
>
OK, I will add this.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support
2021-09-15 6:37 ` Ard Biesheuvel
@ 2021-09-15 16:26 ` Kees Cook
0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2021-09-15 16:26 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linus Walleij, Linux ARM, Keith Packard, Russell King,
Arnd Bergmann, Neil Armstrong, Qing Zhao
On Wed, Sep 15, 2021 at 08:37:44AM +0200, Ard Biesheuvel wrote:
> On Wed, 15 Sept 2021 at 00:04, Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > Hi Ard,
> >
> > thanks for this patch series!
> >
> > On Mon, Sep 13, 2021 at 12:40 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > > config STACKPROTECTOR_PER_TASK
> > > bool "Use a unique stack canary value for each task"
> > > - depends on GCC_PLUGINS && STACKPROTECTOR && SMP && !XIP_DEFLATED_DATA
> > > + depends on GCC_PLUGINS && STACKPROTECTOR && THREAD_INFO_IN_TASK && !XIP_DEFLATED_DATA
> > > select GCC_PLUGIN_ARM_SSP_PER_TASK
> >
> > Am I reading this right that STACKPROTECTOR_PER_TASK
> > was available for ARMv6 with SMP before this
> > change and after this change it will only be available for
> > ARMv7 since THREAD_INFO_IN_TASK which is now a dependency
> > will only be available for ARMv7 (or ARMv6k)?
> >
>
> Yes. We could potentially keep the old code path in the plugin as
> well, but what I would really prefer to do is get
> -mstack-protector-guard=tls implemented in GCC, so we can phase out
> the plugin entirely.
For tracking, I've added this for arm32 support:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102352
(And updated https://github.com/KSPP/linux/issues/29)
--
Kees Cook
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-09-15 16:28 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-13 10:39 [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Ard Biesheuvel
2021-09-13 10:39 ` [PATCH v4 1/5] gcc-plugins: arm-ssp: Prepare for THREAD_INFO_IN_TASK support Ard Biesheuvel
2021-09-13 15:40 ` Kees Cook
2021-09-14 22:04 ` Linus Walleij
2021-09-15 6:37 ` Ard Biesheuvel
2021-09-15 16:26 ` Kees Cook
2021-09-13 10:39 ` [PATCH v4 2/5] ARM: smp: Pass task to secondary_start_kernel Ard Biesheuvel
2021-09-13 23:25 ` Linus Walleij
2021-09-13 10:39 ` [PATCH v4 3/5] ARM: smp: Free up the TLS register while running in the kernel Ard Biesheuvel
2021-09-13 10:40 ` [PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available Ard Biesheuvel
2021-09-13 11:22 ` Russell King (Oracle)
2021-09-13 12:52 ` Ard Biesheuvel
2021-09-13 13:52 ` Russell King (Oracle)
2021-09-13 10:40 ` [PATCH v4 5/5] ARM: smp: Enable THREAD_INFO_IN_TASK Ard Biesheuvel
2021-09-13 11:23 ` [PATCH v4 0/5] ARM: support THREAD_INFO_IN_TASK Russell King (Oracle)
2021-09-13 15:40 ` Kees Cook
2021-09-14 9:44 ` Arnd Bergmann
2021-09-14 9:50 ` Ard Biesheuvel
2021-09-14 13:07 ` Arnd Bergmann
2021-09-14 17:10 ` Ard Biesheuvel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.