[PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

EFI allocate runtime services regions down from EFI_VA_START, -4G.
It should be top-down handling.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a4695da..6cbf9e0 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.
  * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
  */
 static u64 efi_va = EFI_VA_START;
-- 
2.5.5

[PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

EFI allocate runtime services regions down from EFI_VA_START, -4G.
It should be top-down handling.

Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a4695da..6cbf9e0 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -47,7 +47,7 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
+ * We allocate runtime services regions top-down, starting from -4G, i.e.
  * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
  */
 static u64 efi_va = EFI_VA_START;
-- 
2.5.5

[PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Baoquan He]

EFI allocates runtime services regions top-down, starting from EFI_VA_START
to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
EFI region. The upper boundary of memory regions randomized by KASLR should
be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.

Correct it in this patch.

Signed-off-by: Baoquan He <bhe@redhat.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

[PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Baoquan He]

EFI allocates runtime services regions top-down, starting from EFI_VA_START
to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
EFI region. The upper boundary of memory regions randomized by KASLR should
be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.

Correct it in this patch.

Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Dave Young]

On 03/08/17 at 03:47pm, Baoquan He wrote:
> EFI allocate runtime services regions down from EFI_VA_START, -4G.
> It should be top-down handling.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/platform/efi/efi_64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index a4695da..6cbf9e0 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -47,7 +47,7 @@
>  #include <asm/pgalloc.h>
>  
>  /*
> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> + * We allocate runtime services regions top-down, starting from -4G, i.e.

Baoquan, I think original bottom-up is right, it is just considering
-68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
although from mathematics view it is lower then positive addresses.

>   * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
>   */
>  static u64 efi_va = EFI_VA_START;
> -- 
> 2.5.5
> 

Thanks
Dave

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Dave Young]

On 03/08/17 at 03:47pm, Baoquan He wrote:
> EFI allocates runtime services regions top-down, starting from EFI_VA_START
> to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
> EFI region. The upper boundary of memory regions randomized by KASLR should
> be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
> 
> Correct it in this patch.
> 
> Signed-off-by: Baoquan He <bhe@redhat.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
> 

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Dave Young]

On 03/08/17 at 03:47pm, Baoquan He wrote:
> EFI allocates runtime services regions top-down, starting from EFI_VA_START
> to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
> EFI region. The upper boundary of memory regions randomized by KASLR should
> be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
> 
> Correct it in this patch.
> 
> Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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
> 

Acked-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks
Dave

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Bhupesh Sharma]

On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung@redhat.com> wrote:
> On 03/08/17 at 03:47pm, Baoquan He wrote:
>> EFI allocates runtime services regions top-down, starting from EFI_VA_START
>> to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
>> EFI region. The upper boundary of memory regions randomized by KASLR should
>> be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
>>
>> Correct it in this patch.
>>
>> Signed-off-by: Baoquan He <bhe@redhat.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
>>
>
> Acked-by: Dave Young <dyoung@redhat.com>
>

Thanks Bao for this fix. This makes the KASLR code consistent with
Address space markers hints in [1]

[1] http://lxr.free-electrons.com/source/arch/x86/mm/dump_pagetables.c#L82

Reviewed-by: Bhupesh Sharma <bhsharma@redhat.com>

Regards,
Bhupesh

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Bhupesh Sharma]

On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 03/08/17 at 03:47pm, Baoquan He wrote:
>> EFI allocates runtime services regions top-down, starting from EFI_VA_START
>> to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
>> EFI region. The upper boundary of memory regions randomized by KASLR should
>> be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
>>
>> Correct it in this patch.
>>
>> Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  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
>>
>
> Acked-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>

Thanks Bao for this fix. This makes the KASLR code consistent with
Address space markers hints in [1]

[1] http://lxr.free-electrons.com/source/arch/x86/mm/dump_pagetables.c#L82

Reviewed-by: Bhupesh Sharma <bhsharma-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Regards,
Bhupesh

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

Forgot cc to Boris, add him.

