[PATCH 1/2] module: Prepare for addition of new ro_after_init sections

[PATCH 1/2] module: Prepare for addition of new ro_after_init sections

[Joel Fernandes (Google)]

For the purposes of hardening modules by adding sections to
ro_after_init sections, prepare for addition of new ro_after_init
entries which we do in future patches. Create a table to which new
entries could be added later. This makes it less error prone and reduce
code duplication.

Cc: paulmck@linux.vnet.ibm.com
Cc: rostedt@goodmis.org
Cc: mathieu.desnoyers@efficios.com
Cc: rcu@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
Cc: kernel-team@android.com
Suggested-by: keescook@chromium.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

---
 kernel/module.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 524da609c884..f9221381d076 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3300,11 +3300,28 @@ static bool blacklisted(const char *module_name)
 }
 core_param(module_blacklist, module_blacklist, charp, 0400);
 
+/*
+ * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
+ * layout_sections() can put it in the right place.
+ * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
+ */
+static char *ro_after_init_sections[] = {
+	".data..ro_after_init",
+
+	/*
+	 * __jump_table structures are never modified, with the exception of
+	 * entries that refer to code in the __init section, which are
+	 * annotated as such at module load time.
+	 */
+	"__jump_table",
+	NULL
+};
+
 static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
 	struct module *mod;
 	unsigned int ndx;
-	int err;
+	int err, i;
 
 	err = check_modinfo(info->mod, info, flags);
 	if (err)
@@ -3319,23 +3336,12 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	/* We will do a special allocation for per-cpu sections later. */
 	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
 
-	/*
-	 * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
-	 * layout_sections() can put it in the right place.
-	 * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
-	 */
-	ndx = find_sec(info, ".data..ro_after_init");
-	if (ndx)
-		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
-	/*
-	 * Mark the __jump_table section as ro_after_init as well: these data
-	 * structures are never modified, with the exception of entries that
-	 * refer to code in the __init section, which are annotated as such
-	 * at module load time.
-	 */
-	ndx = find_sec(info, "__jump_table");
-	if (ndx)
-		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
+	/* Set sh_flags for read-only after init sections */
+	for (i = 0; ro_after_init_sections[i]; i++) {
+		ndx = find_sec(info, ro_after_init_sections[i]);
+		if (ndx)
+			info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
+	}
 
 	/* Determine total sizes, and put offsets in sh_entsize.  For now
 	   this is done generically; there doesn't appear to be any
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH 2/2] module: Make srcu_struct ptr array as read-only post init

[Joel Fernandes (Google)]

Since commit title ("srcu: Allocate per-CPU data for DEFINE_SRCU() in
modules"), modules that call DEFINE_{STATIC,}SRCU will have a new array
of srcu_struct pointers which is used by srcu code to initialize and
clean up these structures.

There is no reason for this array of pointers to be writable, and can
cause security or other hidden bugs. Mark these are read-only after the
module init has completed.

Suggested-by: paulmck@linux.vnet.ibm.com
Suggested-by: keescook@chromium.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

---
 kernel/module.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index f9221381d076..ed1f2612aebc 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3301,7 +3301,7 @@ static bool blacklisted(const char *module_name)
 core_param(module_blacklist, module_blacklist, charp, 0400);
 
 /*
- * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
+ * These are section names marked with SHF_RO_AFTER_INIT so that
  * layout_sections() can put it in the right place.
  * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
  */
@@ -3314,6 +3314,13 @@ static char *ro_after_init_sections[] = {
 	 * annotated as such at module load time.
 	 */
 	"__jump_table",
+
+	/*
+	 * Used for SRCU structures which need to be initialized/cleaned up
+	 * by the SRCU notifiers
+	 */
+	"___srcu_struct_ptrs",
+
 	NULL
 };
 
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH 2/2] module: Make srcu_struct ptr array as read-only post init

[Joel Fernandes]

