From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hector Marco Subject: Re: [PATCH] mm/x86: AMD Bulldozer ASLR fix Date: Wed, 25 Mar 2015 19:29:26 +0100 Message-ID: <5512FE86.9070209@upv.es> References: <1427220048-6618-1-git-send-email-hecmargi@upv.es> <20150324191556.GA11571@pd.tnic> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Alexander Viro , Jan-Simon , linux-fsdevel@vger.kernel.org, Ismael Ripoll , Kees Cook - ASLRv3 To: Borislav Petkov Return-path: In-Reply-To: <20150324191556.GA11571@pd.tnic> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org El 24/03/15 a las 20:15, Borislav Petkov escribi=C3=B3: > On Tue, Mar 24, 2015 at 07:00:48PM +0100, Hector Marco-Gisbert wrote: >> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c >> index 15c5df9..a693d54 100644 >> --- a/arch/x86/kernel/cpu/amd.c >> +++ b/arch/x86/kernel/cpu/amd.c >> @@ -5,6 +5,7 @@ >> >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -18,6 +19,8 @@ >> >> #include "cpu.h" >> >> +unsigned long rnd_bulldozer_bits =3D 0; >> + >> static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long= *p) >> { >> u32 gprs[8] =3D { 0 }; >> @@ -488,6 +491,8 @@ static void bsp_init_amd(struct cpuinfo_x86 *c) >> >> va_align.mask =3D (upperbit - 1) & PAGE_MASK; >> va_align.flags =3D ALIGN_VA_32 | ALIGN_VA_64; >> + /* A random value per boot for bits 12,13 and 14 */ >> + rnd_bulldozer_bits =3D get_random_int() & va_align.mask; > > Hmm, this should be done differently: > > va_align should have a ->bits member which gets ORed in into the hole > made my va_align.mask... > Yes, It looks better using va_align. >> } >> } >> >> diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_= 64.c >> index 30277e2..5b8ad01 100644 >> --- a/arch/x86/kernel/sys_x86_64.c >> +++ b/arch/x86/kernel/sys_x86_64.c >> @@ -18,6 +18,7 @@ >> >> #include >> #include >> +#include >> >> /* >> * Align a virtual address to avoid aliasing in the I$ on AMD F15h= =2E >> @@ -34,10 +35,16 @@ static unsigned long get_align_mask(void) >> return va_align.mask; >> } >> >> +static unsigned long get_bulldozer_bits(void){ >> + >> + return rnd_bulldozer_bits & get_align_mask(); >> +} >> + >> unsigned long align_vdso_addr(unsigned long addr) >> { >> unsigned long align_mask =3D get_align_mask(); >> - return (addr + align_mask) & ~align_mask; >> + addr =3D (addr + align_mask) & ~align_mask; >> + return addr | get_bulldozer_bits(); >> } >> >> static int __init control_va_addr_alignment(char *str) >> @@ -137,7 +144,10 @@ arch_get_unmapped_area(struct file *filp, unsig= ned long addr, >> info.high_limit =3D end; >> info.align_mask =3D filp ? get_align_mask() : 0; > > info.align_bits =3D get_align_bits() : 0; > I see your point. The drawback of adding a new field (align_bits) to th= e info=20 struct is that all architectures need to initialize the info.align_bits= =2E In=20 addition, the generic functions unmapped_area()/unmapped_area_topdown()= in file=20 mm/mmap.c, need to be modified to set the bits [12..14] using the new f= ield=20 info.align_bits. A possible alternative which does not add a new field, is to use the=20 "align_offset" variable. By adding the "get_align_bits()" value to the=20 "align_offset" the bits [12..14] are randomized per boot. Patch coming. Hector. >> info.align_offset =3D pgoff << PAGE_SHIFT; >> - return vm_unmapped_area(&info); >> + addr =3D vm_unmapped_area(&info); >> + if (!(addr & ~PAGE_MASK)) >> + return filp ? (addr|get_bulldozer_bits()) : addr; >> + return addr; >> } >> >> unsigned long >> @@ -178,7 +188,7 @@ arch_get_unmapped_area_topdown(struct file *filp= , const unsigned long addr0, >> info.align_offset =3D pgoff << PAGE_SHIFT; >> addr =3D vm_unmapped_area(&info); >> if (!(addr & ~PAGE_MASK)) >> - return addr; >> + return filp ? (addr|get_bulldozer_bits()) : addr; > > Ditto. >