On 03/08/17 at 04:18pm, Dave Young wrote:
> On 03/08/17 at 03:47pm, Baoquan He wrote:
> > EFI allocate runtime services regions down from EFI_VA_START, -4G.
> > It should be top-down handling.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/platform/efi/efi_64.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index a4695da..6cbf9e0 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -47,7 +47,7 @@
> >  #include <asm/pgalloc.h>
> >  
> >  /*
> > - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> > + * We allocate runtime services regions top-down, starting from -4G, i.e.
> 
> Baoquan, I think original bottom-up is right, it is just considering
> -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> although from mathematics view it is lower then positive addresses.

Thanks for reviewing!

I am not sure. Just in efi_map_region() it gets the starting va to map
'size' big of region by below code:
	efi_va -= size;

-4G and -68G just a trick which makes people understand easily, still we
think kernel text mapping region is in higher addr area then vmalloc. I
personnally think.

Thanks
Baoquan

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

Forgot cc to Boris, add him.

On 03/08/17 at 04:18pm, Dave Young wrote:
> On 03/08/17 at 03:47pm, Baoquan He wrote:
> > EFI allocate runtime services regions down from EFI_VA_START, -4G.
> > It should be top-down handling.
> > 
> > Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/platform/efi/efi_64.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index a4695da..6cbf9e0 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -47,7 +47,7 @@
> >  #include <asm/pgalloc.h>
> >  
> >  /*
> > - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> > + * We allocate runtime services regions top-down, starting from -4G, i.e.
> 
> Baoquan, I think original bottom-up is right, it is just considering
> -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> although from mathematics view it is lower then positive addresses.

Thanks for reviewing!

I am not sure. Just in efi_map_region() it gets the starting va to map
'size' big of region by below code:
	efi_va -= size;

-4G and -68G just a trick which makes people understand easily, still we
think kernel text mapping region is in higher addr area then vmalloc. I
personnally think.

Thanks
Baoquan

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Borislav Petkov]

On Wed, Mar 08, 2017 at 04:45:13PM +0800, Baoquan He wrote:
> -4G and -68G just a trick which makes people understand easily, still we
> think kernel text mapping region is in higher addr area then vmalloc. I
> personnally think.

Just remove the direction: bottom-up or top-down, it will confuse people.

"We allocate runtime services regions starting from -4G, i.e.
0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space
is limited to 64G."

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Bhupesh Sharma]

Hi Dave,

On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung@redhat.com> wrote:
> On 03/08/17 at 03:47pm, Baoquan He wrote:
>> EFI allocate runtime services regions down from EFI_VA_START, -4G.
>> It should be top-down handling.
>>
>> Signed-off-by: Baoquan He <bhe@redhat.com>
>> ---
>>  arch/x86/platform/efi/efi_64.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
>> index a4695da..6cbf9e0 100644
>> --- a/arch/x86/platform/efi/efi_64.c
>> +++ b/arch/x86/platform/efi/efi_64.c
>> @@ -47,7 +47,7 @@
>>  #include <asm/pgalloc.h>
>>
>>  /*
>> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
>> + * We allocate runtime services regions top-down, starting from -4G, i.e.
>
> Baoquan, I think original bottom-up is right, it is just considering
> -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> although from mathematics view it is lower then positive addresses.

I think you have a valid point, but I think the -4G convention is
probably too confusing to read and may lead to issues when we use this
for future feature addition as well. It would be more useful to use
the macros similar to the MODULES_{} addresses we use currently in
'arch/x86/include/asm/pgtable_64_types.h':

#define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
#define MODULES_END      _AC(0xffffffffff000000, UL)
#define MODULES_LEN   (MODULES_END - MODULES_VADDR)

May be we can use the following convention for the EFI_VA_{} addresses
as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':

#define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
#define EFI_VA_END    _AC(0xffffffef00000000, UL)

which is less confusing to read in my opinion.

@Baoquan: Please share your views.

Regards,
Bhupesh

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

On 03/08/17 at 09:54am, Borislav Petkov wrote:
> On Wed, Mar 08, 2017 at 04:45:13PM +0800, Baoquan He wrote:
> > -4G and -68G just a trick which makes people understand easily, still we
> > think kernel text mapping region is in higher addr area then vmalloc. I
> > personnally think.
> 
> Just remove the direction: bottom-up or top-down, it will confuse people.
> 
> "We allocate runtime services regions starting from -4G, i.e.
> 0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space
> is limited to 64G."

Right, will repost one.

Thanks!

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

On 03/08/17 at 02:30pm, Bhupesh Sharma wrote:
> Hi Dave,
> 
> On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung@redhat.com> wrote:
> > On 03/08/17 at 03:47pm, Baoquan He wrote:
> >> EFI allocate runtime services regions down from EFI_VA_START, -4G.
> >> It should be top-down handling.
> >>
> >> Signed-off-by: Baoquan He <bhe@redhat.com>
> >> ---
> >>  arch/x86/platform/efi/efi_64.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> >> index a4695da..6cbf9e0 100644
> >> --- a/arch/x86/platform/efi/efi_64.c
> >> +++ b/arch/x86/platform/efi/efi_64.c
> >> @@ -47,7 +47,7 @@
> >>  #include <asm/pgalloc.h>
> >>
> >>  /*
> >> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> >> + * We allocate runtime services regions top-down, starting from -4G, i.e.
> >
> > Baoquan, I think original bottom-up is right, it is just considering
> > -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> > although from mathematics view it is lower then positive addresses.
> 
> I think you have a valid point, but I think the -4G convention is
> probably too confusing to read and may lead to issues when we use this
> for future feature addition as well. It would be more useful to use
> the macros similar to the MODULES_{} addresses we use currently in
> 'arch/x86/include/asm/pgtable_64_types.h':
> 
> #define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
> #define MODULES_END      _AC(0xffffffffff000000, UL)
> #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
> 
> May be we can use the following convention for the EFI_VA_{} addresses
> as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':
> 
> #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> #define EFI_VA_END    _AC(0xffffffef00000000, UL)
> 
> which is less confusing to read in my opinion.
> 
> @Baoquan: Please share your views.

Yes, it looks better. I can repost with this change. Thanks.

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

On 03/08/17 at 02:30pm, Bhupesh Sharma wrote:
> Hi Dave,
> 
> On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On 03/08/17 at 03:47pm, Baoquan He wrote:
> >> EFI allocate runtime services regions down from EFI_VA_START, -4G.
> >> It should be top-down handling.
> >>
> >> Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  arch/x86/platform/efi/efi_64.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> >> index a4695da..6cbf9e0 100644
> >> --- a/arch/x86/platform/efi/efi_64.c
> >> +++ b/arch/x86/platform/efi/efi_64.c
> >> @@ -47,7 +47,7 @@
> >>  #include <asm/pgalloc.h>
> >>
> >>  /*
> >> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> >> + * We allocate runtime services regions top-down, starting from -4G, i.e.
> >
> > Baoquan, I think original bottom-up is right, it is just considering
> > -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> > although from mathematics view it is lower then positive addresses.
> 
> I think you have a valid point, but I think the -4G convention is
> probably too confusing to read and may lead to issues when we use this
> for future feature addition as well. It would be more useful to use
> the macros similar to the MODULES_{} addresses we use currently in
> 'arch/x86/include/asm/pgtable_64_types.h':
> 
> #define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
> #define MODULES_END      _AC(0xffffffffff000000, UL)
> #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
> 
> May be we can use the following convention for the EFI_VA_{} addresses
> as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':
> 
> #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> #define EFI_VA_END    _AC(0xffffffef00000000, UL)
> 
> which is less confusing to read in my opinion.
> 
> @Baoquan: Please share your views.

Yes, it looks better. I can repost with this change. Thanks.

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Borislav Petkov]

On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
> Yes, it looks better. I can repost with this change. Thanks.

No it doesn't:

#define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
#define EFI_VA_END       (-68 * (_AC(1, UL) << 30))

That's -4G (the shift by 30) and -68G, respectively.

> > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> > #define EFI_VA_END    _AC(0xffffffef00000000, UL)

That is something which I need to type into a calculator first.

Can you guys point your attention to something which is really broken
and stop wasting your time? And there's enough really broken crap left
and right...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Borislav Petkov]

On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
> Yes, it looks better. I can repost with this change. Thanks.

No it doesn't:

#define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
#define EFI_VA_END       (-68 * (_AC(1, UL) << 30))

That's -4G (the shift by 30) and -68G, respectively.

> > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> > #define EFI_VA_END    _AC(0xffffffef00000000, UL)

That is something which I need to type into a calculator first.

Can you guys point your attention to something which is really broken
and stop wasting your time? And there's enough really broken crap left
and right...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

On 03/08/17 at 02:30pm, Bhupesh Sharma wrote:
> Hi Dave,
> 
> On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung@redhat.com> wrote:
> > On 03/08/17 at 03:47pm, Baoquan He wrote:
> >> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> >> + * We allocate runtime services regions top-down, starting from -4G, i.e.
> >
> > Baoquan, I think original bottom-up is right, it is just considering
> > -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> > although from mathematics view it is lower then positive addresses.
> 
> I think you have a valid point, but I think the -4G convention is
> probably too confusing to read and may lead to issues when we use this
> for future feature addition as well. It would be more useful to use
> the macros similar to the MODULES_{} addresses we use currently in
> 'arch/x86/include/asm/pgtable_64_types.h':
> 
> #define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
> #define MODULES_END      _AC(0xffffffffff000000, UL)
> #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
> 
> May be we can use the following convention for the EFI_VA_{} addresses
> as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':
> 
> #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> #define EFI_VA_END    _AC(0xffffffef00000000, UL)

Isn't it like this:

#define EFI_VA_START    _AC(0xffffffff00000000, UL)
#define EFI_VA_END    _AC(0xffffffef00000000, UL)

Just make them be equal to value which computer stores -4G and -68G?

You can see in arch/x86/platform/efi/efi_64.c, it's using efi_va minus
size directly, here size should be 4K, page aligned. 

	efi_va -= size;

Above formula has considered the open interval attribute of
EFI_VA_START. Making EFI_VA_START be 0xfffffffeffffffff could be wrong.

Right?

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

On 03/08/17 at 02:30pm, Bhupesh Sharma wrote:
> Hi Dave,
> 
> On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On 03/08/17 at 03:47pm, Baoquan He wrote:
> >> - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> >> + * We allocate runtime services regions top-down, starting from -4G, i.e.
> >
> > Baoquan, I think original bottom-up is right, it is just considering
> > -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> > although from mathematics view it is lower then positive addresses.
> 
> I think you have a valid point, but I think the -4G convention is
> probably too confusing to read and may lead to issues when we use this
> for future feature addition as well. It would be more useful to use
> the macros similar to the MODULES_{} addresses we use currently in
> 'arch/x86/include/asm/pgtable_64_types.h':
> 
> #define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
> #define MODULES_END      _AC(0xffffffffff000000, UL)
> #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
> 
> May be we can use the following convention for the EFI_VA_{} addresses
> as per 'http://lxr.free-electrons.com/source/Documentation/x86/x86_64/mm.txt#L19':
> 
> #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> #define EFI_VA_END    _AC(0xffffffef00000000, UL)

Isn't it like this:

#define EFI_VA_START    _AC(0xffffffff00000000, UL)
#define EFI_VA_END    _AC(0xffffffef00000000, UL)

Just make them be equal to value which computer stores -4G and -68G?

You can see in arch/x86/platform/efi/efi_64.c, it's using efi_va minus
size directly, here size should be 4K, page aligned. 

	efi_va -= size;

Above formula has considered the open interval attribute of
EFI_VA_START. Making EFI_VA_START be 0xfffffffeffffffff could be wrong.

Right?

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

On 03/08/17 at 10:35am, Borislav Petkov wrote:
> On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
> > Yes, it looks better. I can repost with this change. Thanks.
> 
> No it doesn't:

All right, I will just update the code comment. Just back ported kaslr
to our OS product, people reviewed and found the upper boundary of kaslr
mm region is EFI_VA_START, that's not correct, it has to be corrected
firstly in upstream. Then found the confusion in code comment.

Change or keep it, both is fine to me.

Thanks!

> 
> #define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
> #define EFI_VA_END       (-68 * (_AC(1, UL) << 30))
> 
> That's -4G (the shift by 30) and -68G, respectively.
> 
> > > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> > > #define EFI_VA_END    _AC(0xffffffef00000000, UL)
> 
> That is something which I need to type into a calculator first.
> 
> Can you guys point your attention to something which is really broken
> and stop wasting your time? And there's enough really broken crap left
> and right...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Baoquan He]

On 03/08/17 at 10:35am, Borislav Petkov wrote:
> On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
> > Yes, it looks better. I can repost with this change. Thanks.
> 
> No it doesn't:

All right, I will just update the code comment. Just back ported kaslr
to our OS product, people reviewed and found the upper boundary of kaslr
mm region is EFI_VA_START, that's not correct, it has to be corrected
firstly in upstream. Then found the confusion in code comment.

Change or keep it, both is fine to me.

Thanks!

> 
> #define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
> #define EFI_VA_END       (-68 * (_AC(1, UL) << 30))
> 
> That's -4G (the shift by 30) and -68G, respectively.
> 
> > > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
> > > #define EFI_VA_END    _AC(0xffffffef00000000, UL)
> 
> That is something which I need to type into a calculator first.
> 
> Can you guys point your attention to something which is really broken
> and stop wasting your time? And there's enough really broken crap left
> and right...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v2 1/2] x86/efi/64: Clean up code comment about efi region

[Baoquan He]

EFI allocates runtime services regions starting from EFI_VA_START, -4G,
decrement to EFI_VA_END. So remove the bottom-up term to avoid confusion.

Signed-off-by: Baoquan He <bhe@redhat.com>
---

 arch/x86/platform/efi/efi_64.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a4695da..e941b29 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -47,8 +47,9 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
- * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
+ * We allocate runtime services regions starting from -4G, i.e.
+ * 0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space
+ * is limited to 64G.
  */
 static u64 efi_va = EFI_VA_START;
 
