[PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Yinghai Lu]

While trying to optimize setup_data handling code, found commit f47233c2d34f
("x86/mm/ASLR: Propagate base load address calculation"), uses a physical
address as the value.

It leads to wrong kaslr status. That kaslr status is passed for controlling
module load offset. So when kernel has kasl support compiled in, and user
use nokaslr to disable kaslr, wrong value cause unwanted offset for module
loading.

We should keep kaslr status consistent between aslr randrom kernel base,
and late module random offset. That is old behavior before that commit.

The setup_data linked list and thus the element which contains
kaslr_enabled is chained together using physical addresses. At the time
when we access it in the kernel, we're already running with paging enabled
and therefore must access it through its virtual address.

This patch changes the code to use early_memmap() and access the value,
as it is still in kernel early stage, we don't have kernel mapping setup
yet.

Fixes: f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Jiri Kosina <jkosina@suse.cz>
Acked-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
-v3: add checking return from early_memmap according to Boris.
-v4: add description about setup_data accessing from Boris to change log.

---
 arch/x86/kernel/setup.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -429,7 +429,18 @@ static void __init reserve_initrd(void)
 
 static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
 {
-	kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
+	/* kaslr_setup_data is defined in aslr.c */
+	unsigned char *data;
+	unsigned long offset = sizeof(struct setup_data);
+
+	data = early_memremap(pa_data, offset + 1);
+	if (!data) {
+		kaslr_enabled = true;
+		return;
+	}
+
+	kaslr_enabled = *(data + offset);
+	early_memunmap(data, offset + 1);
 }
 
 static void __init parse_setup_data(void)

Re: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Minfei Huang]

On 03/15/15 at 12:49am, Yinghai Lu wrote:
> While trying to optimize setup_data handling code, found commit f47233c2d34f
> ("x86/mm/ASLR: Propagate base load address calculation"), uses a physical
> address as the value.
> 
> It leads to wrong kaslr status. That kaslr status is passed for controlling
> module load offset. So when kernel has kasl support compiled in, and user
> use nokaslr to disable kaslr, wrong value cause unwanted offset for module
> loading.
> 

Hi, Yinghai!

It confuses me with the virtual address in function parse_kaslr_setup.
When we are in here(parse_kaslr_setup), we already use the virtual
address, instead of physical address. Is it all right?

In the other words, using physical address in parse_kaslr_setup is
always a mistake, whatever the kaslr is on or off.

Thanks
Minfei

> We should keep kaslr status consistent between aslr randrom kernel base,
> and late module random offset. That is old behavior before that commit.
> 
> The setup_data linked list and thus the element which contains
> kaslr_enabled is chained together using physical addresses. At the time
> when we access it in the kernel, we're already running with paging enabled
> and therefore must access it through its virtual address.
> 
> This patch changes the code to use early_memmap() and access the value,
> as it is still in kernel early stage, we don't have kernel mapping setup
> yet.
> 
> Fixes: f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Acked-by: Jiri Kosina <jkosina@suse.cz>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
> -v3: add checking return from early_memmap according to Boris.
> -v4: add description about setup_data accessing from Boris to change log.
> 
> ---
>  arch/x86/kernel/setup.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/arch/x86/kernel/setup.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/setup.c
> +++ linux-2.6/arch/x86/kernel/setup.c
> @@ -429,7 +429,18 @@ static void __init reserve_initrd(void)
>  
>  static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
>  {
> -	kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
> +	/* kaslr_setup_data is defined in aslr.c */
> +	unsigned char *data;
> +	unsigned long offset = sizeof(struct setup_data);
> +
> +	data = early_memremap(pa_data, offset + 1);
> +	if (!data) {
> +		kaslr_enabled = true;
> +		return;
> +	}
> +
> +	kaslr_enabled = *(data + offset);
> +	early_memunmap(data, offset + 1);
>  }
>  
>  static void __init parse_setup_data(void)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Yinghai Lu]