On Tue, Apr 09, 2019 at 09:14:18PM -0400, Joel Fernandes (Google) wrote:
> Since commit title ("srcu: Allocate per-CPU data for DEFINE_SRCU() in
> modules"), modules that call DEFINE_{STATIC,}SRCU will have a new array
> of srcu_struct pointers which is used by srcu code to initialize and
> clean up these structures.
> 
> There is no reason for this array of pointers to be writable, and can
> cause security or other hidden bugs. Mark these are read-only after the
> module init has completed.
> 
> Suggested-by: paulmck@linux.vnet.ibm.com
> Suggested-by: keescook@chromium.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Just wanted to mention, that I tested that the srcu_struct_ptrs array is not
writeable any more after init, but doing the following after
module_enable_ro() :

@@ -3513,6 +3523,14 @@ static noinline int do_init_module(struct module *mod)
        rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
 #endif
        module_enable_ro(mod, true);
+
+       if (mod->srcu_struct_ptrs) {
+               // Check if SRCU Access is possible
+               char x = *(char *)mod->srcu_struct_ptrs;
+               *(char *)mod->srcu_struct_ptrs = 0;
+               *(char *)mod->srcu_struct_ptrs = x;
+       }
+
        mod_tree_remove_init(mod);
        disable_ro_nx(&mod->init_layout);
        module_arch_freeing_init(mod);


thanks,

 - Joel

> 
> ---
>  kernel/module.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index f9221381d076..ed1f2612aebc 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3301,7 +3301,7 @@ static bool blacklisted(const char *module_name)
>  core_param(module_blacklist, module_blacklist, charp, 0400);
>  
>  /*
> - * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> + * These are section names marked with SHF_RO_AFTER_INIT so that
>   * layout_sections() can put it in the right place.
>   * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
>   */
> @@ -3314,6 +3314,13 @@ static char *ro_after_init_sections[] = {
>  	 * annotated as such at module load time.
>  	 */
>  	"__jump_table",
> +
> +	/*
> +	 * Used for SRCU structures which need to be initialized/cleaned up
> +	 * by the SRCU notifiers
> +	 */
> +	"___srcu_struct_ptrs",
> +
>  	NULL
>  };
>  
> -- 
> 2.21.0.392.gf8f6787159e-goog
> 

Re: [PATCH 2/2] module: Make srcu_struct ptr array as read-only post init

[Steven Rostedt]

On Tue,  9 Apr 2019 21:14:18 -0400
"Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:

>  /*
> - * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> + * These are section names marked with SHF_RO_AFTER_INIT so that

I'm curious to this much of a change. Wouldn't just making "section"
plural also work?

 "Mark ro_after_init sections with ..."

Other than that, the two patches look fine to me.

-- Steve

>   * layout_sections() can put it in the right place.
>   * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
>   */
> @@ -3314,6 +3314,13 @@ static char *ro_after_init_sections[] = {

Re: [PATCH 2/2] module: Make srcu_struct ptr array as read-only post init

[Joel Fernandes]

On Tue, Apr 09, 2019 at 10:38:20PM -0400, Steven Rostedt wrote:
> On Tue,  9 Apr 2019 21:14:18 -0400
> "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> 
> >  /*
> > - * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> > + * These are section names marked with SHF_RO_AFTER_INIT so that
> 
> I'm curious to this much of a change. Wouldn't just making "section"
> plural also work?
> 
>  "Mark ro_after_init sections with ..."

Yes, I will change it to that and actually this comment change should go in
the previous patch so I'll squash it into that.

> Other than that, the two patches look fine to me.

Could I add your Reviewed-by in the respin?

thanks,

 - Joel


> -- Steve
> 
> >   * layout_sections() can put it in the right place.
> >   * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> >   */
> > @@ -3314,6 +3314,13 @@ static char *ro_after_init_sections[] = {

Re: [PATCH 2/2] module: Make srcu_struct ptr array as read-only post init

[Steven Rostedt]

On Tue, 9 Apr 2019 22:41:03 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> > Other than that, the two patches look fine to me.  
> 
> Could I add your Reviewed-by in the respin?

You can add an Acked-by, as I haven't spent enough time to offer a
Reviewed-by tag. ;-)

Maybe I'll get some time to vet it a bit more tomorrow, and then
upgrade the ack to a review.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

Re: [PATCH 2/2] module: Make srcu_struct ptr array as read-only post init

[Joel Fernandes]

On Tue, Apr 9, 2019 at 10:48 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 9 Apr 2019 22:41:03 -0400
> Joel Fernandes <joel@joelfernandes.org> wrote:
>
> > > Other than that, the two patches look fine to me.
> >
> > Could I add your Reviewed-by in the respin?
>
> You can add an Acked-by, as I haven't spent enough time to offer a
> Reviewed-by tag. ;-)
>
> Maybe I'll get some time to vet it a bit more tomorrow, and then
> upgrade the ack to a review.
>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>

Thanks!

Also we could possibly consider adding the tracepoint ptrs array as
well to the list, which would be useful I think, if one were to over
write that array by accident (and the bpf tps related array too).

 - Joel

Re: [PATCH 2/2] module: Make srcu_struct ptr array as read-only post init

[Mathieu Desnoyers]

----- On Apr 9, 2019, at 11:38 PM, Joel Fernandes joelaf@google.com wrote:

> On Tue, Apr 9, 2019 at 10:48 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>> On Tue, 9 Apr 2019 22:41:03 -0400
>> Joel Fernandes <joel@joelfernandes.org> wrote:
>>
>> > > Other than that, the two patches look fine to me.
>> >
>> > Could I add your Reviewed-by in the respin?
>>
>> You can add an Acked-by, as I haven't spent enough time to offer a
>> Reviewed-by tag. ;-)
>>
>> Maybe I'll get some time to vet it a bit more tomorrow, and then
>> upgrade the ack to a review.
>>
>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>
> 
> Thanks!
> 
> Also we could possibly consider adding the tracepoint ptrs array as
> well to the list, which would be useful I think, if one were to over
> write that array by accident (and the bpf tps related array too).

