User stack pointer randomisation
PATCH 1/1 User stack pointer randomisation
From: Franck Bui-Huu <[email protected]>
This patch adds a page size range randomisation to the user stack pointer.
---
Hi Ralf,
This is taken from the x86 architecture. I modified it a bit so the randomisation range is only a page size range. Since the top of the stack is already randomised, I don't see any point to make the range bigger as this is the case in x86 arch.
I tested it and it works fine so far.
Please try to have a look,
Franck
arch/mips/kernel/process.c | 14 ++++++++++++++ include/asm-mips/system.h | 2 +- 2 files changed, 15 insertions(+), 1 deletions(-)
arch/mips/kernel/process.c +14 -0
@@ -25,7 +25,9 @@#include <linux/init.h> #include <linux/completion.h> #include <linux/kallsyms.h> +#include <linux/random.h> +#include <asm/asm.h> #include <asm/bootinfo.h> #include <asm/cpu.h> #include <asm/dsp.h>
@@ -460,3 +462,15 @@ unsigned long get_wchan(struct task_struct *task) out:return pc; } + +/* +* Don't forget that the stack pointer must be aligned on a 8 bytes +* boundary for 32-bits ABI and 16 bytes for 64-bits ABI. +*/ +unsigned long arch_align_stack (unsigned long sp) +{ +if (!(current->personality &ADDR_NO_RANDOMIZE ) && randomize_va_space) + sp -=get_random_int () & ~PAGE_MASK ; + +return sp &ALMASK ; +}
include/asm-mips/system.h +1 -1
@@ -355,6 +355,6 @@ extern int stop_a_enabled;*/ #define __ARCH_WANT_UNLOCKED_CTXSW -#define arch_align_stack (x) (x) +extern unsigned long arch_align_stack (unsigned long sp);#endif /* _ASM_SYSTEM_H */
Signature · 1 line
1.5.2.2
arch/mips/kernel/process.c unsigned long get_wchan(struct task_struct *task) · line 476
Quoted 12 lines · Franck Bui-Huu
arch/mips/kernel/process.c +11
-0+/* +* Don't forget that the stack pointer must be aligned on a 8 bytes +* boundary for 32-bits ABI and 16 bytes for 64-bits ABI. +*/ +unsigned long arch_align_stack (unsigned long sp) +{ +if (!(current->personality &ADDR_NO_RANDOMIZE ) && randomize_va_space) + sp -=get_random_int () & ~PAGE_MASK ; + +return sp &ALMASK ; +}
Hmm, the kernel isn't necessarily built using the same ABI as applications. While this will in fact do the right thing for O32 apps running on 64-bit kernels, it's kind of by accident, and suggests some equivalence which isn't really there. Would it be better to force 16 byte alignment (the maximum alignment required by any ABI) in all cases, rather than relying on the kernel's ALMASK being correct for user applications? Just a thought.
Nigel
Quoted 2 lines · Nigel Stephens
Hmm, the kernel isn't necessarily built using the same ABI as applications. While this will in fact do the right thing for O32 apps
Hey that's true.
Quoted 6 lines
running on 64-bit kernels, it's kind of by accident, and suggests some equivalence which isn't really there. Would it be better to force 16 byte alignment (the maximum alignment required by any ABI) in all cases, rather than relying on the kernel's ALMASK being correct for user applications? Just a thought.
Again I totaly agree, this seems to me cleaner to force 16 bytes alignment rather than using ALMASK which is part of the kernel context.
Let's go for a take #3 if Ralf has no objection.
Thanks
Signature · 1 line
Franck
On Thu, Jul 19, 2007 at 02:04:21PM +0200, Franck Bui-Huu wrote:
Okay, applied.
Ralf
Quoted 4 lines · Ralf Baechle
On Thu, Jul 19, 2007 at 02:04:21PM +0200, Franck Bui-Huu wrote:
Okay, applied.
ouch you were too fast ;)
I think Nigel is right in his last comment.
Do you care to amend this on top of what you applied ?
-- 8< --
arch/mips/kernel/process.c +4 -4
@@ -27,7 +27,6 @@#include <linux/kallsyms.h> #include <linux/random.h> -#include <asm/asm.h> #include <asm/bootinfo.h> #include <asm/cpu.h> #include <asm/dsp.h>
@@ -464,13 +463,14 @@ out: }/* -* Don't forget that the stack pointer must be aligned on a 8 bytes -* boundary for 32-bits ABI and 16 bytes for 64-bits ABI. +* The stack pointer must be aligned on a 8 bytes boundary for 32-bits +* ABI and 16 bytes for 64-bits ABI. To make things simple we force to +* the maximum alignment required by any ABI. */ unsigned long arch_align_stack (unsigned long sp) {if (!(current->personality &ADDR_NO_RANDOMIZE ) && randomize_va_space) sp -=get_random_int () & ~PAGE_MASK ; -return sp &ALMASK ; +return sp & ~0xf ; }
Quoted 1 line · Franck Bui-Huu
if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
actually this condition can be replaced by this simpler one:
if (current->flags & PF_RANDOMIZE)
since PF_RANDOMIZE flag is raised if and only if the old condition is true.
At this point do you prefer a patch to amend these modifications or to replace the old one ?
Sorry for the annoyance.
Signature · 1 line
Franck