| Message ID | 20190410011418.76408-2-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] module: Prepare for addition of new ro_after_init sections | expand |
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 >
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[] = {
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[] = {
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
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
----- 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
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 };
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(-)