-- 
2.5.5

[PATCH v2 1/2] x86/efi/64: Clean up code comment about efi region

[Baoquan He]

EFI allocates runtime services regions starting from EFI_VA_START, -4G,
decrement to EFI_VA_END. So remove the bottom-up term to avoid confusion.

Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---

 arch/x86/platform/efi/efi_64.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a4695da..e941b29 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -47,8 +47,9 @@
 #include <asm/pgalloc.h>
 
 /*
- * We allocate runtime services regions bottom-up, starting from -4G, i.e.
- * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.
+ * We allocate runtime services regions starting from -4G, i.e.
+ * 0xffff_ffff_0000_0000 and decrement as we go. The EFI VA mapping space
+ * is limited to 64G.
  */
 static u64 efi_va = EFI_VA_START;
 
-- 
2.5.5

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Borislav Petkov]

On Wed, Mar 08, 2017 at 06:17:50PM +0800, Baoquan He wrote:
> All right, I will just update the code comment. Just back ported kaslr
> to our OS product, people reviewed and found the upper boundary of kaslr
> mm region is EFI_VA_START, that's not correct, it has to be corrected
> firstly in upstream. Then found the confusion in code comment.

        BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
+                    vaddr_end >= EFI_VA_END);

so I think that once we've done the mapping, we won't need anymore VA
space so we could simply check the range [efi_va, EFI_VA_START] instead.

