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

[PATCH v3 1/3] 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
Reviewed-by: keescook@chromium.org
Acked-by: rostedt@goodmis.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

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

diff --git a/kernel/module.c b/kernel/module.c
index 524da609c884..42e4e289d6c7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3300,11 +3300,27 @@ 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 const char * const 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",
+};
+
 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 +3335,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; 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
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v3 2/3] 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
Acked-by: keescook@chromium.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 42e4e289d6c7..8b9631e789f0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3314,6 +3314,12 @@ static const char * const 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",
 };
 
 static struct module *layout_and_allocate(struct load_info *info, int flags)
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Joel Fernandes (Google)]

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",
 };
 
 static struct module *layout_and_allocate(struct load_info *info, int flags)
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Steven Rostedt]

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)

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Joel Fernandes]

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

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Steven Rostedt]

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

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Joel Fernandes]

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)					\

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

[Miroslav Benes]

On Wed, 10 Apr 2019, Joel Fernandes (Google) 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
> Reviewed-by: keescook@chromium.org
> Acked-by: rostedt@goodmis.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Steven Rostedt]

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

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Paul E. McKenney]

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)					\
> 

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Joel Fernandes]

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

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

[Jessica Yu]

+++ Joel Fernandes (Google) [10/04/19 15:57 -0400]:
>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
>Reviewed-by: keescook@chromium.org
>Acked-by: rostedt@goodmis.org
>Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Hi Joel!

Thanks for the patchset! Apologies for the late reply, I've returned
from vacation this week and am catching up on the previous threads..

And if I'm all caught up now, it looks like the plan now is to drop
this patchset in favor of just marking the srcu pointer array const,
is that correct? That should put it in a rodata section (ie., a
section without SHF_WRITE) and the module loader will mark it RO
appropriately after relocations are applied, so it really doesn't have
to be a ro_after_init section then if I'm understanding correctly.

Jessica

>---
> kernel/module.c | 41 +++++++++++++++++++++++------------------
> 1 file changed, 23 insertions(+), 18 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 524da609c884..42e4e289d6c7 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3300,11 +3300,27 @@ 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 const char * const 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",
>+};
>+
> 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 +3335,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; 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
>-- 
>2.21.0.392.gf8f6787159e-goog
>

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Jessica Yu]

+++ 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

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Paul E. McKenney]

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

Re: [PATCH v3 3/3] module: Make __tracepoints_ptrs as read-only

[Joel Fernandes]

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