[v3,1/2] module: verify address is read-only

Commit Message

Eddie Kovsky March 23, 2017, 2:55 a.m. UTC

Implement a mechanism to check if a module's address is in
the rodata or ro_after_init sections. It mimics the exsiting functions
that test if an address is inside a module's text section.

Functions that take a module as an argument will be able to
verify that the module is in a read-only section.

Signed-off-by: Eddie Kovsky <[e-mail]>
---
Changes in v3:
 - Change function name is_module_ro_address() to
is_module_rodata_address().
 - Improve comments on is_module_rodata_address().
 - Add a !CONFIG_MODULES stub for is_module_rodata_address.
 - Correct and simplify the check for the read-only memory regions in
the module address.

 include/linux/module.h | 12 ++++++++++++
 kernel/module.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

--
2.12.0

Comments

Kees Cook March 23, 2017, 9:13 p.m. UTC | #1

On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <[e-mail]> wrote:
> Implement a mechanism to check if a module's address is in
> the rodata or ro_after_init sections. It mimics the exsiting functions
> that test if an address is inside a module's text section.
>
> Functions that take a module as an argument will be able to
> verify that the module is in a read-only section.
>
> Signed-off-by: Eddie Kovsky <[e-mail]>

Awesome! I'll be glad to have these.

Reviewed-by: Kees Cook <[e-mail]>

Jessica, if this looks good to you, should this go via modules or via
my tree? If mine, can I have your Ack?

Thanks!

-Kees

