diff mbox series

[DEMO] um: demonstrate super early constructors

Message ID 20200206204937.db680692f366.Ie05fc2ad440c8db59c1519194aeac356469efa72@changeid
State Not Applicable
Headers show
Series [DEMO] um: demonstrate super early constructors | expand

Commit Message

Johannes Berg Feb. 6, 2020, 7:49 p.m. UTC
From: Johannes Berg <johannes.berg@intel.com>

For KASAN, we need super early constructors, demonstrate
how we could perhaps do that.

I'm not sure why we have two places with .init_array in the
linker scripts, the common one does get included, but then
the dyn one seems to override it, perhaps for alignment? but
it seems it would be harmless to do the alignment in the
common one ... not sure.

---
 arch/um/include/asm/common.lds.S |  1 +
 arch/um/kernel/dyn.lds.S         |  5 ++++-
 arch/um/kernel/mem.c             | 15 +++++++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

Comments

Johannes Berg Feb. 6, 2020, 7:55 p.m. UTC | #1
Now,


> +static void early_print(void)
> +{
> +	printf("I'm super early, before constructors\n");
> +}
[...]
> +static void (*early_print_ptr)(void)
> +__attribute__((section(".kasan_init"), used))
> + = early_print;

As you can see from the section name, I was obviously thinking of KASAN
here :-)

You had the following kasan_init function, and doing it this early will
be a bit problematic:

> +void kasan_init(void)
> +{
> +       kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);

This should be OK. I actually thought we'd have to know the memory size
even for this, but it looks like not, which is great! I guess you just
made it have a mapping that's basically large enough for everything.

> +       // unpoison the kernel text which is form uml_physmem -> uml_reserved
> +       kasan_unpoison_shadow((void *)uml_physmem, physmem_size);

This I guess you'd have to defer until they're actually set up, but
that's OK - we can only use them after that. So just moving that to
wherever uml_physmem/physmem_size are initialized should be OK.

> +       // unpoison the vmalloc region, which is start_vm -> end_vm
> +       kasan_unpoison_shadow((void *)start_vm, (end_vm - start_vm + 1));

This I'm not sure about, but again - I suppose kasan_unpoison_shadow()
really only needs the shadow memory, so we can do it whenever we
actually init the start_vm/end_vm variables (i.e. in linux_main()).

> +       init_task.kasan_depth = 0;

I guess that's OK, though it should be 0-initialized anyway?

> +       pr_info("KernelAddressSanitizer initialized\n");

That might be a bit dangerous this early in the init process, but I
guess we don't really need it :-) Can always print something much later
saying that yes, you have KASAN enabled.

johannes
Patricia Alfonso Feb. 7, 2020, 12:31 a.m. UTC | #2
On Thu, Feb 6, 2020 at 11:55 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Now,
>
>
> > +static void early_print(void)
> > +{
> > +     printf("I'm super early, before constructors\n");
> > +}
> [...]
> > +static void (*early_print_ptr)(void)
> > +__attribute__((section(".kasan_init"), used))
> > + = early_print;
>
> As you can see from the section name, I was obviously thinking of KASAN
> here :-)
>
> You had the following kasan_init function, and doing it this early will
> be a bit problematic:
>
With my new changes, this actually works great! Thank you!

> > +void kasan_init(void)
> > +{
> > +       kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
>
> This should be OK. I actually thought we'd have to know the memory size
> even for this, but it looks like not, which is great! I guess you just
> made it have a mapping that's basically large enough for everything.
>
> > +       // unpoison the kernel text which is form uml_physmem -> uml_reserved
> > +       kasan_unpoison_shadow((void *)uml_physmem, physmem_size);
>
> This I guess you'd have to defer until they're actually set up, but
> that's OK - we can only use them after that. So just moving that to
> wherever uml_physmem/physmem_size are initialized should be OK.
>
> > +       // unpoison the vmalloc region, which is start_vm -> end_vm
> > +       kasan_unpoison_shadow((void *)start_vm, (end_vm - start_vm + 1));
>
> This I'm not sure about, but again - I suppose kasan_unpoison_shadow()
> really only needs the shadow memory, so we can do it whenever we
> actually init the start_vm/end_vm variables (i.e. in linux_main()).
>
These are actually extraneous lines of code, as pointed out by Dmitry
Vyukov, because mmap
always gives zeroed memory so they're no longer a problem.

Thank you so much for this patch!! I have been trying to find an
elegant fix all week and this is perfect!
Johannes Berg Feb. 10, 2020, 7:24 p.m. UTC | #3
Hi,

> Thank you so much for this patch!! I have been trying to find an
> elegant fix all week and this is perfect!

Glad it helped :-)

johannes
diff mbox series

Patch

diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
index eca6c452a41b..731f8c8422a2 100644
--- a/arch/um/include/asm/common.lds.S
+++ b/arch/um/include/asm/common.lds.S
@@ -83,6 +83,7 @@ 
   }
   .init_array : {
 	__init_array_start = .;
+	*(.kasan_init)
 	*(.init_array)
 	__init_array_end = .;
   }
diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
index f5001481010c..4f0447cd9e38 100644
--- a/arch/um/kernel/dyn.lds.S
+++ b/arch/um/kernel/dyn.lds.S
@@ -103,7 +103,10 @@  SECTIONS
      be empty, which isn't pretty.  */
   . = ALIGN(32 / 8);
   .preinit_array     : { *(.preinit_array) }
-  .init_array     : { *(.init_array) }
+  .init_array     : {
+	*(.kasan_init)
+	*(.init_array)
+  }
   .fini_array     : { *(.fini_array) }
   .data           : {
     INIT_TASK_DATA(KERNEL_STACK_SIZE)
diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
index 30885d0b94ac..32fc941c80f7 100644
--- a/arch/um/kernel/mem.c
+++ b/arch/um/kernel/mem.c
@@ -19,6 +19,21 @@ 
 #include <mem_user.h>
 #include <os.h>
 
+extern int printf(const char *msg, ...);
+static void early_print(void)
+{
+	printf("I'm super early, before constructors\n");
+}
+
+static void __attribute__((constructor)) constructor_test(void)
+{
+	printf("yes, you can see it\n");
+}
+
+static void (*early_print_ptr)(void)
+__attribute__((section(".kasan_init"), used))
+ = early_print;
+
 /* allocated in paging_init, zeroed in mem_init, and unchanged thereafter */
 unsigned long *empty_zero_page = NULL;
 EXPORT_SYMBOL(empty_zero_page);