| Message ID | 20190410195708.162185-3-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] module: Prepare for addition of new ro_after_init sections | expand |
On Wed, 10 Apr 2019 15:57:08 -0400 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote: > This series hardens the tracepoints in modules by making the array of > pointers referring to the tracepoints as read-only. This array is needed > during module unloading to verify that the tracepoint is quiescent. > There is no reason for the array to be to be writable after init, and > can cause security or other hidden bugs. Mark these as ro_after_init. > > Suggested-by: paulmck@linux.vnet.ibm.com > Suggested-by: keescook@chromium.org > Suggested-by: mathieu.desnoyers@efficios.com > Cc: rostedt@goodmis.org > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > kernel/module.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/kernel/module.c b/kernel/module.c > index 8b9631e789f0..be980aaa8804 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3320,6 +3320,12 @@ static const char * const ro_after_init_sections[] = { > * by the SRCU notifiers > */ > "___srcu_struct_ptrs", > + > + /* > + * Array of tracepoint pointers used for checking if tracepoints are > + * quiescent during unloading. > + */ > + "__tracepoints_ptrs", Do we ever modify the __tracepoint_ptrs section? I know the jump_label sections are sorted on load, which means they need to be writable during init, but if __tracepoint_ptrs is not sorted or touched during load, why not just put them in the rodata section to begin with? -- Steve > }; > > static struct module *layout_and_allocate(struct load_info *info, int flags)
On Wed, Apr 10, 2019 at 04:11:12PM -0400, Steven Rostedt wrote: > On Wed, 10 Apr 2019 15:57:08 -0400 > "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote: > > > This series hardens the tracepoints in modules by making the array of > > pointers referring to the tracepoints as read-only. This array is needed > > during module unloading to verify that the tracepoint is quiescent. > > There is no reason for the array to be to be writable after init, and > > can cause security or other hidden bugs. Mark these as ro_after_init. > > > > Suggested-by: paulmck@linux.vnet.ibm.com > > Suggested-by: keescook@chromium.org > > Suggested-by: mathieu.desnoyers@efficios.com > > Cc: rostedt@goodmis.org > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > --- > > kernel/module.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/kernel/module.c b/kernel/module.c > > index 8b9631e789f0..be980aaa8804 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -3320,6 +3320,12 @@ static const char * const ro_after_init_sections[] = { > > * by the SRCU notifiers > > */ > > "___srcu_struct_ptrs", > > + > > + /* > > + * Array of tracepoint pointers used for checking if tracepoints are > > + * quiescent during unloading. > > + */ > > + "__tracepoints_ptrs", > > Do we ever modify the __tracepoint_ptrs section? I know the jump_label > sections are sorted on load, which means they need to be writable > during init, but if __tracepoint_ptrs is not sorted or touched during > load, why not just put them in the rodata section to begin with? > > -- Steve The srcu structure pointer array is modified at module load time because the array is fixed up by the module loader at load-time with the final locations of the tracepoints right? Basically relocation fixups. At compile time, I believe it is not know what the values in the ptr array are. I believe same is true for the tracepoint ptrs array. Also it needs to be in a separate __tracepoint_ptrs so that this code works: #ifdef CONFIG_TRACEPOINTS mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs", sizeof(*mod->tracepoints_ptrs), &mod->num_tracepoints); #endif Did I miss some point? Thanks, - Joel
On Wed, 10 Apr 2019 16:29:02 -0400 Joel Fernandes <joel@joelfernandes.org> wrote: > The srcu structure pointer array is modified at module load time because the > array is fixed up by the module loader at load-time with the final locations > of the tracepoints right? Basically relocation fixups. At compile time, I > believe it is not know what the values in the ptr array are. I believe same > is true for the tracepoint ptrs array. > > Also it needs to be in a separate __tracepoint_ptrs so that this code works: > > > #ifdef CONFIG_TRACEPOINTS > mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs", > sizeof(*mod->tracepoints_ptrs), > &mod->num_tracepoints); > #endif > > Did I miss some point? Thanks, But there's a lot of others too. Hmm, does this mean that the RO data sections that are in modules are not set to RO? There's a bunch of separate sections that are RO. Just look in include/asm-generic/vmlinux.lds.h under the RO_DATA_SECTION() macro. A lot of the sections saved in module.c:find_module_sections() are in that RO_DATA when compiled as a builtin. Are they not RO when loaded via a module? If this is the case, there probably is going to be a lot more sections added to your list. -- Steve
On Wed, Apr 10, 2019 at 08:44:01PM -0400, Steven Rostedt wrote: > On Wed, 10 Apr 2019 16:29:02 -0400 > Joel Fernandes <joel@joelfernandes.org> wrote: > > > The srcu structure pointer array is modified at module load time because the > > array is fixed up by the module loader at load-time with the final locations > > of the tracepoints right? Basically relocation fixups. At compile time, I > > believe it is not know what the values in the ptr array are. I believe same > > is true for the tracepoint ptrs array. > > > > Also it needs to be in a separate __tracepoint_ptrs so that this code works: > > > > > > #ifdef CONFIG_TRACEPOINTS > > mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs", > > sizeof(*mod->tracepoints_ptrs), > > &mod->num_tracepoints); > > #endif > > > > Did I miss some point? Thanks, > > But there's a lot of others too. Hmm, does this mean that the RO data > sections that are in modules are not set to RO? > > There's a bunch of separate sections that are RO. Just look in > include/asm-generic/vmlinux.lds.h under the RO_DATA_SECTION() macro. > > A lot of the sections saved in module.c:find_module_sections() are in > that RO_DATA when compiled as a builtin. Are they not RO when loaded via > a module? > > If this is the case, there probably is going to be a lot more sections > added to your list. Hi Steven, You are right. It turns out that this patch for tracepoint is not needed since each tracepoint pointer is marked as a const which automatically makes the section non-writable after relocations.. #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS typedef const int tracepoint_ptr_t; #else typedef struct tracepoint * const tracepoint_ptr_t; #endif So the fix for SRCU could just be the following. I verified with the change that the ELF section section is marked only with the ALLOCATE flag, not the WRITE flag which should automatically put the srcu pointer array in rodata. I'll test this out tomorrow. Patch 2/3 and 3/3 would not be nececessary if this works out. 1/3 may be a nice clean up but is not something urgent and we could do that in the future if needed. Any thoughts? Thanks a lot for the review! (I believe it is still worth auditing other sections in built-in RODATA and making sure they are non-writable for modules as well). ---8<----------------------- diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h index 8af1824c46a8..9cfcc8a756ae 100644 --- a/include/linux/srcutree.h +++ b/include/linux/srcutree.h @@ -123,7 +123,7 @@ struct srcu_struct { #ifdef MODULE # define __DEFINE_SRCU(name, is_static) \ is_static struct srcu_struct name; \ - struct srcu_struct *__srcu_struct_##name \ + struct srcu_struct * const __srcu_struct_##name \ __section("___srcu_struct_ptrs") = &name #else # define __DEFINE_SRCU(name, is_static) \
On Thu, 11 Apr 2019 04:21:06 -0400 Joel Fernandes <joel@joelfernandes.org> wrote: > Patch 2/3 and 3/3 would not be nececessary if this works out. 1/3 may be a > nice clean up but is not something urgent and we could do that in the future > if needed. Well, jump_labels is "special" because it requires sorting the RO data and is done via module notify. The only other user that had to modify RO data on module load is ftrace. It had to do the nop conversions in the text area. It use to do it via module notify, but because of the hardening of the kernel, doing it there was no longer possible because everything was RO then. The solution was to call into ftrace directly from the module code instead of a notifier. This was done before sections were made RO. One option is to do the same with jump_labels and just have a call to the sorting before the notifiers and before the section gets turned into RO. Or I would say just leave it as is. As I stated, jump_labels are "special" and adding a loop of one section where I don't envision any other sections needing to do the same thing for a long time to come. I would save that patch for if there is another section that comes along that needs to be modify at module notify. -- Steve
On Thu, Apr 11, 2019 at 04:21:06AM -0400, Joel Fernandes wrote: > On Wed, Apr 10, 2019 at 08:44:01PM -0400, Steven Rostedt wrote: > > On Wed, 10 Apr 2019 16:29:02 -0400 > > Joel Fernandes <joel@joelfernandes.org> wrote: > > > > > The srcu structure pointer array is modified at module load time because the > > > array is fixed up by the module loader at load-time with the final locations > > > of the tracepoints right? Basically relocation fixups. At compile time, I > > > believe it is not know what the values in the ptr array are. I believe same > > > is true for the tracepoint ptrs array. > > > > > > Also it needs to be in a separate __tracepoint_ptrs so that this code works: > > > > > > > > > #ifdef CONFIG_TRACEPOINTS > > > mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs", > > > sizeof(*mod->tracepoints_ptrs), > > > &mod->num_tracepoints); > > > #endif > > > > > > Did I miss some point? Thanks, > > > > But there's a lot of others too. Hmm, does this mean that the RO data > > sections that are in modules are not set to RO? > > > > There's a bunch of separate sections that are RO. Just look in > > include/asm-generic/vmlinux.lds.h under the RO_DATA_SECTION() macro. > > > > A lot of the sections saved in module.c:find_module_sections() are in > > that RO_DATA when compiled as a builtin. Are they not RO when loaded via > > a module? > > > > If this is the case, there probably is going to be a lot more sections > > added to your list. > > Hi Steven, > > You are right. It turns out that this patch for tracepoint is not needed > since each tracepoint pointer is marked as a const which automatically makes > the section non-writable after relocations.. > > #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS > typedef const int tracepoint_ptr_t; > #else > typedef struct tracepoint * const tracepoint_ptr_t; > #endif > > So the fix for SRCU could just be the following. I verified with the change > that the ELF section section is marked only with the ALLOCATE flag, not the > WRITE flag which should automatically put the srcu pointer array in rodata. > I'll test this out tomorrow. > > Patch 2/3 and 3/3 would not be nececessary if this works out. 1/3 may be a > nice clean up but is not something urgent and we could do that in the future > if needed. > > Any thoughts? Thanks a lot for the review! > > (I believe it is still worth auditing other sections in built-in RODATA and > making sure they are non-writable for modules as well). Nice and simple change! ;-) If it works and Steve is OK with it, I will be happy to take the corresponding formal patch. Thanx, Paul > ---8<----------------------- > > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h > index 8af1824c46a8..9cfcc8a756ae 100644 > --- a/include/linux/srcutree.h > +++ b/include/linux/srcutree.h > @@ -123,7 +123,7 @@ struct srcu_struct { > #ifdef MODULE > # define __DEFINE_SRCU(name, is_static) \ > is_static struct srcu_struct name; \ > - struct srcu_struct *__srcu_struct_##name \ > + struct srcu_struct * const __srcu_struct_##name \ > __section("___srcu_struct_ptrs") = &name > #else > # define __DEFINE_SRCU(name, is_static) \ >
On Thu, Apr 11, 2019 at 09:19:55AM -0400, Steven Rostedt wrote: > On Thu, 11 Apr 2019 04:21:06 -0400 > Joel Fernandes <joel@joelfernandes.org> wrote: > > > Patch 2/3 and 3/3 would not be nececessary if this works out. 1/3 may be a > > nice clean up but is not something urgent and we could do that in the future > > if needed. > > Well, jump_labels is "special" because it requires sorting the RO data > and is done via module notify. The only other user that had to modify > RO data on module load is ftrace. It had to do the nop conversions in > the text area. It use to do it via module notify, but because of the > hardening of the kernel, doing it there was no longer possible because > everything was RO then. The solution was to call into ftrace directly > from the module code instead of a notifier. This was done before > sections were made RO. > > One option is to do the same with jump_labels and just have a call to > the sorting before the notifiers and before the section gets turned > into RO. Or I would say just leave it as is. As I stated, jump_labels > are "special" and adding a loop of one section where I don't envision > any other sections needing to do the same thing for a long time to > come. I would save that patch for if there is another section that > comes along that needs to be modify at module notify. Sounds good, thanks for the detailed history of this. It sounds like you are Ok with making of the srcu pointer array as const, which I will test shortly and send it out for your review. And we can drop this patch series for now. thanks, - Joel
+++ Steven Rostedt [10/04/19 20:44 -0400]: >On Wed, 10 Apr 2019 16:29:02 -0400 >Joel Fernandes <joel@joelfernandes.org> wrote: > >> The srcu structure pointer array is modified at module load time because the >> array is fixed up by the module loader at load-time with the final locations >> of the tracepoints right? Basically relocation fixups. At compile time, I >> believe it is not know what the values in the ptr array are. I believe same >> is true for the tracepoint ptrs array. >> >> Also it needs to be in a separate __tracepoint_ptrs so that this code works: >> >> >> #ifdef CONFIG_TRACEPOINTS >> mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs", >> sizeof(*mod->tracepoints_ptrs), >> &mod->num_tracepoints); >> #endif >> >> Did I miss some point? Thanks, > >But there's a lot of others too. Hmm, does this mean that the RO data >sections that are in modules are not set to RO? > >There's a bunch of separate sections that are RO. Just look in >include/asm-generic/vmlinux.lds.h under the RO_DATA_SECTION() macro. > >A lot of the sections saved in module.c:find_module_sections() are in >that RO_DATA when compiled as a builtin. Are they not RO when loaded via >a module? Unlike the kernel, the module loader does not rely on a linker script to determine which sections get what protections. On module load, all sections in a module are looped through and those sections without the SHF_WRITE flag will be set to RO. For example, when there is a section filled with structs declared as const or if the section was explicitly given only the SHF_ALLOC attribute, those will be read-only. As long as the sections were given the correct section attributes for read-only, it'll have read-only protection. I see this is already the case for __param and __ksymtab*/__kcrctab* sections, but I agree that a full audit would be useful to be consistent with builtin RO protections. Hope that helps, Jessica
On Wed, Apr 17, 2019 at 05:16:18PM +0200, Jessica Yu wrote: > +++ Steven Rostedt [10/04/19 20:44 -0400]: > >On Wed, 10 Apr 2019 16:29:02 -0400 > >Joel Fernandes <joel@joelfernandes.org> wrote: > > > >>The srcu structure pointer array is modified at module load time because the > >>array is fixed up by the module loader at load-time with the final locations > >>of the tracepoints right? Basically relocation fixups. At compile time, I > >>believe it is not know what the values in the ptr array are. I believe same > >>is true for the tracepoint ptrs array. > >> > >>Also it needs to be in a separate __tracepoint_ptrs so that this code works: > >> > >> > >>#ifdef CONFIG_TRACEPOINTS > >> mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs", > >> sizeof(*mod->tracepoints_ptrs), > >> &mod->num_tracepoints); > >>#endif > >> > >>Did I miss some point? Thanks, > > > >But there's a lot of others too. Hmm, does this mean that the RO data > >sections that are in modules are not set to RO? > > > >There's a bunch of separate sections that are RO. Just look in > >include/asm-generic/vmlinux.lds.h under the RO_DATA_SECTION() macro. > > > >A lot of the sections saved in module.c:find_module_sections() are in > >that RO_DATA when compiled as a builtin. Are they not RO when loaded via > >a module? > > Unlike the kernel, the module loader does not rely on a linker script > to determine which sections get what protections. On module load, all > sections in a module are looped through and those sections without the > SHF_WRITE flag will be set to RO. For example, when there is a section > filled with structs declared as const or if the section was explicitly > given only the SHF_ALLOC attribute, those will be read-only. As long > as the sections were given the correct section attributes for > read-only, it'll have read-only protection. I see this is already the > case for __param and __ksymtab*/__kcrctab* sections, but I agree that > a full audit would be useful to be consistent with builtin RO > protections. Thank you very much for the explanation! Thanx, Paul
On Wed, Apr 17, 2019 at 05:16:18PM +0200, Jessica Yu wrote: > +++ Steven Rostedt [10/04/19 20:44 -0400]: > > On Wed, 10 Apr 2019 16:29:02 -0400 > > Joel Fernandes <joel@joelfernandes.org> wrote: > > > > > The srcu structure pointer array is modified at module load time because the > > > array is fixed up by the module loader at load-time with the final locations > > > of the tracepoints right? Basically relocation fixups. At compile time, I > > > believe it is not know what the values in the ptr array are. I believe same > > > is true for the tracepoint ptrs array. > > > > > > Also it needs to be in a separate __tracepoint_ptrs so that this code works: > > > > > > > > > #ifdef CONFIG_TRACEPOINTS > > > mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs", > > > sizeof(*mod->tracepoints_ptrs), > > > &mod->num_tracepoints); > > > #endif > > > > > > Did I miss some point? Thanks, > > > > But there's a lot of others too. Hmm, does this mean that the RO data > > sections that are in modules are not set to RO? > > > > There's a bunch of separate sections that are RO. Just look in > > include/asm-generic/vmlinux.lds.h under the RO_DATA_SECTION() macro. > > > > A lot of the sections saved in module.c:find_module_sections() are in > > that RO_DATA when compiled as a builtin. Are they not RO when loaded via > > a module? > > Unlike the kernel, the module loader does not rely on a linker script > to determine which sections get what protections. On module load, all > sections in a module are looped through and those sections without the > SHF_WRITE flag will be set to RO. For example, when there is a section > filled with structs declared as const or if the section was explicitly > given only the SHF_ALLOC attribute, those will be read-only. As long > as the sections were given the correct section attributes for > read-only, it'll have read-only protection. I see this is already the > case for __param and __ksymtab*/__kcrctab* sections, but I agree that > a full audit would be useful to be consistent with builtin RO > protections. Thanks a lot for the explanations. Yes we dropped the patches because const worked. This is good to know for future such ventures as well ;-) Best, - Joel > Jessica
diff --git a/kernel/module.c b/kernel/module.c index 8b9631e789f0..be980aaa8804 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3320,6 +3320,12 @@ static const char * const ro_after_init_sections[] = { * by the SRCU notifiers */ "___srcu_struct_ptrs", + + /* + * Array of tracepoint pointers used for checking if tracepoints are + * quiescent during unloading. + */ + "__tracepoints_ptrs", }; static struct module *layout_and_allocate(struct load_info *info, int flags)
This series hardens the tracepoints in modules by making the array of pointers referring to the tracepoints as read-only. This array is needed during module unloading to verify that the tracepoint is quiescent. There is no reason for the array to be to be writable after init, and can cause security or other hidden bugs. Mark these as ro_after_init. Suggested-by: paulmck@linux.vnet.ibm.com Suggested-by: keescook@chromium.org Suggested-by: mathieu.desnoyers@efficios.com Cc: rostedt@goodmis.org Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- kernel/module.c | 6 ++++++ 1 file changed, 6 insertions(+)