> ---
> Changes in v3:
>  - Change function name is_module_ro_address() to
> is_module_rodata_address().
>  - Improve comments on is_module_rodata_address().
>  - Add a !CONFIG_MODULES stub for is_module_rodata_address.
>  - Correct and simplify the check for the read-only memory regions in
> the module address.
>
>  include/linux/module.h | 12 ++++++++++++
>  kernel/module.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 9ad68561d8c2..66b7fd321334 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -492,7 +492,9 @@ static inline int module_is_live(struct module *mod)
>
>  struct module *__module_text_address(unsigned long addr);
>  struct module *__module_address(unsigned long addr);
> +struct module *__module_ro_address(unsigned long addr);
>  bool is_module_address(unsigned long addr);
> +bool is_module_rodata_address(unsigned long addr);
>  bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
>  bool is_module_percpu_address(unsigned long addr);
>  bool is_module_text_address(unsigned long addr);
> @@ -646,6 +648,16 @@ static inline struct module *__module_address(unsigned long addr)
>         return NULL;
>  }
>
> +static inline struct module *__module_ro_address(unsigned long addr)
> +{
> +       return NULL;
> +}
> +
> +static inline bool is_module_rodata_address(unsigned long addr)
> +{
> +       return false;
> +}
> +
>  static inline struct module *__module_text_address(unsigned long addr)
>  {
>         return NULL;
> diff --git a/kernel/module.c b/kernel/module.c
> index 8ffcd29a4245..99ada1257aaa 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4292,6 +4292,59 @@ struct module *__module_text_address(unsigned long addr)
>  }
>  EXPORT_SYMBOL_GPL(__module_text_address);
>
> +/**
> + * is_module_rodata_address - is this address inside read-only module code?
> + * @addr: the address to check.
> + *
> + */
> +bool is_module_rodata_address(unsigned long addr)
> +{
> +       bool ret;
> +
> +       preempt_disable();
> +       ret = __module_ro_address(addr) != NULL;
> +       preempt_enable();
> +
> +       return ret;
> +}
> +
> +/*
> + * __module_ro_address - get the module whose rodata/ro_after_init sections
> + * contain the given address.
> + * @addr: the address.
> + *
> + * Must be called with preempt disabled or module mutex held so that
> + * module doesn't get freed during this.
> + */
> +struct module *__module_ro_address(unsigned long addr)
> +{
> +       struct module *mod = __module_address(addr);
> +
> +       void *init_base = mod->init_layout.base;
> +       unsigned int init_text_size = mod->init_layout.text_size;
> +       unsigned int init_ro_after_init_size = mod->init_layout.ro_after_init_size;
> +
> +       void *core_base = mod->core_layout.base;
> +       unsigned int core_text_size = mod->core_layout.text_size;
> +       unsigned int core_ro_after_init_size = mod->core_layout.ro_after_init_size;
> +
> +       /*
> +        * Make sure module is within the read-only section.
> +        * range(base + text_size, base + ro_after_init_size)
> +        * encompasses both the rodata and ro_after_init regions.
> +        * See comment above frob_text() for the layout diagram.
> +        */
> +       if (mod) {
> +               if (!within(addr, init_base + init_text_size,
> +                           init_ro_after_init_size - init_text_size)
> +                   && !within(addr, core_base + core_text_size,
> +                              core_ro_after_init_size - core_text_size))
> +                       mod = NULL;
> +       }
> +       return mod;
> +}
> +EXPORT_SYMBOL_GPL(__module_ro_address);
> +
>  /* Don't grab lock, we're oopsing. */
>  void print_modules(void)
>  {
> --
> 2.12.0

Jessica Yu March 25, 2017, 12:40 a.m. UTC | #2

+++ Eddie Kovsky [22/03/17 20:55 -0600]:
>Implement a mechanism to check if a module's address is in
>the rodata or ro_after_init sections. It mimics the exsiting functions
>that test if an address is inside a module's text section.
>
>Functions that take a module as an argument will be able to

>verify that the module is in a read-only section.

s/module/module address/?

Also, there is some useful information in your cover letter on the
context and motivation for adding to the api, it would be good to
reproduce that info here so that we can have it officially in the
changelog. (sentences "This implements a suggestion made by Kees..."
and "The idea is to prevent structures..." would be nice to copy here)

>Signed-off-by: Eddie Kovsky <[e-mail]>
>---
>Changes in v3:
> - Change function name is_module_ro_address() to
>is_module_rodata_address().
> - Improve comments on is_module_rodata_address().
> - Add a !CONFIG_MODULES stub for is_module_rodata_address.
> - Correct and simplify the check for the read-only memory regions in
>the module address.
>
> include/linux/module.h | 12 ++++++++++++
> kernel/module.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 65 insertions(+)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 9ad68561d8c2..66b7fd321334 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -492,7 +492,9 @@ static inline int module_is_live(struct module *mod)
>
> struct module *__module_text_address(unsigned long addr);
> struct module *__module_address(unsigned long addr);
>+struct module *__module_ro_address(unsigned long addr);

Can we rename __module_ro_address to __module_rodata_address (to match
is_module_rodata_address())?

> bool is_module_address(unsigned long addr);
>+bool is_module_rodata_address(unsigned long addr);
> bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
> bool is_module_percpu_address(unsigned long addr);
> bool is_module_text_address(unsigned long addr);
>@@ -646,6 +648,16 @@ static inline struct module *__module_address(unsigned long addr)
> 	return NULL;
> }
>
>+static inline struct module *__module_ro_address(unsigned long addr)
>+{
>+	return NULL;
>+}
>+
>+static inline bool is_module_rodata_address(unsigned long addr)
>+{
>+	return false;
>+}
>+

Style nitpick: Can you move is_module_rodata_address() below is_module_address()?
That way, the function stubs are grouped a bit more consistently.

