x86, kaslr: Access the correct kaslr_enabled variable

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 <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Kees Cook <[email protected]>

Cc Jiri Kosina <[email protected]>

Acked-by Jiri Kosina <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

---

(quoted diff) +0 -3

-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
===================================================================

arch/x86/kernel/setup.c +12 -2

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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?

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

Quoted 4 lines · Minfei Huang

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.

Quoted 4 lines

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

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================

arch/x86/kernel/setup.c +6 -0

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

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 .

Quoted 5 lines · Baoquan He

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.

Quoted 8 lines

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.

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.

Quoted 5 lines · Baoquan He

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.

Quoted 6 lines

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