On Sun, Mar 15, 2015 at 5:18 AM, Minfei Huang <mhuang@redhat.com> wrote:
> On 03/15/15 at 12:49am, Yinghai Lu wrote:
> It confuses me with the virtual address in function parse_kaslr_setup.
> When we are in here(parse_kaslr_setup), we already use the virtual
> address, instead of physical address. Is it all right?

setup_data is using physical address to have linked list.

we have setup_arch==>parse_setup_data(), that is way before
init_mem_mapping() to have final kernel mapping setup yet.

For 64bit, we may use virtual address with help of early mapping with
#PF handler.
But 32bit, we don't have that.

So just use early_memmap to get virtual address to access the value.

>
> In the other words, using physical address in parse_kaslr_setup is
> always a mistake, whatever the kaslr is on or off.
>

The problem is: old code just use physical address as value.

Thanks

Yinghai

Re: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Minfei Huang]

On 03/15/15 at 10:02am, Yinghai Lu wrote:
> On Sun, Mar 15, 2015 at 5:18 AM, Minfei Huang <mhuang@redhat.com> wrote:
> > On 03/15/15 at 12:49am, Yinghai Lu wrote:
> > It confuses me with the virtual address in function parse_kaslr_setup.
> > When we are in here(parse_kaslr_setup), we already use the virtual
> > address, instead of physical address. Is it all right?
> 
> setup_data is using physical address to have linked list.
> 
> we have setup_arch==>parse_setup_data(), that is way before
> init_mem_mapping() to have final kernel mapping setup yet.
> 
> For 64bit, we may use virtual address with help of early mapping with
> #PF handler.
> But 32bit, we don't have that.
> 
> So just use early_memmap to get virtual address to access the value.
> 

Hi, Yinghai!

Thank you for your reply.
I find that the variate kaslr_setup_data is a global variate. Always we
can use physical address to access the value.

But it is more comfortable that we use the early_memmap to access the
value.

Thanks
Minfei

> >
> > In the other words, using physical address in parse_kaslr_setup is
> > always a mistake, whatever the kaslr is on or off.
> >
> 
> The problem is: old code just use physical address as value.
> 
> Thanks
> 
> Yinghai

Re: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Baoquan He]

On 03/15/15 at 12:49am, Yinghai Lu wrote:

> Index: linux-2.6/arch/x86/kernel/setup.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/setup.c
> +++ linux-2.6/arch/x86/kernel/setup.c
> @@ -429,7 +429,18 @@ static void __init reserve_initrd(void)
>  
>  static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
>  {
> -	kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
> +	/* kaslr_setup_data is defined in aslr.c */
> +	unsigned char *data;
> +	unsigned long offset = sizeof(struct setup_data);
> +
> +	data = early_memremap(pa_data, offset + 1);
> +	if (!data) {

It's good to check the ret value as Boris suggested. However it could
fail since early_memremap self fail, e.g slot not found. In this case
making kaslr_enabled true may not be good.

As Minfei talked with you kaslr_setup_data is a global variable inside
kernel code, it has been ident mapped. Just derefencing the physical
address which is virtual address too and getting the real stored value
may be safer. And also parse_kaslr_setup is a function specified to
handle kaslr, it doesn't make me uncomfortable to implement with a
specific knowledge which here means the setup_data is a global varialbe
in kernel code and no need to do early_memremap since mapping has been
built .

Thanks
Baoquan

> +		kaslr_enabled = true;
> +		return;
> +	}
> +
> +	kaslr_enabled = *(data + offset);
> +	early_memunmap(data, offset + 1);
>  }
>  
>  static void __init parse_setup_data(void)

Re: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Yinghai Lu]

On Sun, Mar 15, 2015 at 8:28 PM, Baoquan He <bhe@redhat.com> wrote:
> On 03/15/15 at 12:49am, Yinghai Lu wrote:
> It's good to check the ret value as Boris suggested. However it could
> fail since early_memremap self fail, e.g slot not found. In this case
> making kaslr_enabled true may not be good.
>
> As Minfei talked with you kaslr_setup_data is a global variable inside
> kernel code, it has been ident mapped. Just derefencing the physical
> address which is virtual address too and getting the real stored value
> may be safer. And also parse_kaslr_setup is a function specified to
> handle kaslr, it doesn't make me uncomfortable to implement with a
> specific knowledge which here means the setup_data is a global varialbe
> in kernel code and no need to do early_memremap since mapping has been
> built .