However, that won't work currently because evi_va is not valid at
build time. And it won't work at boot time either because, AFAICT,
kernel_randomize_memory() runs before efi_enter_virtual_mode() so ...

So yours is probably OK.

I guess what's confusing there is the naming - EFI_VA_START and
EFI_VA_END. They're kinda swapped because of the direction we take when
we start mapping runtime services, i.e., from the higher (unsigned)
address to lower.

I guess we could swap the naming so that it doesn't confuse people but
that would be up to EFI maintainers.

Then stuff like that:

# ifdef CONFIG_EFI
        { EFI_VA_END,           "EFI Runtime Services" },
# endif

will make more sense when they are:

# ifdef CONFIG_EFI
        { EFI_VA_START,           "EFI Runtime Services" },
# endif

But changing it now could confuse more people who have the current
mental picture of the mapping direction so I'd vote for the simple fix
above.

Again, as previously, this is a maintainer decision.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Thomas Garnier]

Thanks for the change.

Acked-by: Thomas Garnier <thgarnie@google.com>

On Wed, Mar 8, 2017 at 12:35 AM, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> On Wed, Mar 8, 2017 at 1:48 PM, Dave Young <dyoung@redhat.com> wrote:
>> On 03/08/17 at 03:47pm, Baoquan He wrote:
>>> EFI allocates runtime services regions top-down, starting from EFI_VA_START
>>> to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
>>> EFI region. The upper boundary of memory regions randomized by KASLR should
>>> be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
>>>
>>> Correct it in this patch.
>>>
>>> Signed-off-by: Baoquan He <bhe@redhat.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
>>>
>>
>> Acked-by: Dave Young <dyoung@redhat.com>
>>
>
> Thanks Bao for this fix. This makes the KASLR code consistent with
> Address space markers hints in [1]
>
> [1] http://lxr.free-electrons.com/source/arch/x86/mm/dump_pagetables.c#L82
>
> Reviewed-by: Bhupesh Sharma <bhsharma@redhat.com>
>
> Regards,
> Bhupesh