Yes, please!

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 1/2] module: Prepare for addition of new ro_after_init sections

[Kees Cook]

On Tue, Apr 9, 2019 at 6:14 PM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> For the purposes of hardening modules by adding sections to
> ro_after_init sections, prepare for addition of new ro_after_init
> entries which we do in future patches. Create a table to which new
> entries could be added later. This makes it less error prone and reduce
> code duplication.
>
> Cc: paulmck@linux.vnet.ibm.com
> Cc: rostedt@goodmis.org
> Cc: mathieu.desnoyers@efficios.com
> Cc: rcu@vger.kernel.org
> Cc: kernel-hardening@lists.openwall.com
> Cc: kernel-team@android.com
> Suggested-by: keescook@chromium.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> ---
>  kernel/module.c | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 524da609c884..f9221381d076 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3300,11 +3300,28 @@ static bool blacklisted(const char *module_name)
>  }
>  core_param(module_blacklist, module_blacklist, charp, 0400);
>
> +/*
> + * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> + * layout_sections() can put it in the right place.
> + * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> + */
> +static char *ro_after_init_sections[] = {

static const char * const ... Need to make sure the table and its
strings can't be changed. :)

> +       ".data..ro_after_init",
> +
> +       /*
> +        * __jump_table structures are never modified, with the exception of
> +        * entries that refer to code in the __init section, which are
> +        * annotated as such at module load time.
> +        */
> +       "__jump_table",
> +       NULL

Since this table is known at build time, you don't need a NULL
terminator, you can use ARRAY_SIZE() instead.

> +};
> +
>  static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
>         struct module *mod;
>         unsigned int ndx;
> -       int err;
> +       int err, i;
>
>         err = check_modinfo(info->mod, info, flags);
>         if (err)
> @@ -3319,23 +3336,12 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>         /* We will do a special allocation for per-cpu sections later. */
>         info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>
> -       /*
> -        * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> -        * layout_sections() can put it in the right place.
> -        * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> -        */
> -       ndx = find_sec(info, ".data..ro_after_init");
> -       if (ndx)
> -               info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> -       /*
> -        * Mark the __jump_table section as ro_after_init as well: these data
> -        * structures are never modified, with the exception of entries that
> -        * refer to code in the __init section, which are annotated as such
> -        * at module load time.
> -        */
> -       ndx = find_sec(info, "__jump_table");
> -       if (ndx)
> -               info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> +       /* Set sh_flags for read-only after init sections */
> +       for (i = 0; ro_after_init_sections[i]; i++) {

i.e. for (i = 0; i < ARRAY_SIZE(ro_after_init_sections); i++)

> +               ndx = find_sec(info, ro_after_init_sections[i]);
> +               if (ndx)
> +                       info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> +       }
>
>         /* Determine total sizes, and put offsets in sh_entsize.  For now
>            this is done generically; there doesn't appear to be any

Otherwise, yeah, looks good to me.

-- 
Kees Cook

Re: [PATCH 1/2] module: Prepare for addition of new ro_after_init sections

[Joel Fernandes]

On Wed, Apr 10, 2019 at 09:26:44AM -0700, Kees Cook wrote:
> On Tue, Apr 9, 2019 at 6:14 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> >
> > For the purposes of hardening modules by adding sections to
> > ro_after_init sections, prepare for addition of new ro_after_init
> > entries which we do in future patches. Create a table to which new
> > entries could be added later. This makes it less error prone and reduce
> > code duplication.
> >
> > Cc: paulmck@linux.vnet.ibm.com
> > Cc: rostedt@goodmis.org
> > Cc: mathieu.desnoyers@efficios.com
> > Cc: rcu@vger.kernel.org
> > Cc: kernel-hardening@lists.openwall.com
> > Cc: kernel-team@android.com
> > Suggested-by: keescook@chromium.org
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > ---
> >  kernel/module.c | 42 ++++++++++++++++++++++++------------------
> >  1 file changed, 24 insertions(+), 18 deletions(-)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 524da609c884..f9221381d076 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3300,11 +3300,28 @@ static bool blacklisted(const char *module_name)
> >  }
> >  core_param(module_blacklist, module_blacklist, charp, 0400);
> >
> > +/*
> > + * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> > + * layout_sections() can put it in the right place.
> > + * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> > + */
> > +static char *ro_after_init_sections[] = {
> 
> static const char * const ... Need to make sure the table and its
> strings can't be changed. :)

Will fix :)