I should not put that checking in. even parse_setup_data does not check
early_memmap return.

Thanks

Yinghai

Re: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Baoquan He]

On 03/15/15 at 09:36pm, Yinghai Lu wrote:
> On Sun, Mar 15, 2015 at 8:28 PM, Baoquan He <bhe@redhat.com> wrote:
> > On 03/15/15 at 12:49am, Yinghai Lu wrote:
> > It's good to check the ret value as Boris suggested. However it could
> > fail since early_memremap self fail, e.g slot not found. In this case
> > making kaslr_enabled true may not be good.
> >
> > As Minfei talked with you kaslr_setup_data is a global variable inside
> > kernel code, it has been ident mapped. Just derefencing the physical
> > address which is virtual address too and getting the real stored value
> > may be safer. And also parse_kaslr_setup is a function specified to
> > handle kaslr, it doesn't make me uncomfortable to implement with a
> > specific knowledge which here means the setup_data is a global varialbe
> > in kernel code and no need to do early_memremap since mapping has been
> > built .
> 
> I should not put that checking in. even parse_setup_data does not check
> early_memmap return.

Yes, I think so. Otherwise it involves incorrect handling. 

Still I think no need to do early_memremap. Because kaslr_setup_data is
ident mapped, it's not like other setup_data info which is passed in
from bootloader. But I am fine too if only do early_memremap
without checking.


Thanks
Baoquan

Re: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Yinghai Lu]

On Sun, Mar 15, 2015 at 8:28 PM, Baoquan He <bhe@redhat.com> wrote:
> On 03/15/15 at 12:49am, Yinghai Lu wrote:
>
> It's good to check the ret value as Boris suggested. However it could
> fail since early_memremap self fail, e.g slot not found. In this case
> making kaslr_enabled true may not be good.

It should not fail. we always follow map/access/unmap. and sometime
would have two for copying between them.

>
> As Minfei talked with you kaslr_setup_data is a global variable inside
> kernel code, it has been ident mapped. Just derefencing the physical
> address which is virtual address too and getting the real stored value
> may be safer.

No.
That ident mapping is set in arch/x86/kernel/head_64.S and it
is only for switchover. and it is gone when
arch/x86/kernel/head64.c::x86_64_start_kernel/reset_early_page_tables
is called.

That reset_early_page_tables will only keep kernel high mapping
and clear all other.

Thanks

Yinghai

Re: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable

[Baoquan He]

On 03/15/15 at 10:04pm, Yinghai Lu wrote:
> On Sun, Mar 15, 2015 at 8:28 PM, Baoquan He <bhe@redhat.com> wrote:
> > On 03/15/15 at 12:49am, Yinghai Lu wrote:
> >
> > It's good to check the ret value as Boris suggested. However it could
> > fail since early_memremap self fail, e.g slot not found. In this case
> > making kaslr_enabled true may not be good.
> 
> It should not fail. we always follow map/access/unmap. and sometime
> would have two for copying between them.

Well, then I would say if add checking too in other places for
setup_data handling, e.g parse_setup_data you mentioned. Or don't check
at all place to make them consistent. 

> 
> >
> > As Minfei talked with you kaslr_setup_data is a global variable inside
> > kernel code, it has been ident mapped. Just derefencing the physical
> > address which is virtual address too and getting the real stored value
> > may be safer.
> 
> No.
> That ident mapping is set in arch/x86/kernel/head_64.S and it
> is only for switchover. and it is gone when
> arch/x86/kernel/head64.c::x86_64_start_kernel/reset_early_page_tables
> is called.
> 
> That reset_early_page_tables will only keep kernel high mapping
> and clear all other.

Ah, yes. You are right. I didn't get this clearly, Thanks for telling.

Thanks
Baoquan

> 
> Thanks
> 
> Yinghai