Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1398338 > unrolled thread

[PATCH v8 1/4] x86/KASLR: Clarify identity map interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v8 1/4] x86/KASLR: Clarify identity map interface Kees Cook <keescook@chromium.org> - 2016-05-10 19:20 +0200
    Re: [PATCH v8 1/4] x86/KASLR: Clarify identity map interface Borislav Petkov <bp@suse.de> - 2016-05-10 19:50 +0200
    Re: [PATCH v8 1/4] x86/KASLR: Clarify identity map interface Ingo Molnar <mingo@kernel.org> - 2016-05-11 08:30 +0200
      Re: [PATCH v8 1/4] x86/KASLR: Clarify identity map interface Kees Cook <keescook@chromium.org> - 2016-05-11 17:30 +0200
        Re: [PATCH v8 1/4] x86/KASLR: Clarify identity map interface Ingo Molnar <mingo@kernel.org> - 2016-05-12 10:40 +0200
        Re: [PATCH v8 1/4] x86/KASLR: Clarify identity map interface Ingo Molnar <mingo@kernel.org> - 2016-05-12 10:40 +0200

#1398338 — [PATCH v8 1/4] x86/KASLR: Clarify identity map interface

This extracts the call to prepare_level4() into a top-level function
that the user of the pagetable.c interface must call to initialize
the new page tables. For clarity and to match the "finalize" function,
it has been renamed to initialize_identity_maps(). This function also
gains the initialization of mapping_info so we don't have to do it each
time in add_identity_map().