-- 
Thomas

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Bhupesh Sharma]

On Wed, Mar 8, 2017 at 3:05 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
>> Yes, it looks better. I can repost with this change. Thanks.
>
> No it doesn't:
>
> #define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
> #define EFI_VA_END       (-68 * (_AC(1, UL) << 30))
>
> That's -4G (the shift by 30) and -68G, respectively.
>
>> > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
>> > #define EFI_VA_END    _AC(0xffffffef00000000, UL)
>
> That is something which I need to type into a calculator first.

Right, my point was that this -4G convention stands out, as compared
to the rest of the addressing convention used throughout
'arch/x86/include/asm/pgtable_64_types.h'.

For e.g.:

#define __VMALLOC_BASE    _AC(0xffffc90000000000, UL)
#define __VMEMMAP_BASE    _AC(0xffffea0000000000, UL)
..
#define MODULES_END      _AC(0xffffffffff000000, UL)
..
and so on.

Also it seems inconsistent to the convention used in
'Documentation/x86/x86_64/mm.txt'

As you noted in one of your other comments in this thread, this causes
a confusion as to whether the EFI_VA_END and EFI_VA_START macros need
to be swapped or the comments elsewhere in the x86 code which use
these MACROS are incorrect.

However, I think it is more a matter of code readability and each of
these styles have their own advantages.

I would be happy to cook up a patch to have uniformity in the
addressing conventions used across pgtable_64_types.h (if it is
required at all), but it would be a different topic and not related to
this patch.

Regards,
Bhupesh

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Bhupesh Sharma]

On Wed, Mar 8, 2017 at 3:05 PM, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
> On Wed, Mar 08, 2017 at 05:09:55PM +0800, Baoquan He wrote:
>> Yes, it looks better. I can repost with this change. Thanks.
>
> No it doesn't:
>
> #define EFI_VA_START     ( -4 * (_AC(1, UL) << 30))
> #define EFI_VA_END       (-68 * (_AC(1, UL) << 30))
>
> That's -4G (the shift by 30) and -68G, respectively.
>
>> > #define EFI_VA_START    _AC(0xfffffffeffffffff, UL)
>> > #define EFI_VA_END    _AC(0xffffffef00000000, UL)
>
> That is something which I need to type into a calculator first.

Right, my point was that this -4G convention stands out, as compared
to the rest of the addressing convention used throughout
'arch/x86/include/asm/pgtable_64_types.h'.

For e.g.:

#define __VMALLOC_BASE    _AC(0xffffc90000000000, UL)
#define __VMEMMAP_BASE    _AC(0xffffea0000000000, UL)
..
#define MODULES_END      _AC(0xffffffffff000000, UL)
..
and so on.

Also it seems inconsistent to the convention used in
'Documentation/x86/x86_64/mm.txt'

As you noted in one of your other comments in this thread, this causes
a confusion as to whether the EFI_VA_END and EFI_VA_START macros need
to be swapped or the comments elsewhere in the x86 code which use
these MACROS are incorrect.

However, I think it is more a matter of code readability and each of
these styles have their own advantages.

I would be happy to cook up a patch to have uniformity in the
addressing conventions used across pgtable_64_types.h (if it is
required at all), but it would be a different topic and not related to
this patch.

Regards,
Bhupesh

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Dave Young]

On 03/08/17 at 11:50am, Borislav Petkov wrote:
> On Wed, Mar 08, 2017 at 06:17:50PM +0800, Baoquan He wrote:
> > All right, I will just update the code comment. Just back ported kaslr
> > to our OS product, people reviewed and found the upper boundary of kaslr
> > mm region is EFI_VA_START, that's not correct, it has to be corrected
> > firstly in upstream. Then found the confusion in code comment.
> 
>         BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
> +                    vaddr_end >= EFI_VA_END);
> 
> so I think that once we've done the mapping, we won't need anymore VA
> space so we could simply check the range [efi_va, EFI_VA_START] instead.
> 
> However, that won't work currently because evi_va is not valid at
> build time. And it won't work at boot time either because, AFAICT,
> kernel_randomize_memory() runs before efi_enter_virtual_mode() so ...
> 
> So yours is probably OK.
> 
> I guess what's confusing there is the naming - EFI_VA_START and
> EFI_VA_END. They're kinda swapped because of the direction we take when
> we start mapping runtime services, i.e., from the higher (unsigned)
> address to lower.
> 
> I guess we could swap the naming so that it doesn't confuse people but
> that would be up to EFI maintainers.
> 
> Then stuff like that:
> 
> # ifdef CONFIG_EFI
>         { EFI_VA_END,           "EFI Runtime Services" },
> # endif
> 
> will make more sense when they are:
> 
> # ifdef CONFIG_EFI
>         { EFI_VA_START,           "EFI Runtime Services" },
> # endif
> 
> But changing it now could confuse more people who have the current
> mental picture of the mapping direction so I'd vote for the simple fix
> above.

