Re: [PATCH v7 30/41] x86/shstk: Handle thread shadow stack
From: Borislav Petkov <bp@alien8.de>
Date: 2023-03-09 14:14:13
Also in:
linux-api, linux-arch, linux-mm, lkml
Subsystem:
the rest, x86 architecture (32-bit and 64-bit) · Maintainers:
Linus Torvalds, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen
On Wed, Mar 08, 2023 at 08:03:17PM +0000, Edgecombe, Rick P wrote: Btw, pls try to trim your replies as I need ot scroll through pages of quoted text to find the response.
Sure. Sometimes people tell me to only ifdef out whole functions to make it easier to read. I suppose in this case it's not hard to see.
Yeah, the less ifdeffery we have, the better.
If the default SSP value logic is too hidden, what about some clearer code and comments, like this?
The problem with this function is that it needs to return three things: * success: ** 0 or ** shadow stack address * failure: due to allocation. How about this below instead? (totally untested ofc):
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index bf703f53fa49..6e323d4e32fc 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c@@ -142,7 +142,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) struct inactive_task_frame *frame; struct fork_frame *fork_frame; struct pt_regs *childregs; - unsigned long shstk_addr = 0; + unsigned long shstk_addr; int ret = 0; childregs = task_pt_regs(p);
@@ -178,10 +178,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) #endif /* Allocate a new shadow stack for pthread if needed */ - ret = shstk_alloc_thread_stack(p, clone_flags, args->stack_size, - &shstk_addr); - if (ret) - return ret; + shstk_addr = shstk_alloc_thread_stack(p, clone_flags, args->stack_size); + if (IS_ERR_VALUE(shstk_addr)) + return PTR_ERR((void *)shstk_addr); fpu_clone(p, clone_flags, args->fn, shstk_addr);
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 13c02747386f..b1668b499e9a 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c@@ -157,8 +157,8 @@ void reset_thread_features(void) current->thread.features_locked = 0; } -int shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags, - unsigned long stack_size, unsigned long *shstk_addr) +unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags, + unsigned long stack_size) { struct thread_shstk *shstk = &tsk->thread.shstk; unsigned long addr, size;
@@ -180,14 +180,12 @@ int shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags, size = adjust_shstk_size(stack_size); addr = alloc_shstk(size); if (IS_ERR_VALUE(addr)) - return PTR_ERR((void *)addr); + return addr; shstk->base = addr; shstk->size = size; - *shstk_addr = addr + size; - - return 0; + return addr + size; } static unsigned long get_user_shstk_addr(void)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette