DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 36379C049D5D
Date: Fri, 24 Mar 2017 12:35:10 +0800
From: Baoquan He <bhe@redhat.com>
To: Dave Young <dyoung@redhat.com>
Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        Thomas Garnier <thgarnie@google.com>,
        Kees Cook <keescook@chromium.org>, Borislav Petkov <bp@alien8.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        Masahiro Yamada <yamada.masahiro@socionext.com>
Subject: Re: [PATCH v1 RESEND 1/2] x86/mm/KASLR: EFI region is mistakenly
 included into KASLR VA space for randomization
Message-ID: <20170324043510.GF31260@x1>
References: <1490239655-20902-1-git-send-email-bhe@redhat.com>
 <1490239655-20902-2-git-send-email-bhe@redhat.com>
 <20170324022953.GA3715@dhcp-128-65.nay.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170324022953.GA3715@dhcp-128-65.nay.redhat.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3364
Lines: 84

On 03/24/17 at 10:29am, Dave Young wrote:
> Hi, Baoquan
> 
> On 03/23/17 at 11:27am, Baoquan He wrote:
> > Currently KASLR is enabled on three regions: the direct mapping of physical
> > memory, vamlloc and vmemmap. However EFI region is also mistakenly included
> > for VA space randomization because of misusing EFI_VA_START macro and
> > assuming EFI_VA_START < EFI_VA_END.
> > 
> > The EFI region is reserved for EFI runtime services virtual mapping which
> > should not be included in kaslr ranges. It will be re-used by kexec/kdump
> > kernel, the mistake may cause failure when jump to kexec/kdump kernel if
> > vmemmap allocation stomps on the allocated efi mapping region.
> 
> No need to mention kexec/kdump in changelog although it is true that
> kexec kernel will use the persistent efi runtime mapping. The main point
> is it is wrong to use the reserved vm space for efi.

I only say the consequence from kdump point of view and point out that.

Anyway I am fine w/o kexec/kdump text. Will repost this patch only
without kexec-ed kernel saying.


> 
> Also I think this patch can be sent as a standalone patch, no need to be
> a patch series. For the second patch I think it depends on efi
> maintainer's opinion, personally I think only this simple fix for kaslr only
> will be better.
> 
> > 
> > In Documentation/x86/x86_64/mm.txt, we can see:
> >   ffffffef00000000 - fffffffeffffffff (=64 GB) EFI region mapping space
> > EFI use the space from -4G to -64G thus EFI_VA_START > EFI_VA_END
> > Here EFI_VA_START = -4G, and EFI_VA_END = -64G
> > 
> > Changing EFI_VA_START to EFI_VA_END in mm/kaslr.c fixes this problem.
> > 
> > Cc: <stable@vger.kernel.org> #4.8+
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Acked-by: Dave Young <dyoung@redhat.com>
> > Reviewed-by: Bhupesh Sharma <bhsharma@redhat.com>
> > Acked-by: Thomas Garnier <thgarnie@google.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com> 
> > Cc: x86@kernel.org
> > Cc: Thomas Garnier <thgarnie@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > ---
> >  arch/x86/mm/kaslr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> > index 887e571..aed2064 100644
> > --- a/arch/x86/mm/kaslr.c
> > +++ b/arch/x86/mm/kaslr.c
> > @@ -48,7 +48,7 @@ static const unsigned long vaddr_start = __PAGE_OFFSET_BASE;
> >  #if defined(CONFIG_X86_ESPFIX64)
> >  static const unsigned long vaddr_end = ESPFIX_BASE_ADDR;
> >  #elif defined(CONFIG_EFI)
> > -static const unsigned long vaddr_end = EFI_VA_START;
> > +static const unsigned long vaddr_end = EFI_VA_END;
> >  #else
> >  static const unsigned long vaddr_end = __START_KERNEL_map;
> >  #endif
> > @@ -105,7 +105,7 @@ void __init kernel_randomize_memory(void)
> >  	 */
> >  	BUILD_BUG_ON(vaddr_start >= vaddr_end);
> >  	BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
> > -		     vaddr_end >= EFI_VA_START);
> > +		     vaddr_end >= EFI_VA_END);
> >  	BUILD_BUG_ON((IS_ENABLED(CONFIG_X86_ESPFIX64) ||
> >  		      IS_ENABLED(CONFIG_EFI)) &&
> >  		     vaddr_end >= __START_KERNEL_map);
> > -- 
> > 2.5.5
> > 
> 
> Thanks
> Dave