[v3,3/3] module: Make __tracepoints_ptrs as read-only
diff mbox series

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

Commit Message

Joel Fernandes April 10, 2019, 7:57 p.m. UTC
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(+)

Comments

Steven Rostedt April 10, 2019, 8:11 p.m. UTC | #1
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)
Joel Fernandes April 10, 2019, 8:29 p.m. UTC | #2
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
Steven Rostedt April 11, 2019, 12:44 a.m. UTC | #3
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
Joel Fernandes April 11, 2019, 8:21 a.m. UTC | #4
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)					\
Steven Rostedt April 11, 2019, 1:19 p.m. UTC | #5
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
Paul E. McKenney April 11, 2019, 1:44 p.m. UTC | #6
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)					\
>
Joel Fernandes April 11, 2019, 8:06 p.m. UTC | #7
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
Jessica Yu April 17, 2019, 3:16 p.m. UTC | #8
+++ 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
Paul E. McKenney April 17, 2019, 3:36 p.m. UTC | #9
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
Joel Fernandes April 20, 2019, 11:38 a.m. UTC | #10
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

Patch
diff mbox series

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)