[RFC PATCH 1/1] MIPS: Enable VDSO randomization.

[RFC PATCH 1/1] MIPS: Enable VDSO randomization.

[Prem Karat]

Based on commit 1091458d09e1a (mmap randomization)

For 32-bit address spaces randomize within a
16MB space, for 64-bit within a 256MB space.

Signed-off-by: Prem Karat <pkarat@mvista.com>
---
 arch/mips/kernel/vdso.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/vdso.c b/arch/mips/kernel/vdso.c
index 0f1af58..b49c705 100644
--- a/arch/mips/kernel/vdso.c
+++ b/arch/mips/kernel/vdso.c
@@ -16,9 +16,11 @@
 #include <linux/elf.h>
 #include <linux/vmalloc.h>
 #include <linux/unistd.h>
+#include <linux/random.h>
 
 #include <asm/vdso.h>
 #include <asm/uasm.h>
+#include <asm/processor.h>
 
 /*
  * Including <asm/unistd.h> would give use the 64-bit syscall numbers ...
@@ -67,7 +69,18 @@ subsys_initcall(init_vdso);
 
 static unsigned long vdso_addr(unsigned long start)
 {
-	return STACK_TOP;
+	unsigned long offset = 0UL;
+
+	if (current->flags & PF_RANDOMIZE) {
+		offset = get_random_int();
+		offset = offset << PAGE_SHIFT;
+		if (TASK_IS_32BIT_ADDR)
+			offset &= 0xfffffful;
+		else
+			offset &= 0xffffffful;
+	}
+
+	return (STACK_TOP + offset);
 }
 
 int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
-- 
1.7.9.5

Re: [RFC PATCH 1/1] MIPS: Enable VDSO randomization.

[Sergei Shtylyov]

Hello.

On 19-04-2014 13:33, Prem Karat wrote:

> Based on commit 1091458d09e1a (mmap randomization)

> For 32-bit address spaces randomize within a
> 16MB space, for 64-bit within a 256MB space.

> Signed-off-by: Prem Karat <pkarat@mvista.com>
[...]

> @@ -67,7 +69,18 @@ subsys_initcall(init_vdso);
>
>   static unsigned long vdso_addr(unsigned long start)
>   {
> -	return STACK_TOP;
> +	unsigned long offset = 0UL;
> +
> +	if (current->flags & PF_RANDOMIZE) {
> +		offset = get_random_int();
> +		offset = offset << PAGE_SHIFT;

    Why not:

		offset <<= PAGE_SHIFT;

> +		if (TASK_IS_32BIT_ADDR)
> +			offset &= 0xfffffful;
> +		else
> +			offset &= 0xffffffful;
> +	}
> +
> +	return (STACK_TOP + offset);

    Parens not needed.

WBR, Sergei

Re: [RFC PATCH 1/1] MIPS: Enable VDSO randomization.

[David Daney]

On 04/19/2014 02:33 AM, Prem Karat wrote:
> Based on commit 1091458d09e1a (mmap randomization)
>
> For 32-bit address spaces randomize within a
> 16MB space, for 64-bit within a 256MB space.
>

How was it tested (i.e. what workload did you run to verify that the 
kernel still functions with this change)?

> Signed-off-by: Prem Karat <pkarat@mvista.com>
> ---
>   arch/mips/kernel/vdso.c |   15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/kernel/vdso.c b/arch/mips/kernel/vdso.c
> index 0f1af58..b49c705 100644
> --- a/arch/mips/kernel/vdso.c
> +++ b/arch/mips/kernel/vdso.c
> @@ -16,9 +16,11 @@
>   #include <linux/elf.h>
>   #include <linux/vmalloc.h>
>   #include <linux/unistd.h>
> +#include <linux/random.h>
>
>   #include <asm/vdso.h>
>   #include <asm/uasm.h>
> +#include <asm/processor.h>
>
>   /*
>    * Including <asm/unistd.h> would give use the 64-bit syscall numbers ...
> @@ -67,7 +69,18 @@ subsys_initcall(init_vdso);
>
>   static unsigned long vdso_addr(unsigned long start)
>   {
> -	return STACK_TOP;
> +	unsigned long offset = 0UL;
> +
> +	if (current->flags & PF_RANDOMIZE) {
> +		offset = get_random_int();
> +		offset = offset << PAGE_SHIFT;
> +		if (TASK_IS_32BIT_ADDR)
> +			offset &= 0xfffffful;
> +		else
> +			offset &= 0xffffffful;
> +	}
> +
> +	return (STACK_TOP + offset);

How can you be sure this address doesn't collide with, or otherwise 
interfere with, the stack?


Also, as mentioned by Sergei, run checkpatch.pl to catch obvious 
stylistic problems before submitting patches.

Thanks,
David Daney

>   }
>
>   int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>

Re: [RFC PATCH 1/1] MIPS: Enable VDSO randomization.

[Prem Karat]

On 04/19/14 03:56pm, David Daney wrote:
> On 04/19/2014 02:33 AM, Prem Karat wrote:
> >Based on commit 1091458d09e1a (mmap randomization)
> >
> >For 32-bit address spaces randomize within a
> >16MB space, for 64-bit within a 256MB space.
> >
> 
> How was it tested (i.e. what workload did you run to verify that the
> kernel still functions with this change)?
>
David, Sergei,

Thank You for reviewing the patch. 

Am using test suite from Ubuntu which is available here.
http://bazaar.launchpad.net/~ubuntu-bugcontrol/qa-regression-testing/master/files/head:/scripts/kernel-security/aslr/

Please find the test results below.

Without Patch (VDSO is not randomized)
---------------------------------------

root@Maleo:~# ./aslr vdso
FAIL: ASLR not functional (vdso always at 0x7fff7000)

root@Maleo:~# ./aslr rekey vdso
pre_val==cur_val
value=0x7fff7000

With patch:(VDSO is randmoized and doesn't interfere with stack)
----------------------------------------------------------------
root@cavium-octeon2:~# ./aslr rekey vdso
pre_val!=cur_val
previous_value=0x7f830ea2
current_value=0x776e2000

root@cavium-octeon2:~# ./aslr rekey vdso
pre_val!=cur_val
previous_value=0x7fb0cea2
current_value=0x77209000

root@cavium-octeon2:~# ./aslr rekey vdso
pre_val!=cur_val
previous_value=0x7f985ea2
current_value=0x7770c000

root@cavium-octeon2:~# ./aslr rekey vdso
pre_val!=cur_val
previous_value=0x7fbc6ea2
current_value=0x7fe25000

root@cavium-octeon2:~# ./aslr vdso
ok: ASLR of vdso functional
root@cavium-octeon2:~#

 
> >+
> >+	return (STACK_TOP + offset);
> 
> How can you be sure this address doesn't collide with, or otherwise
> interfere with, the stack?
>

It doesn't, as this program can print the maps file and here is the output of the
maps file each time we run aslr showing maps file.

root@cavium-octeon2:~# ./aslr rekey maps
78584000-785a5000 rwxp 00000000 00:00 0                                  [heap]
7f9d0000-7f9f1000 rw-p 00000000 00:00 0                                  [stack]
7ffa5000-7ffa6000 r-xp 00000000 00:00 0                                  [vdso]

root@cavium-octeon2:~# ./aslr rekey maps
77de0000-77e01000 rwxp 00000000 00:00 0                                  [heap]
7f91b000-7f93c000 rw-p 00000000 00:00 0                                  [stack]
7ff99000-7ff9a000 r-xp 00000000 00:00 0                                  [vdso]

root@cavium-octeon2:~# ./aslr rekey maps
77d7f000-77da0000 rwxp 00000000 00:00 0                                  [heap]
7fc2a000-7fc4b000 rw-p 00000000 00:00 0                                  [stack]
7fe09000-7fe0a000 r-xp 00000000 00:00 0                                  [vdso]

root@cavium-octeon2:~# ./aslr rekey maps
7794c000-7794d000 r-xp 00000000 00:00 0                                  [vdso]
77e4b000-77e6c000 rwxp 00000000 00:00 0                                  [heap]
7f6e7000-7f708000 rw-p 00000000 00:00 0                                  [stack]
root@cavium-octeon2:~#  

> 
> Also, as mentioned by Sergei, run checkpatch.pl to catch obvious
> stylistic problems before submitting patches.
> 

I will make the changes and send a v2 patch. 


-- 
	-prem

Re: [RFC PATCH 1/1] MIPS: Enable VDSO randomization.

[David Daney]

On 04/20/2014 08:40 PM, Prem Karat wrote:
> On 04/19/14 03:56pm, David Daney wrote:
>> On 04/19/2014 02:33 AM, Prem Karat wrote:
>>> Based on commit 1091458d09e1a (mmap randomization)
>>>
>>> For 32-bit address spaces randomize within a
>>> 16MB space, for 64-bit within a 256MB space.
>>>
>>
>> How was it tested (i.e. what workload did you run to verify that the
>> kernel still functions with this change)?
>>
> David, Sergei,
>
> Thank You for reviewing the patch.
>
> Am using test suite from Ubuntu which is available here.
> http://bazaar.launchpad.net/~ubuntu-bugcontrol/qa-regression-testing/master/files/head:/scripts/kernel-security/aslr/
>

As you probably know, currently the "vdso" in mips is really only used 
for signal return trampolines.  So to properly test it, you need code 
that does returns from signal handlers.

Does your test suite do that?


> Please find the test results below.
>
> Without Patch (VDSO is not randomized)
> ---------------------------------------
>
> root@Maleo:~# ./aslr vdso
> FAIL: ASLR not functional (vdso always at 0x7fff7000)
>
> root@Maleo:~# ./aslr rekey vdso
> pre_val==cur_val
> value=0x7fff7000
>
[...]
>
>>> +
>>> +	return (STACK_TOP + offset);
>>
>> How can you be sure this address doesn't collide with, or otherwise
>> interfere with, the stack?
>>
>
> It doesn't, as this program can print the maps file and here is the output of the
> maps file each time we run aslr showing maps file.
>
> root@cavium-octeon2:~# ./aslr rekey maps
> 78584000-785a5000 rwxp 00000000 00:00 0                                  [heap]
> 7f9d0000-7f9f1000 rw-p 00000000 00:00 0                                  [stack]
> 7ffa5000-7ffa6000 r-xp 00000000 00:00 0                                  [vdso]
>
> root@cavium-octeon2:~# ./aslr rekey maps
> 77de0000-77e01000 rwxp 00000000 00:00 0                                  [heap]
> 7f91b000-7f93c000 rw-p 00000000 00:00 0                                  [stack]
> 7ff99000-7ff9a000 r-xp 00000000 00:00 0                                  [vdso]
>
> root@cavium-octeon2:~# ./aslr rekey maps
> 77d7f000-77da0000 rwxp 00000000 00:00 0                                  [heap]
> 7fc2a000-7fc4b000 rw-p 00000000 00:00 0                                  [stack]
> 7fe09000-7fe0a000 r-xp 00000000 00:00 0                                  [vdso]
>
> root@cavium-octeon2:~# ./aslr rekey maps
> 7794c000-7794d000 r-xp 00000000 00:00 0                                  [vdso]
> 77e4b000-77e6c000 rwxp 00000000 00:00 0                                  [heap]
> 7f6e7000-7f708000 rw-p 00000000 00:00 0                                  [stack]
> root@cavium-octeon2:~#
>

Four test runs is not enough to satisfy my curiosity.  It could be that 
in these test cases, the random numbers never lined up for a collision.

You are attempting to generate two random memory maps (Stack and VDSO) 
that are in the same region of memory.  How does the system handle the 
possibility that the initial random values would collide for these two 
things.

Showing a few runs of a test program is not enough.  I would like an 
explanation of what happens when there is a collision, and how the 
system properly handles it.

Thanks,
David Daney


>>
>> Also, as mentioned by Sergei, run checkpatch.pl to catch obvious
>> stylistic problems before submitting patches.
>>
>
> I will make the changes and send a v2 patch.
>
>