> > +       ".data..ro_after_init",
> > +
> > +       /*
> > +        * __jump_table structures are never modified, with the exception of
> > +        * entries that refer to code in the __init section, which are
> > +        * annotated as such at module load time.
> > +        */
> > +       "__jump_table",
> > +       NULL
> 
> Since this table is known at build time, you don't need a NULL
> terminator, you can use ARRAY_SIZE() instead.

Ok, sounds good. You are absolutely right.

> > +};
> > +
> >  static struct module *layout_and_allocate(struct load_info *info, int flags)
> >  {
> >         struct module *mod;
> >         unsigned int ndx;
> > -       int err;
> > +       int err, i;
> >
> >         err = check_modinfo(info->mod, info, flags);
> >         if (err)
> > @@ -3319,23 +3336,12 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
> >         /* We will do a special allocation for per-cpu sections later. */
> >         info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
> >
> > -       /*
> > -        * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> > -        * layout_sections() can put it in the right place.
> > -        * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> > -        */
> > -       ndx = find_sec(info, ".data..ro_after_init");
> > -       if (ndx)
> > -               info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> > -       /*
> > -        * Mark the __jump_table section as ro_after_init as well: these data
> > -        * structures are never modified, with the exception of entries that
> > -        * refer to code in the __init section, which are annotated as such
> > -        * at module load time.
> > -        */
> > -       ndx = find_sec(info, "__jump_table");
> > -       if (ndx)
> > -               info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> > +       /* Set sh_flags for read-only after init sections */
> > +       for (i = 0; ro_after_init_sections[i]; i++) {
> 
> i.e. for (i = 0; i < ARRAY_SIZE(ro_after_init_sections); i++)

Yep.

> > +               ndx = find_sec(info, ro_after_init_sections[i]);
> > +               if (ndx)
> > +                       info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> > +       }
> >
> >         /* Determine total sizes, and put offsets in sh_entsize.  For now
> >            this is done generically; there doesn't appear to be any
> 
> Otherwise, yeah, looks good to me.

Thanks, if you are Ok with it, I will add your Reviewed-by tag as well.

 - Joel

Re: [PATCH 1/2] module: Prepare for addition of new ro_after_init sections

[Kees Cook]

On Wed, Apr 10, 2019 at 10:48 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> Thanks, if you are Ok with it, I will add your Reviewed-by tag as well.

With those fixes, absolutely. :) Thanks!

-- 
Kees Cook

Re: [PATCH 1/2] module: Prepare for addition of new ro_after_init sections

[Steven Rostedt]

On Wed, 10 Apr 2019 11:21:23 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Wed, Apr 10, 2019 at 10:48 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > Thanks, if you are Ok with it, I will add your Reviewed-by tag as well.  
> 
> With those fixes, absolutely. :) Thanks!
> 

I'll wait for v2 before adding my reviewed-by. ;-)

-- Steve