Re: [PATCH] mm/x86: AMD Bulldozer ASLR fix

[Borislav Petkov <bp@alien8.de>]

On Wed, Mar 25, 2015 at 07:36:17PM +0100, Hector Marco-Gisbert wrote:
> A bug in Linux ASLR implementation which affects some AMD processors has been
> found. The issue affects to all Linux process even if they are not using
> shared libraries (statically compiled).
> 
> The problem appears because some mmapped objects (VDSO, libraries, etc.)
> are poorly randomized in an attempt to avoid cache aliasing penalties for
> AMD Bulldozer (Family 15h) processors.
> 
> Affected systems have reduced the mmapped files entropy by eight.
> 
> The following output is the run on an AMD Opteron 62xx class CPU processor
> under x86_64 Linux 4.0.0:
> 
> for i in `seq 1 10`; do cat /proc/self/maps | grep "r-xp.*libc" ; done
> b7588000-b7736000 r-xp 00000000 00:01 4924       /lib/i386-linux-gnu/libc.so.6
> b7570000-b771e000 r-xp 00000000 00:01 4924       /lib/i386-linux-gnu/libc.so.6
> b75d0000-b777e000 r-xp 00000000 00:01 4924       /lib/i386-linux-gnu/libc.so.6
> b75b0000-b775e000 r-xp 00000000 00:01 4924       /lib/i386-linux-gnu/libc.so.6
> b7578000-b7726000 r-xp 00000000 00:01 4924       /lib/i386-linux-gnu/libc.so.6
> 
> As shown in the previous output, the bits 12, 13 and 14 are always 0. The address
> always ends in 0x8000 or 0x0000.
> 
> The bug is caused by a hack to improve performance by avoiding cache aliasing
> penalties in the Family 15h of AMD Bulldozer processors (commit: dfb09f9b).
> 
> 32-bit systems are specially sensitive to this issue because the entropy for
> libraries is reduced from 2^8 to 2^5, which means that libraries only have 32
> different places where they can be loaded.
> 
> This patch randomizes per boot the three affected bits, rather than setting
> them to zero. Since all the shared pages have the same value at the bits
> [12..14], there is no cache aliasing problems (which is supposed to be the
> cause of performance loss). On the other hand, since the value is not
> known by a potential remote attacker, the ASLR preserves its effectiveness.
> 
> More details at:
> 
> http://hmarco.org/bugs/AMD-Bulldozer-linux-ASLR-weakness-reducing-mmaped-files-by-eight.html
> 
> 
> Signed-off-by: Hector Marco-Gisbert <hecmargi@upv.es>
> Signed-off-by: Ismael Ripoll <iripoll@disca.upv.es>
> ---
>  arch/x86/include/asm/elf.h   |  1 +
>  arch/x86/kernel/cpu/amd.c    |  3 +++
>  arch/x86/kernel/sys_x86_64.c | 20 +++++++++++++++++---
>  3 files changed, 21 insertions(+), 3 deletions(-)

Please run your patch through checkpatch to address minor coding style
issues.

> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index ca3347a..bd292ce 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -365,6 +365,7 @@ enum align_flags {
>  struct va_alignment {
>  	int flags;
>  	unsigned long mask;
> +	unsigned long bits;
>  } ____cacheline_aligned;
>  
>  extern struct va_alignment va_align;
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 15c5df9..45a41be 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/io.h>
>  #include <linux/sched.h>
> +#include <linux/random.h>
>  #include <asm/processor.h>
>  #include <asm/apic.h>
>  #include <asm/cpu.h>
> @@ -488,6 +489,8 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
>  
>  		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
>  		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;

Newline here.

> +		/* A random value per boot for bits 12,13 and 14 */

		"A random value for bit slice [12:upper_bit)"

the interval end is open.

> +		va_align.bits = get_random_int() & va_align.mask;
>  	}
>  }
>  
> diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
> index 30277e2..d38905d 100644
> --- a/arch/x86/kernel/sys_x86_64.c
> +++ b/arch/x86/kernel/sys_x86_64.c
> @@ -34,10 +34,16 @@ static unsigned long get_align_mask(void)
>  	return va_align.mask;
>  }

Please add a short comment here over this function explaining why we're
doing this and thus what the value that goes into ->align_offset below
represents.

> +static unsigned long get_align_bits(void){
> +
> +	return va_align.bits & get_align_mask();
> +}
> +

...

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--