Additionally add copyright notice to the top, to make it clear that the
bulk of the pagetable.c code was written by Yinghai, and that I just
added bugs later. :)

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/boot/compressed/kaslr.c     |  3 +++
 arch/x86/boot/compressed/misc.h      |  3 +++
 arch/x86/boot/compressed/pagetable.c | 26 ++++++++++++++++----------
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index cfeb0259ed81..03a6f5d85a6b 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -485,6 +485,9 @@ unsigned char *choose_random_location(unsigned long input,
 
 	boot_params->hdr.loadflags |= KASLR_FLAG;
 
+	/* Prepare to add new identity pagetables on demand. */
+	initialize_identity_maps();
+
 	/* Record the various known unsafe memory ranges. */
 	mem_avoid_init(input, input_size, output);
 
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index b6fec1ff10e4..09c4ddd02ac6 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -85,10 +85,13 @@ unsigned char *choose_random_location(unsigned long input_ptr,
 #endif
 
 #ifdef CONFIG_X86_64
+void initialize_identity_maps(void);
 void add_identity_map(unsigned long start, unsigned long size);
 void finalize_identity_maps(void);
 extern unsigned char _pgtable[];
 #else
+static inline void initialize_identity_maps(void)
+{ }
 static inline void add_identity_map(unsigned long start, unsigned long size)
 { }
 static inline void finalize_identity_maps(void)
diff --git a/arch/x86/boot/compressed/pagetable.c b/arch/x86/boot/compressed/pagetable.c
index 34b95df14e69..ea4ee6abc0d2 100644
--- a/arch/x86/boot/compressed/pagetable.c
+++ b/arch/x86/boot/compressed/pagetable.c
@@ -2,6 +2,9 @@
  * This code is used on x86_64 to create page table identity mappings on
  * demand by building up a new set of page tables (or appending to the
  * existing ones), and then switching over to them when ready.
+ *
+ * Copyright (C) 2015-2016  Yinghai Lu
+ * Copyright (C)      2016  Kees Cook
  */
 
 /*
@@ -59,9 +62,21 @@ static struct alloc_pgt_data pgt_data;
 /* The top level page table entry pointer. */
 static unsigned long level4p;
 
+/*
+ * Mapping information structure passed to kernel_ident_mapping_init().
+ * Due to relocation, pointers must be assigned at run time not build time.
+ */
+static struct x86_mapping_info mapping_info = {
+	.pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
+};
+
 /* Locates and clears a region for a new top level page table. */
-static void prepare_level4(void)
+void initialize_identity_maps(void)
 {
+	/* Init mapping_info with run-time function/buffer pointers. */
+	mapping_info.alloc_pgt_page = alloc_pgt_page;
+	mapping_info.context = &pgt_data;
+
 	/*
 	 * It should be impossible for this not to already be true,
 	 * but since calling this a second time would rewind the other
@@ -96,17 +111,8 @@ static void prepare_level4(void)
  */
 void add_identity_map(unsigned long start, unsigned long size)
 {
-	struct x86_mapping_info mapping_info = {
-		.alloc_pgt_page	= alloc_pgt_page,
-		.context	= &pgt_data,
-		.pmd_flag	= __PAGE_KERNEL_LARGE_EXEC,
-	};
 	unsigned long end = start + size;
 
-	/* Make sure we have a top level page table ready to use. */
-	if (!level4p)
-		prepare_level4();
-
 	/* Align boundary to 2M. */
 	start = round_down(start, PMD_SIZE);
 	end = round_up(end, PMD_SIZE);
-- 
2.6.3

[toc] | [next] | [standalone]

#1398363

On Tue, May 10, 2016 at 10:19:12AM -0700, Kees Cook wrote:
> This extracts the call to prepare_level4() into a top-level function
> that the user of the pagetable.c interface must call to initialize
> the new page tables. For clarity and to match the "finalize" function,
> it has been renamed to initialize_identity_maps(). This function also
> gains the initialization of mapping_info so we don't have to do it each
> time in add_identity_map().
> 
> Additionally add copyright notice to the top, to make it clear that the
> bulk of the pagetable.c code was written by Yinghai, and that I just
> added bugs later. :)
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/x86/boot/compressed/kaslr.c     |  3 +++
>  arch/x86/boot/compressed/misc.h      |  3 +++
>  arch/x86/boot/compressed/pagetable.c | 26 ++++++++++++++++----------
>  3 files changed, 22 insertions(+), 10 deletions(-)

Exactly!

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1398711

* Kees Cook <keescook@chromium.org> wrote:

> +/*
> + * Mapping information structure passed to kernel_ident_mapping_init().
> + * Due to relocation, pointers must be assigned at run time not build time.
> + */
> +static struct x86_mapping_info mapping_info = {
> +	.pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
> +};

> +void initialize_identity_maps(void)
>  {
> +	/* Init mapping_info with run-time function/buffer pointers. */
> +	mapping_info.alloc_pgt_page = alloc_pgt_page;
> +	mapping_info.context = &pgt_data;

Could you please outline the precise failure mode? What gets executed when, which 
pointer gets relocated and which not, and exactly when does it pose a problem, 
etc.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1399253

On Tue, May 10, 2016 at 11:24 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> +/*
>> + * Mapping information structure passed to kernel_ident_mapping_init().
>> + * Due to relocation, pointers must be assigned at run time not build time.
>> + */
>> +static struct x86_mapping_info mapping_info = {
>> +     .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
>> +};
>
>> +void initialize_identity_maps(void)
>>  {
>> +     /* Init mapping_info with run-time function/buffer pointers. */
>> +     mapping_info.alloc_pgt_page = alloc_pgt_page;
>> +     mapping_info.context = &pgt_data;
>
> Could you please outline the precise failure mode? What gets executed when, which
> pointer gets relocated and which not, and exactly when does it pose a problem,
> etc.

It's the issue described at the top of misc.c:

/*
 * WARNING!!
 * This code is compiled with -fPIC and it is relocated dynamically at
 * run time, but no relocation processing is performed. This means that
 * it is not safe to place pointers in static structures.
 */

Should this be repeated in each .c file maybe as a reminder?

If the rest of the patches look good, we could clean this up as a
following patch? What do you think?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1399747

* Kees Cook <keescook@chromium.org> wrote:

> On Tue, May 10, 2016 at 11:24 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Kees Cook <keescook@chromium.org> wrote:
> >
> >> +/*
> >> + * Mapping information structure passed to kernel_ident_mapping_init().
> >> + * Due to relocation, pointers must be assigned at run time not build time.
> >> + */
> >> +static struct x86_mapping_info mapping_info = {
> >> +     .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
> >> +};
> >
> >> +void initialize_identity_maps(void)
> >>  {
> >> +     /* Init mapping_info with run-time function/buffer pointers. */
> >> +     mapping_info.alloc_pgt_page = alloc_pgt_page;
> >> +     mapping_info.context = &pgt_data;
> >
> > Could you please outline the precise failure mode? What gets executed when, which
> > pointer gets relocated and which not, and exactly when does it pose a problem,
> > etc.
> 
> It's the issue described at the top of misc.c:
> 
> /*
>  * WARNING!!
>  * This code is compiled with -fPIC and it is relocated dynamically at
>  * run time, but no relocation processing is performed. This means that
>  * it is not safe to place pointers in static structures.

So I think this is still a bit confusing: what's the difference between 'relocated 
dynamically at run time' and 'relocation processing'?

So 'to relocate code' usually means the whole deal: 'copy code and fix up'.

The problem here is that we copy the code to an address not known at build time, 
but don't fix it up (because this is the fixup code), right?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1399753

* Kees Cook <keescook@chromium.org> wrote:

> On Tue, May 10, 2016 at 11:24 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Kees Cook <keescook@chromium.org> wrote:
> >
> >> +/*
> >> + * Mapping information structure passed to kernel_ident_mapping_init().
> >> + * Due to relocation, pointers must be assigned at run time not build time.
> >> + */
> >> +static struct x86_mapping_info mapping_info = {
> >> +     .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
> >> +};
> >
> >> +void initialize_identity_maps(void)
> >>  {
> >> +     /* Init mapping_info with run-time function/buffer pointers. */
> >> +     mapping_info.alloc_pgt_page = alloc_pgt_page;
> >> +     mapping_info.context = &pgt_data;
> >
> > Could you please outline the precise failure mode? What gets executed when, which
> > pointer gets relocated and which not, and exactly when does it pose a problem,
> > etc.
> 
> It's the issue described at the top of misc.c:
> 
> /*
>  * WARNING!!
>  * This code is compiled with -fPIC and it is relocated dynamically at
>  * run time, but no relocation processing is performed. This means that
>  * it is not safe to place pointers in static structures.
>  */

Btw., is there any way to detect such invalid pointers during build-time somehow? 
This is not the first such bug and I suspect it won't be the last one either.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web