> static inline struct module *__module_text_address(unsigned long addr)
> {
> 	return NULL;
>diff --git a/kernel/module.c b/kernel/module.c
>index 8ffcd29a4245..99ada1257aaa 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -4292,6 +4292,59 @@ struct module *__module_text_address(unsigned long addr)
> }
> EXPORT_SYMBOL_GPL(__module_text_address);
>
>+/**
>+ * is_module_rodata_address - is this address inside read-only module code?

s/code/data/

>+ * @addr: the address to check.
>+ *
>+ */
>+bool is_module_rodata_address(unsigned long addr)
>+{
>+	bool ret;
>+
>+	preempt_disable();
>+	ret = __module_ro_address(addr) != NULL;
>+	preempt_enable();
>+
>+	return ret;
>+}
>+
>+/*
>+ * __module_ro_address - get the module whose rodata/ro_after_init sections
>+ * contain the given address.

As mentioned in the first comment, let's rename __module_ro_address to
__module_rodata_address, so it mirrors is_module_rodata_address().

>+ * @addr: the address.
>+ *
>+ * Must be called with preempt disabled or module mutex held so that
>+ * module doesn't get freed during this.
>+ */
>+struct module *__module_ro_address(unsigned long addr)
>+{
>+	struct module *mod = __module_address(addr);

We need to check that mod is not NULL before dereferencing it below.

>+	void *init_base = mod->init_layout.base;
>+	unsigned int init_text_size = mod->init_layout.text_size;
>+	unsigned int init_ro_after_init_size = mod->init_layout.ro_after_init_size;
>+
>+	void *core_base = mod->core_layout.base;
>+	unsigned int core_text_size = mod->core_layout.text_size;
>+	unsigned int core_ro_after_init_size = mod->core_layout.ro_after_init_size;
>+
>+	/*
>+	 * Make sure module is within the read-only section.
>+	 * range(base + text_size, base + ro_after_init_size)
>+	 * encompasses both the rodata and ro_after_init regions.
>+	 * See comment above frob_text() for the layout diagram.
>+	 */
>+	if (mod) {
>+		if (!within(addr, init_base + init_text_size,
>+			    init_ro_after_init_size - init_text_size)
>+		    && !within(addr, core_base + core_text_size,
>+			       core_ro_after_init_size - core_text_size))
>+			mod = NULL;
>+	}
>+	return mod;
>+}
>+EXPORT_SYMBOL_GPL(__module_ro_address);
>+
> /* Don't grab lock, we're oopsing. */
> void print_modules(void)
> {
>--
>2.12.0

Jessica Yu March 25, 2017, 12:42 a.m. UTC | #3

+++ Kees Cook [23/03/17 14:13 -0700]:
>On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <[e-mail]> wrote:
>> Implement a mechanism to check if a module's address is in
>> the rodata or ro_after_init sections. It mimics the exsiting functions
>> that test if an address is inside a module's text section.
>>
>> Functions that take a module as an argument will be able to
>> verify that the module is in a read-only section.
>>
>> Signed-off-by: Eddie Kovsky <[e-mail]>
>
>Awesome! I'll be glad to have these.
>
>Reviewed-by: Kees Cook <[e-mail]>
>
>Jessica, if this looks good to you, should this go via modules or via
>my tree? If mine, can I have your Ack?

Sure, would be happy to let you take this patch through your tree. I
only had a few minor comments left, so I think just one more small
respin and it'll be good to go.

Thanks!

Jessica

Eddie Kovsky March 25, 2017, 1:41 a.m. UTC | #4

On 03/24/17, Jessica Yu wrote:
> +++ Eddie Kovsky [22/03/17 20:55 -0600]:
> > Implement a mechanism to check if a module's address is in
> > the rodata or ro_after_init sections. It mimics the exsiting functions
> > that test if an address is inside a module's text section.
> > 
> > Functions that take a module as an argument will be able to
> 
> > verify that the module is in a read-only section.
> 
> s/module/module address/?
> 
Yes, that is more accurate.

> Also, there is some useful information in your cover letter on the
> context and motivation for adding to the api, it would be good to
> reproduce that info here so that we can have it officially in the
> changelog. (sentences "This implements a suggestion made by Kees..."
> and "The idea is to prevent structures..." would be nice to copy here)
> 
Okay, that's easy to add.

Kees, would you like me to add your Suggested-by as well?


> > Signed-off-by: Eddie Kovsky <[e-mail]>
> > ---
> > Changes in v3:
> > - Change function name is_module_ro_address() to
> > is_module_rodata_address().
> > - Improve comments on is_module_rodata_address().
> > - Add a !CONFIG_MODULES stub for is_module_rodata_address.
> > - Correct and simplify the check for the read-only memory regions in
> > the module address.
> > 
> > include/linux/module.h | 12 ++++++++++++
> > kernel/module.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 65 insertions(+)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 9ad68561d8c2..66b7fd321334 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -492,7 +492,9 @@ static inline int module_is_live(struct module *mod)
> > 
> > struct module *__module_text_address(unsigned long addr);
> > struct module *__module_address(unsigned long addr);
> > +struct module *__module_ro_address(unsigned long addr);
> 
> Can we rename __module_ro_address to __module_rodata_address (to match
> is_module_rodata_address())?
> 
Sure, consistency is a good thing.

> > bool is_module_address(unsigned long addr);
> > +bool is_module_rodata_address(unsigned long addr);
> > bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
> > bool is_module_percpu_address(unsigned long addr);
> > bool is_module_text_address(unsigned long addr);
> > @@ -646,6 +648,16 @@ static inline struct module *__module_address(unsigned long addr)
> > 	return NULL;
> > }
> > 
> > +static inline struct module *__module_ro_address(unsigned long addr)
> > +{
> > +	return NULL;
> > +}
> > +
> > +static inline bool is_module_rodata_address(unsigned long addr)
> > +{
> > +	return false;
> > +}
> > +
> 
> Style nitpick: Can you move is_module_rodata_address() below is_module_address()?
> That way, the function stubs are grouped a bit more consistently.
> 
I think I might have shuffled that on the last rebase.

