[PATCH] User stack pointer randomisation

[PATCH] User stack pointer randomisation

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

This patch adds a page size range randomisation to the user
stack pointer.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---

  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(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 6bdfb5a..42a60b4 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -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;
+}
diff --git a/include/asm-mips/system.h b/include/asm-mips/system.h
index 2908870..0cfb6e1 100644
--- a/include/asm-mips/system.h
+++ b/include/asm-mips/system.h
@@ -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 */
-- 
1.5.2.2

Re: [PATCH] User stack pointer randomisation

[Nigel Stephens]

Franck Bui-Huu wrote:
> +/*
> + * 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

Re: [PATCH] User stack pointer randomisation

[Franck Bui-Huu]

On 7/19/07, Nigel Stephens <nigel@mips.com> wrote:
> 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.

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

Re: [PATCH] User stack pointer randomisation

[Ralf Baechle]

On Thu, Jul 19, 2007 at 02:04:21PM +0200, Franck Bui-Huu wrote:

Okay, applied.

  Ralf

Re: [PATCH] User stack pointer randomisation

[Franck Bui-Huu]

Ralf Baechle wrote:
> 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< --

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 42a60b4..d6b9653 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -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;
 }

Re: [PATCH] User stack pointer randomisation

[Franck Bui-Huu]

On 7/19/07, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>         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.
-- 
               Franck