[RFC] User stack pointer randomisation

[RFC] User stack pointer randomisation

[Franck Bui-Huu]

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'm surely missing something
 obvious and that's the reason this patch is a RFC.

 I tested it and it works fine so far.

 Please try to have a look,

		Franck

 arch/mips/kernel/process.c |   13 +++++++++++++
 include/asm-mips/system.h  |    2 +-
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 6bdfb5a..4f411fa 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -25,6 +25,7 @@
 #include <linux/init.h>
 #include <linux/completion.h>
 #include <linux/kallsyms.h>
+#include <linux/random.h>
 
 #include <asm/bootinfo.h>
 #include <asm/cpu.h>
@@ -460,3 +461,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 at least.
+ */
+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 & ~7;
+}
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: [RFC] 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 at least.
> + */
> +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 & ~7;
> +}
>   

For the 64-bit ABIs (N32 & N64) the stack must be 16 byte aligned.

Nigel

Re: [RFC] User stack pointer randomisation

[Ralf Baechle]

On Thu, Jul 19, 2007 at 09:10:23AM +0200, Franck Bui-Huu wrote:

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

Looks fine to me aside of the issue Nigel raised.

There is a constant defining the ABI-specific alignment in <asm/asm.h>:

#if (_MIPS_SIM == _MIPS_SIM_ABI32)
#define ALSZ    7
#define ALMASK  ~7
#endif
#if (_MIPS_SIM == _MIPS_SIM_NABI32) || (_MIPS_SIM == _MIPS_SIM_ABI64)
#define ALSZ    15
#define ALMASK  ~15
#endif

This will unnecessarily increase the alignment of the stack wasting a few
bytes of memory for O32 binaries running on 64-bit kernels but I'd just
ignore this artefact; the cure would be uglier than the disease ;-)

  Ralf

Re: [RFC] User stack pointer randomisation

[Franck Bui-Huu]

Hi,

On 7/19/07, Nigel Stephens <nigel@mips.com> wrote:
>
> For the 64-bit ABIs (N32 & N64) the stack must be 16 byte aligned.
>

Thanks for this, I'll cook up a new patch.

-- 
               Franck

Re: [RFC] User stack pointer randomisation

[Franck Bui-Huu]

Hi Ralf,

On 7/19/07, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Thu, Jul 19, 2007 at 09:10:23AM +0200, Franck Bui-Huu wrote:
>
> > This patch adds a page size range randomisation to the user
> > stack pointer.
>
> Looks fine to me aside of the issue Nigel raised.
>

I'll fix it.

> There is a constant defining the ABI-specific alignment in <asm/asm.h>:
>

I didn't know about them. ALSZ name is not really self speaking, don't
you think ?

> #if (_MIPS_SIM == _MIPS_SIM_ABI32)
> #define ALSZ    7
> #define ALMASK  ~7
> #endif
> #if (_MIPS_SIM == _MIPS_SIM_NABI32) || (_MIPS_SIM == _MIPS_SIM_ABI64)
> #define ALSZ    15
> #define ALMASK  ~15
> #endif
>

this is weird I would have defined them like this instead:

#if (_MIPS_SIM == _MIPS_SIM_ABI32)
#define ALSZ 8
#elif (_MIPS_SIM == _MIPS_SIM_NABI32) || (_MIPS_SIM == _MIPS_SIM_ABI64)
#define ALSZ 16
#endif

#define ALMASK (~(ALSZ-1))


> This will unnecessarily increase the alignment of the stack wasting a few
> bytes of memory for O32 binaries running on 64-bit kernels but I'd just
> ignore this artefact; the cure would be uglier than the disease ;-)
>

specially that we don't care to waste a couple of bytes in this case...

Thanks
-- 
               Franck

Re: [RFC] User stack pointer randomisation

[Ralf Baechle]

On Thu, Jul 19, 2007 at 01:47:19PM +0200, Franck Bui-Huu wrote:

> this is weird I would have defined them like this instead:
> 
> #if (_MIPS_SIM == _MIPS_SIM_ABI32)
> #define ALSZ 8
> #elif (_MIPS_SIM == _MIPS_SIM_NABI32) || (_MIPS_SIM == _MIPS_SIM_ABI64)
> #define ALSZ 16
> #endif
> 
> #define ALMASK (~(ALSZ-1))

<asm/asm.h> which is fairly similar to it's userspace equivalent <sys/asm.h>
contains definitions which are some sort of pseudo-standard in the MIPS
world, including ALSZ and ALMASK.  If I had choosen them I'd have set
ALSZ to 8 rsp. 16, just like you ...  Anyway, having similar macros makes
porting of assembler code easier.  This also is why <asm/regdef.h> and
<asm/fpregdef.h> are as they are.  RISC/os, IRIX, some of the BSD variants,
even the non-Linux SDE variants for example for baremetal use a similar
set of macros and headers.

  Ralf

Re: [RFC] User stack pointer randomisation

[Franck Bui-Huu]

Ralf Baechle wrote:
> On Thu, Jul 19, 2007 at 01:47:19PM +0200, Franck Bui-Huu wrote:
> 
>> this is weird I would have defined them like this instead:
>>
>> #if (_MIPS_SIM == _MIPS_SIM_ABI32)
>> #define ALSZ 8
>> #elif (_MIPS_SIM == _MIPS_SIM_NABI32) || (_MIPS_SIM == _MIPS_SIM_ABI64)
>> #define ALSZ 16
>> #endif
>>
>> #define ALMASK (~(ALSZ-1))
> 

> <asm/asm.h> which is fairly similar to it's userspace equivalent
> <sys/asm.h> contains definitions which are some sort of
> pseudo-standard in the MIPS world, including ALSZ and ALMASK. If I
> had choosen them I'd have set ALSZ to 8 rsp. 16, just like you ...
> Anyway, having similar macros makes porting of assembler code
> easier.  This also is why <asm/regdef.h> and <asm/fpregdef.h> are as
> they are.  RISC/os, IRIX, some of the BSD variants, even the
> non-Linux SDE variants for example for baremetal use a similar set
> of macros and headers.

Thanks for explanations.

That makes me think that we may have the same alignement issue in
include/asm-mips/ptrace.h header file:

	struct pt_regs {
	#ifdef CONFIG_32BIT
	        /* Pad bytes for argument save space on the stack. */
	        unsigned long pad0[6];
	#endif
	
	        /* Saved main processor registers. */
	        unsigned long regs[32];
	
	        /* Saved special registers. */
	        unsigned long cp0_status;
	        unsigned long hi;
	        unsigned long lo;
	#ifdef CONFIG_CPU_HAS_SMARTMIPS
	        unsigned long acx;
	#endif
	        unsigned long cp0_badvaddr;
	        unsigned long cp0_cause;
	        unsigned long cp0_epc;
	#ifdef CONFIG_MIPS_MT_SMTC
	        unsigned long cp0_tcstatus;
	#endif /* CONFIG_MIPS_MT_SMTC */
	} __attribute__ ((aligned (8)));

Note that the structure is aligned on a 8 bytes boundary which is not
correct for a 64 bit kernel, is it ?

Thanks
		Franck