> > static inline struct module *__module_text_address(unsigned long addr)
> > {
> > 	return NULL;
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 8ffcd29a4245..99ada1257aaa 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -4292,6 +4292,59 @@ struct module *__module_text_address(unsigned long addr)
> > }
> > EXPORT_SYMBOL_GPL(__module_text_address);
> > 
> > +/**
> > + * is_module_rodata_address - is this address inside read-only module code?
> 
> s/code/data/
> 
Yes.

> > + * @addr: the address to check.
> > + *
> > + */
> > +bool is_module_rodata_address(unsigned long addr)
> > +{
> > +	bool ret;
> > +
> > +	preempt_disable();
> > +	ret = __module_ro_address(addr) != NULL;
> > +	preempt_enable();
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * __module_ro_address - get the module whose rodata/ro_after_init sections
> > + * contain the given address.
> 
> As mentioned in the first comment, let's rename __module_ro_address to
> __module_rodata_address, so it mirrors is_module_rodata_address().
> 
> > + * @addr: the address.
> > + *
> > + * Must be called with preempt disabled or module mutex held so that
> > + * module doesn't get freed during this.
> > + */
> > +struct module *__module_ro_address(unsigned long addr)
> > +{
> > +	struct module *mod = __module_address(addr);
> 
> We need to check that mod is not NULL before dereferencing it below.
> 
I missed that. The new variables are now using mod before we check it.
I'll fix it on the next round.

> > +	void *init_base = mod->init_layout.base;
> > +	unsigned int init_text_size = mod->init_layout.text_size;
> > +	unsigned int init_ro_after_init_size = mod->init_layout.ro_after_init_size;
> > +
> > +	void *core_base = mod->core_layout.base;
> > +	unsigned int core_text_size = mod->core_layout.text_size;
> > +	unsigned int core_ro_after_init_size = mod->core_layout.ro_after_init_size;
> > +
> > +	/*
> > +	 * Make sure module is within the read-only section.
> > +	 * range(base + text_size, base + ro_after_init_size)
> > +	 * encompasses both the rodata and ro_after_init regions.
> > +	 * See comment above frob_text() for the layout diagram.
> > +	 */
> > +	if (mod) {
> > +		if (!within(addr, init_base + init_text_size,
> > +			    init_ro_after_init_size - init_text_size)
> > +		    && !within(addr, core_base + core_text_size,
> > +			       core_ro_after_init_size - core_text_size))
> > +			mod = NULL;
> > +	}
> > +	return mod;
> > +}
> > +EXPORT_SYMBOL_GPL(__module_ro_address);
> > +
> > /* Don't grab lock, we're oopsing. */
> > void print_modules(void)
> > {
> > --
> > 2.12.0

Kees Cook March 25, 2017, 1:58 a.m. UTC | #5

On Fri, Mar 24, 2017 at 6:41 PM, Eddie Kovsky <[e-mail]> wrote:
> On 03/24/17, Jessica Yu wrote:
>> +++ Eddie Kovsky [22/03/17 20:55 -0600]:
>> > Implement a mechanism to check if a module's address is in
>> > the rodata or ro_after_init sections. It mimics the exsiting functions
>> > that test if an address is inside a module's text section.
>> >
>> > Functions that take a module as an argument will be able to
>>
>> > verify that the module is in a read-only section.
>>
>> s/module/module address/?
>>
> Yes, that is more accurate.
>
>> Also, there is some useful information in your cover letter on the
>> context and motivation for adding to the api, it would be good to
>> reproduce that info here so that we can have it officially in the
>> changelog. (sentences "This implements a suggestion made by Kees..."
>> and "The idea is to prevent structures..." would be nice to copy here)
>>
> Okay, that's easy to add.
>
> Kees, would you like me to add your Suggested-by as well?

Sure! I'm find either way. :)

-Kees

Kees Cook March 25, 2017, 1:59 a.m. UTC | #6

On Fri, Mar 24, 2017 at 5:42 PM, Jessica Yu <[e-mail]> wrote:
> +++ Kees Cook [23/03/17 14:13 -0700]:
>>
>> On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <[e-mail]> wrote:
>>>
>>> Implement a mechanism to check if a module's address is in
>>> the rodata or ro_after_init sections. It mimics the exsiting functions
>>> that test if an address is inside a module's text section.
>>>
>>> Functions that take a module as an argument will be able to
>>> verify that the module is in a read-only section.
>>>
>>> Signed-off-by: Eddie Kovsky <[e-mail]>
>>
>>
>> Awesome! I'll be glad to have these.
>>
>> Reviewed-by: Kees Cook <[e-mail]>
>>
>> Jessica, if this looks good to you, should this go via modules or via
>> my tree? If mine, can I have your Ack?
>
>
> Sure, would be happy to let you take this patch through your tree. I
> only had a few minor comments left, so I think just one more small
> respin and it'll be good to go.

Awesome, I'll wait for v4 and your Ack and put it through the kspp tree. :)

Thanks for the review!

-Kees

Patch

diff --git a/include/linux/module.h b/include/linux/module.h
index 9ad68561d8c2..66b7fd321334 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -492,7 +492,9 @@  static inline int module_is_live(struct module *mod)

 struct module *__module_text_address(unsigned long addr);
 struct module *__module_address(unsigned long addr);
+struct module *__module_ro_address(unsigned long addr);
 bool is_module_address(unsigned long addr);
+bool is_module_rodata_address(unsigned long addr);
 bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
 bool is_module_percpu_address(unsigned long addr);
 bool is_module_text_address(unsigned long addr);
@@ -646,6 +648,16 @@  static inline struct module *__module_address(unsigned long addr)
 	return NULL;
 }

+static inline struct module *__module_ro_address(unsigned long addr)
+{
+	return NULL;
+}
+
+static inline bool is_module_rodata_address(unsigned long addr)
+{
+	return false;
+}
+
 static inline struct module *__module_text_address(unsigned long addr)
 {
 	return NULL;
diff --git a/kernel/module.c b/kernel/module.c
index 8ffcd29a4245..99ada1257aaa 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4292,6 +4292,59 @@  struct module *__module_text_address(unsigned long addr)
 }
 EXPORT_SYMBOL_GPL(__module_text_address);

+/**
+ * is_module_rodata_address - is this address inside read-only module code?
+ * @addr: the address to check.
+ *
+ */
+bool is_module_rodata_address(unsigned long addr)
+{
+	bool ret;
+
+	preempt_disable();
+	ret = __module_ro_address(addr) != NULL;
+	preempt_enable();
+
+	return ret;
+}
+
+/*
+ * __module_ro_address - get the module whose rodata/ro_after_init sections
+ * contain the given address.
+ * @addr: the address.
+ *
+ * Must be called with preempt disabled or module mutex held so that
+ * module doesn't get freed during this.
+ */
+struct module *__module_ro_address(unsigned long addr)
+{
+	struct module *mod = __module_address(addr);
+
+	void *init_base = mod->init_layout.base;
+	unsigned int init_text_size = mod->init_layout.text_size;
+	unsigned int init_ro_after_init_size = mod->init_layout.ro_after_init_size;
+
+	void *core_base = mod->core_layout.base;
+	unsigned int core_text_size = mod->core_layout.text_size;
+	unsigned int core_ro_after_init_size = mod->core_layout.ro_after_init_size;
+
+	/*
+	 * Make sure module is within the read-only section.
+	 * range(base + text_size, base + ro_after_init_size)
+	 * encompasses both the rodata and ro_after_init regions.
+	 * See comment above frob_text() for the layout diagram.
+	 */
+	if (mod) {
+		if (!within(addr, init_base + init_text_size,
+			    init_ro_after_init_size - init_text_size)
+		    && !within(addr, core_base + core_text_size,
+			       core_ro_after_init_size - core_text_size))
+			mod = NULL;
+	}
+	return mod;
+}
+EXPORT_SYMBOL_GPL(__module_ro_address);
+
 /* Don't grab lock, we're oopsing. */
 void print_modules(void)
 {