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: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Suggested-by: [email protected]
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
kernel/module.c | 42 ++++++++++++++++++++++++------------------
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 524da609c884..f9221381d076 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3300,11 +3300,28 @@ 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 char *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",
+ NULL
+};
+
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 +3336,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; ro_after_init_sections[i]; 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
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: [email protected]
Suggested-by: [email protected]
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
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, 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: [email protected]
> Suggested-by: [email protected]
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
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)" <[email protected]> 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)" <[email protected]> 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 <[email protected]> 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) <[email protected]>
-- Steve
On Tue, Apr 9, 2019 at 10:48 PM Steven Rostedt <[email protected]> wrote:
>
> On Tue, 9 Apr 2019 22:41:03 -0400
> Joel Fernandes <[email protected]> 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) <[email protected]>
>
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 [email protected] wrote:
> On Tue, Apr 9, 2019 at 10:48 PM Steven Rostedt <[email protected]> wrote:
>>
>> On Tue, 9 Apr 2019 22:41:03 -0400
>> Joel Fernandes <[email protected]> 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) <[email protected]>
>>
>
> 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
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Tue, Apr 9, 2019 at 6:14 PM Joel Fernandes (Google)
<[email protected]> 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: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Suggested-by: [email protected]
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
>
> ---
> kernel/module.c | 42 ++++++++++++++++++++++++------------------
> 1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 524da609c884..f9221381d076 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3300,11 +3300,28 @@ 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 char *ro_after_init_sections[] = {
static const char * const ... Need to make sure the table and its
strings can't be changed. :)
> + ".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",
> + NULL
Since this table is known at build time, you don't need a NULL
terminator, you can use ARRAY_SIZE() instead.
> +};
> +
> 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 +3336,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; ro_after_init_sections[i]; i++) {
i.e. 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
Otherwise, yeah, looks good to me.
--
Kees Cook
On Wed, Apr 10, 2019 at 09:26:44AM -0700, Kees Cook wrote:
> On Tue, Apr 9, 2019 at 6:14 PM Joel Fernandes (Google)
> <[email protected]> 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: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Suggested-by: [email protected]
> > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> >
> > ---
> > kernel/module.c | 42 ++++++++++++++++++++++++------------------
> > 1 file changed, 24 insertions(+), 18 deletions(-)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 524da609c884..f9221381d076 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3300,11 +3300,28 @@ 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 char *ro_after_init_sections[] = {
>
> static const char * const ... Need to make sure the table and its
> strings can't be changed. :)
Will fix :)
> > + ".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",
> > + NULL
>
> Since this table is known at build time, you don't need a NULL
> terminator, you can use ARRAY_SIZE() instead.
Ok, sounds good. You are absolutely right.
> > +};
> > +
> > 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 +3336,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; ro_after_init_sections[i]; i++) {
>
> i.e. for (i = 0; i < ARRAY_SIZE(ro_after_init_sections); i++)
Yep.
> > + 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
>
> Otherwise, yeah, looks good to me.
Thanks, if you are Ok with it, I will add your Reviewed-by tag as well.
- Joel
On Wed, Apr 10, 2019 at 10:48 AM Joel Fernandes <[email protected]> wrote:
> Thanks, if you are Ok with it, I will add your Reviewed-by tag as well.
With those fixes, absolutely. :) Thanks!
--
Kees Cook
On Wed, 10 Apr 2019 11:21:23 -0700
Kees Cook <[email protected]> wrote:
> On Wed, Apr 10, 2019 at 10:48 AM Joel Fernandes <[email protected]> wrote:
> > Thanks, if you are Ok with it, I will add your Reviewed-by tag as well.
>
> With those fixes, absolutely. :) Thanks!
>
I'll wait for v2 before adding my reviewed-by. ;-)
-- Steve