[v5,3/4] arm64: mm: support ARCH_MMAP_RND_BITS.

Commit Message

Jon Hunter Dec. 7, 2015, 10:26 a.m. UTC

On 01/12/15 20:10, Daniel Cashman wrote:
> From: dcashman <[e-mail]>
> 
> arm64: arch_mmap_rnd() uses STACK_RND_MASK to generate the
> random offset for the mmap base address.  This value represents a
> compromise between increased ASLR effectiveness and avoiding
> address-space fragmentation. Replace it with a Kconfig option, which
> is sensibly bounded, so that platform developers may choose where to
> place this compromise. Keep default values as new minimums.
> 
> Signed-off-by: Daniel Cashman <[e-mail]>
> ---
>  arch/arm64/Kconfig   | 31 +++++++++++++++++++++++++++++++
>  arch/arm64/mm/mmap.c |  8 ++++++--
>  2 files changed, 37 insertions(+), 2 deletions(-)

[snip]

> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
> index ed17747..af461b9 100644
> --- a/arch/arm64/mm/mmap.c
> +++ b/arch/arm64/mm/mmap.c
> @@ -51,8 +51,12 @@ unsigned long arch_mmap_rnd(void)
>  {
>  	unsigned long rnd;
>  
> -	rnd = (unsigned long)get_random_int() & STACK_RND_MASK;
> -
> +ifdef CONFIG_COMPAT
> +	if (test_thread_flag(TIF_32BIT))
> +		rnd = (unsigned long)get_random_int() % (1 << mmap_rnd_compat_bits);
> +	else
> +#endif
> +		rnd = (unsigned long)get_random_int() % (1 << mmap_rnd_bits);
>  	return rnd << PAGE_SHIFT;
>  }

The above is causing a build failure on -next today.

commit 42a6c8953112a9856dd09148c3d6a2cc106b6003
Author: Jon Hunter <[e-mail]>
Date:   Mon Dec 7 10:15:47 2015 +0000

    ARM64: mm: Fix build failure caused by invalid ifdef statement
    
    Commit 2e4614190421 ("arm64-mm-support-arch_mmap_rnd_bits-v4") caused the
    following build failure due to a missing "#". Fix this.
    
    arch/arm64/mm/mmap.c: In function ‘arch_mmap_rnd’:
    arch/arm64/mm/mmap.c:54:1: error: ‘ifdef’ undeclared (first use in this function)
     ifdef CONFIG_COMPAT
      ^
    Signed-off-by: Jon Hunter <[e-mail]>


Cheers
Jon

Comments

Arnd Bergmann Dec. 7, 2015, 11:13 a.m. UTC | #1

On Monday 07 December 2015 10:26:16 Jon Hunter wrote:
> 
> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
> index af461b935137..e59a75a308bc 100644
> --- a/arch/arm64/mm/mmap.c
> +++ b/arch/arm64/mm/mmap.c
> @@ -51,7 +51,7 @@ unsigned long arch_mmap_rnd(void)
>  {
>         unsigned long rnd;
>  
> -ifdef CONFIG_COMPAT
> +#ifdef CONFIG_COMPAT
>         if (test_thread_flag(TIF_32BIT))
>                 rnd = (unsigned long)get_random_int() % (1 << mmap_rnd_compat_bits);
>         else
> 
> Cheers
> 

Ideally we'd remove the #ifdef around the mmap_rnd_compat_bits declaration
and change this code to use

	if (IS_ENABLED(CONFIG_COMPAT) && test_thread_flag(TIF_32BIT))

	Arnd

Daniel Cashman Dec. 7, 2015, 6:26 p.m. UTC | #2

On 12/07/2015 03:13 AM, Arnd Bergmann wrote:
> On Monday 07 December 2015 10:26:16 Jon Hunter wrote:
>>
>> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
>> index af461b935137..e59a75a308bc 100644
>> --- a/arch/arm64/mm/mmap.c
>> +++ b/arch/arm64/mm/mmap.c
>> @@ -51,7 +51,7 @@ unsigned long arch_mmap_rnd(void)
>>  {
>>         unsigned long rnd;
>>  
>> -ifdef CONFIG_COMPAT
>> +#ifdef CONFIG_COMPAT

Thank you Jon.  This ought to persuade me to do a final build against
the final patch, rather than the ugly porting I had been doing.  I'll
include this in v6. (how embarassing =/)

>>         if (test_thread_flag(TIF_32BIT))
>>                 rnd = (unsigned long)get_random_int() % (1 << mmap_rnd_compat_bits);
>>         else
>>
>> Cheers
>>
> 
> Ideally we'd remove the #ifdef around the mmap_rnd_compat_bits declaration
> and change this code to use
> 
> 	if (IS_ENABLED(CONFIG_COMPAT) && test_thread_flag(TIF_32BIT))
> 
> 	Arnd

That would result in "undefined reference to mmap_rnd_compat_bits" in
the not-defined case, no?

Thank You,
Dan

Arnd Bergmann Dec. 8, 2015, 10:03 a.m. UTC | #3

On Monday 07 December 2015 10:26:34 Daniel Cashman wrote:
> > Ideally we'd remove the #ifdef around the mmap_rnd_compat_bits declaration
> > and change this code to use
> > 
> >       if (IS_ENABLED(CONFIG_COMPAT) && test_thread_flag(TIF_32BIT))
> > 
> That would result in "undefined reference to mmap_rnd_compat_bits" in
> the not-defined case, no?

No. The compiler eliminates all code paths that it knows are unused.
The IS_ENABLED() macro is designed to let the compiler figure this out.

	Arnd

Patch

diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
index af461b935137..e59a75a308bc 100644
--- a/arch/arm64/mm/mmap.c
+++ b/arch/arm64/mm/mmap.c
@@ -51,7 +51,7 @@  unsigned long arch_mmap_rnd(void)
 {
        unsigned long rnd;
 
-ifdef CONFIG_COMPAT
+#ifdef CONFIG_COMPAT
        if (test_thread_flag(TIF_32BIT))
                rnd = (unsigned long)get_random_int() % (1 << mmap_rnd_compat_bits);
        else