People should understand the meaning of the macro then use it correctly,
one should not assume START == lower address unless they are sure. 

> 
> Again, as previously, this is a maintainer decision.
> 

Personally I think current way is just fine, but agreed it is up to efi
maintainer. 

Thanks
Dave

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Dave Young]

On 03/08/17 at 11:50am, Borislav Petkov wrote:
> On Wed, Mar 08, 2017 at 06:17:50PM +0800, Baoquan He wrote:
> > All right, I will just update the code comment. Just back ported kaslr
> > to our OS product, people reviewed and found the upper boundary of kaslr
> > mm region is EFI_VA_START, that's not correct, it has to be corrected
> > firstly in upstream. Then found the confusion in code comment.
> 
>         BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_ESPFIX64) &&
> +                    vaddr_end >= EFI_VA_END);
> 
> so I think that once we've done the mapping, we won't need anymore VA
> space so we could simply check the range [efi_va, EFI_VA_START] instead.
> 
> However, that won't work currently because evi_va is not valid at
> build time. And it won't work at boot time either because, AFAICT,
> kernel_randomize_memory() runs before efi_enter_virtual_mode() so ...
> 
> So yours is probably OK.
> 
> I guess what's confusing there is the naming - EFI_VA_START and
> EFI_VA_END. They're kinda swapped because of the direction we take when
> we start mapping runtime services, i.e., from the higher (unsigned)
> address to lower.
> 
> I guess we could swap the naming so that it doesn't confuse people but
> that would be up to EFI maintainers.
> 
> Then stuff like that:
> 
> # ifdef CONFIG_EFI
>         { EFI_VA_END,           "EFI Runtime Services" },
> # endif
> 
> will make more sense when they are:
> 
> # ifdef CONFIG_EFI
>         { EFI_VA_START,           "EFI Runtime Services" },
> # endif
> 
> But changing it now could confuse more people who have the current
> mental picture of the mapping direction so I'd vote for the simple fix
> above.

People should understand the meaning of the macro then use it correctly,
one should not assume START == lower address unless they are sure. 

> 
> Again, as previously, this is a maintainer decision.
> 

Personally I think current way is just fine, but agreed it is up to efi
maintainer. 

Thanks
Dave

Re: [PATCH 1/2] x86/efi: Correct a tiny mistake in code comment

[Dave Young]

Hi,

