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

Message ID 20190410011418.76408-2-joel@joelfernandes.org
State New
Headers show
Series
  • [1/2] module: Prepare for addition of new ro_after_init sections
Related show

Commit Message

Joel Fernandes April 10, 2019, 1:14 a.m. UTC
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(-)

Comments

Joel Fernandes April 10, 2019, 1:17 a.m. UTC | #1
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
>
Steven Rostedt April 10, 2019, 2:38 a.m. UTC | #2
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[] = {
Joel Fernandes April 10, 2019, 2:41 a.m. UTC | #3
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[] = {
Steven Rostedt April 10, 2019, 2:48 a.m. UTC | #4
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
Joel Fernandes April 10, 2019, 3:38 a.m. UTC | #5
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
Mathieu Desnoyers April 10, 2019, 4:20 a.m. UTC | #6
----- 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

Patch
diff mbox series

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
 };