On 03/08/17 at 04:45pm, Baoquan He wrote:
> Forgot cc to Boris, add him.
> 
> On 03/08/17 at 04:18pm, Dave Young wrote:
> > On 03/08/17 at 03:47pm, Baoquan He wrote:
> > > EFI allocate runtime services regions down from EFI_VA_START, -4G.
> > > It should be top-down handling.
> > > 
> > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > ---
> > >  arch/x86/platform/efi/efi_64.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > index a4695da..6cbf9e0 100644
> > > --- a/arch/x86/platform/efi/efi_64.c
> > > +++ b/arch/x86/platform/efi/efi_64.c
> > > @@ -47,7 +47,7 @@
> > >  #include <asm/pgalloc.h>
> > >  
> > >  /*
> > > - * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> > > + * We allocate runtime services regions top-down, starting from -4G, i.e.
> > 
> > Baoquan, I think original bottom-up is right, it is just considering
> > -68G as up, see the x86_64 mm.txt. We regard vmalloc as higher address
> > although from mathematics view it is lower then positive addresses.
> 
> Thanks for reviewing!
> 
> I am not sure. Just in efi_map_region() it gets the starting va to map
> 'size' big of region by below code:
> 	efi_va -= size;
> 
> -4G and -68G just a trick which makes people understand easily, still we
> think kernel text mapping region is in higher addr area then vmalloc. I
> personnally think.

I understand your points, there is not right or wrong. So I think drop
the words like the change in your V2 looks good.

Thanks
Dave

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Baoquan He]

PING!

Is there any suggestion for this code bug fix?

Boris added comment in patch 1/2 thread that it can also be fixed by
swapping the naming - EFI_VA_START and EFI_VA_END. As he said the
swapping can remove the confusion about the naming, while the con is
changing it now could confuse more people who have the current
mental picture of the mapping direction.

And there's also a well known similar use case, stack, like stack_end
naming in arch/x86/boot/main.c which is the low addr boundary of stack
region.

Any idea?

Thanks
Baoquan

On 03/08/17 at 03:47pm, Baoquan He wrote:
> EFI allocates runtime services regions top-down, starting from EFI_VA_START
> to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
> EFI region. The upper boundary of memory regions randomized by KASLR should
> be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
> 
> Correct it in this patch.
> 
> Signed-off-by: Baoquan He <bhe@redhat.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
> 

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Baoquan He]

PING!

Is there any suggestion for this code bug fix?

Boris added comment in patch 1/2 thread that it can also be fixed by
swapping the naming - EFI_VA_START and EFI_VA_END. As he said the
swapping can remove the confusion about the naming, while the con is
changing it now could confuse more people who have the current
mental picture of the mapping direction.

And there's also a well known similar use case, stack, like stack_end
naming in arch/x86/boot/main.c which is the low addr boundary of stack
region.

Any idea?

Thanks
Baoquan

On 03/08/17 at 03:47pm, Baoquan He wrote:
> EFI allocates runtime services regions top-down, starting from EFI_VA_START
> to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
> EFI region. The upper boundary of memory regions randomized by KASLR should
> be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
> 
> Correct it in this patch.
> 
> Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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
> 

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Baoquan He]

On 03/15/17 at 02:13pm, Baoquan He wrote:
> PING!
> 
> Is there any suggestion for this code bug fix?
> 
> Boris added comment in patch 1/2 thread that it can also be fixed by
> swapping the naming - EFI_VA_START and EFI_VA_END. As he said the
> swapping can remove the confusion about the naming, while the con is
> changing it now could confuse more people who have the current
> mental picture of the mapping direction.

If swapping the naming is suggested, I can post v2 to change efi code.
Both of them is fine to me.

> 
> And there's also a well known similar use case, stack, like stack_end
> naming in arch/x86/boot/main.c which is the low addr boundary of stack
> region.
> 
> Any idea?
> 
> Thanks
> Baoquan
> 
> On 03/08/17 at 03:47pm, Baoquan He wrote:
> > EFI allocates runtime services regions top-down, starting from EFI_VA_START
> > to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
> > EFI region. The upper boundary of memory regions randomized by KASLR should
> > be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
> > 
> > Correct it in this patch.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.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
> > 

Re: [PATCH 2/2] x86/mm/KASLR: Correct the upper boundary of KALSR mm regions if adjacent to EFI

[Baoquan He]

On 03/15/17 at 02:13pm, Baoquan He wrote:
> PING!
> 
> Is there any suggestion for this code bug fix?
> 
> Boris added comment in patch 1/2 thread that it can also be fixed by
> swapping the naming - EFI_VA_START and EFI_VA_END. As he said the
> swapping can remove the confusion about the naming, while the con is
> changing it now could confuse more people who have the current
> mental picture of the mapping direction.

If swapping the naming is suggested, I can post v2 to change efi code.
Both of them is fine to me.

> 
> And there's also a well known similar use case, stack, like stack_end
> naming in arch/x86/boot/main.c which is the low addr boundary of stack
> region.
> 
> Any idea?
> 
> Thanks
> Baoquan
> 
> On 03/08/17 at 03:47pm, Baoquan He wrote:
> > EFI allocates runtime services regions top-down, starting from EFI_VA_START
> > to EFI_VA_END. So EFI_VA_START is bigger than EFI_VA_END and is the end of
> > EFI region. The upper boundary of memory regions randomized by KASLR should
> > be EFI_VA_END if it's adjacent to EFI region, but not EFI_VA_START.
> > 
> > Correct it in this patch.
> > 
> > Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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
> >