[PATCH v3 0/2] provide check for ro_after_init memory sections

[PATCH v3 0/2] provide check for ro_after_init memory sections

[Eddie Kovsky]

Provide a mechanism for other functions to verify that their arguments
are read-only.

This implements the first half of a suggestion made by Kees Cook for
the Kernel Self Protection Project:

    - provide mechanism to check for ro_after_init memory areas, and
      reject structures not marked ro_after_init in vmbus_register()

      http://www.openwall.com/lists/kernel-hardening/2017/02/04/1

The idea is to prevent structures (including modules) that are not
read-only from being passed to functions. It builds upon the functions
in kernel/extable.c that test if an address is in the text section.

I have dropped the third patch that uses these features to check the
arguments to vmbus_register() because the maintainers have not been
receptive to using it. My goal right now is to get the API right.

I have test compiled this series on next-20170321 for x86.


Eddie Kovsky (2):
  module: verify address is read-only
  extable: verify address is read-only

 include/linux/kernel.h |  2 ++
 include/linux/module.h | 12 ++++++++++++
 kernel/extable.c       | 29 +++++++++++++++++++++++++++
 kernel/module.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+)

--
2.12.0

[kernel-hardening] [PATCH v3 0/2] provide check for ro_after_init memory sections

[Eddie Kovsky]

Provide a mechanism for other functions to verify that their arguments
are read-only.

This implements the first half of a suggestion made by Kees Cook for
the Kernel Self Protection Project:

    - provide mechanism to check for ro_after_init memory areas, and
      reject structures not marked ro_after_init in vmbus_register()

      http://www.openwall.com/lists/kernel-hardening/2017/02/04/1

The idea is to prevent structures (including modules) that are not
read-only from being passed to functions. It builds upon the functions
in kernel/extable.c that test if an address is in the text section.

I have dropped the third patch that uses these features to check the
arguments to vmbus_register() because the maintainers have not been
receptive to using it. My goal right now is to get the API right.

I have test compiled this series on next-20170321 for x86.


Eddie Kovsky (2):
  module: verify address is read-only
  extable: verify address is read-only

 include/linux/kernel.h |  2 ++
 include/linux/module.h | 12 ++++++++++++
 kernel/extable.c       | 29 +++++++++++++++++++++++++++
 kernel/module.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+)

--
2.12.0

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

[Eddie Kovsky]

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 <ewk@edkovsky.org>
---
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

[kernel-hardening] [PATCH v3 1/2] module: verify address is read-only

[Eddie Kovsky]

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 <ewk@edkovsky.org>
---
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

[PATCH v3 2/2] extable: verify address is read-only

[Eddie Kovsky]

Provide a mechanism to check if the address of a variable is
const or ro_after_init. It mimics the existing functions that test if an
address is inside the kernel's text section.

Other functions inside the kernel could then use this capability to
verify that their arguments are read-only.

Signed-off-by: Eddie Kovsky <ewk@edkovsky.org>
---
Changes in v3:
 - Fix missing declaration of is_module_rodata_address()

 include/linux/kernel.h |  2 ++
 kernel/extable.c       | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4c26dc3a8295..51beea39e6c4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -444,6 +444,8 @@ extern int core_kernel_data(unsigned long addr);
 extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
+extern int core_kernel_ro_data(unsigned long addr);
+extern int kernel_ro_address(unsigned long addr);

 unsigned long int_sqrt(unsigned long);

diff --git a/kernel/extable.c b/kernel/extable.c
index 2676d7f8baf6..3c3a9f4e6250 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -154,3 +154,32 @@ int func_ptr_is_kernel_text(void *ptr)
 		return 1;
 	return is_module_text_address(addr);
 }
+
+/**
+ * core_kernel_ro_data - Verify address points to read-only section
+ * @addr: address to test
+ *
+ */
+int core_kernel_ro_data(unsigned long addr)
+{
+	if (addr >= (unsigned long)__start_rodata &&
+	    addr < (unsigned long)__end_rodata)
+		return 1;
+
+	if (addr >= (unsigned long)__start_data_ro_after_init &&
+	    addr < (unsigned long)__end_data_ro_after_init)
+		return 1;
+
+	return 0;
+}
+
+/* Verify that address is const or ro_after_init. */
+int kernel_ro_address(unsigned long addr)
+{
+	if (core_kernel_ro_data(addr))
+		return 1;
+	if (is_module_rodata_address(addr))
+		return 1;
+
+	return 0;
+}
--
2.12.0

[kernel-hardening] [PATCH v3 2/2] extable: verify address is read-only

[Eddie Kovsky]

Provide a mechanism to check if the address of a variable is
const or ro_after_init. It mimics the existing functions that test if an
address is inside the kernel's text section.

Other functions inside the kernel could then use this capability to
verify that their arguments are read-only.

Signed-off-by: Eddie Kovsky <ewk@edkovsky.org>
---
Changes in v3:
 - Fix missing declaration of is_module_rodata_address()

 include/linux/kernel.h |  2 ++
 kernel/extable.c       | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4c26dc3a8295..51beea39e6c4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -444,6 +444,8 @@ extern int core_kernel_data(unsigned long addr);
 extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
 extern int func_ptr_is_kernel_text(void *ptr);
+extern int core_kernel_ro_data(unsigned long addr);
+extern int kernel_ro_address(unsigned long addr);

 unsigned long int_sqrt(unsigned long);

diff --git a/kernel/extable.c b/kernel/extable.c
index 2676d7f8baf6..3c3a9f4e6250 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -154,3 +154,32 @@ int func_ptr_is_kernel_text(void *ptr)
 		return 1;
 	return is_module_text_address(addr);
 }
+
+/**
+ * core_kernel_ro_data - Verify address points to read-only section
+ * @addr: address to test
+ *
+ */
+int core_kernel_ro_data(unsigned long addr)
+{
+	if (addr >= (unsigned long)__start_rodata &&
+	    addr < (unsigned long)__end_rodata)
+		return 1;
+
+	if (addr >= (unsigned long)__start_data_ro_after_init &&
+	    addr < (unsigned long)__end_data_ro_after_init)
+		return 1;
+
+	return 0;
+}
+
+/* Verify that address is const or ro_after_init. */
+int kernel_ro_address(unsigned long addr)
+{
+	if (core_kernel_ro_data(addr))
+		return 1;
+	if (is_module_rodata_address(addr))
+		return 1;
+
+	return 0;
+}
--
2.12.0

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

[Kees Cook]

On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <ewk@edkovsky.org> 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 <ewk@edkovsky.org>

Awesome! I'll be glad to have these.

Reviewed-by: Kees Cook <keescook@chromium.org>

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



-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [PATCH v3 1/2] module: verify address is read-only

[Kees Cook]

On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <ewk@edkovsky.org> 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 <ewk@edkovsky.org>

Awesome! I'll be glad to have these.

Reviewed-by: Kees Cook <keescook@chromium.org>

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



-- 
Kees Cook
Pixel Security

Re: [PATCH v3 2/2] extable: verify address is read-only

[Kees Cook]

On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <ewk@edkovsky.org> wrote:
> Provide a mechanism to check if the address of a variable is
> const or ro_after_init. It mimics the existing functions that test if an
> address is inside the kernel's text section.
>
> Other functions inside the kernel could then use this capability to
> verify that their arguments are read-only.
>
> Signed-off-by: Eddie Kovsky <ewk@edkovsky.org>

Looks great!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> Changes in v3:
>  - Fix missing declaration of is_module_rodata_address()
>
>  include/linux/kernel.h |  2 ++
>  kernel/extable.c       | 29 +++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 4c26dc3a8295..51beea39e6c4 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -444,6 +444,8 @@ extern int core_kernel_data(unsigned long addr);
>  extern int __kernel_text_address(unsigned long addr);
>  extern int kernel_text_address(unsigned long addr);
>  extern int func_ptr_is_kernel_text(void *ptr);
> +extern int core_kernel_ro_data(unsigned long addr);
> +extern int kernel_ro_address(unsigned long addr);
>
>  unsigned long int_sqrt(unsigned long);
>
> diff --git a/kernel/extable.c b/kernel/extable.c
> index 2676d7f8baf6..3c3a9f4e6250 100644
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -154,3 +154,32 @@ int func_ptr_is_kernel_text(void *ptr)
>                 return 1;
>         return is_module_text_address(addr);
>  }
> +
> +/**
> + * core_kernel_ro_data - Verify address points to read-only section
> + * @addr: address to test
> + *
> + */
> +int core_kernel_ro_data(unsigned long addr)
> +{
> +       if (addr >= (unsigned long)__start_rodata &&
> +           addr < (unsigned long)__end_rodata)
> +               return 1;
> +
> +       if (addr >= (unsigned long)__start_data_ro_after_init &&
> +           addr < (unsigned long)__end_data_ro_after_init)
> +               return 1;
> +
> +       return 0;
> +}
> +
> +/* Verify that address is const or ro_after_init. */
> +int kernel_ro_address(unsigned long addr)
> +{
> +       if (core_kernel_ro_data(addr))
> +               return 1;
> +       if (is_module_rodata_address(addr))
> +               return 1;
> +
> +       return 0;
> +}
> --
> 2.12.0



-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [PATCH v3 2/2] extable: verify address is read-only

[Kees Cook]

On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <ewk@edkovsky.org> wrote:
> Provide a mechanism to check if the address of a variable is
> const or ro_after_init. It mimics the existing functions that test if an
> address is inside the kernel's text section.
>
> Other functions inside the kernel could then use this capability to
> verify that their arguments are read-only.
>
> Signed-off-by: Eddie Kovsky <ewk@edkovsky.org>

Looks great!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> Changes in v3:
>  - Fix missing declaration of is_module_rodata_address()
>
>  include/linux/kernel.h |  2 ++
>  kernel/extable.c       | 29 +++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 4c26dc3a8295..51beea39e6c4 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -444,6 +444,8 @@ extern int core_kernel_data(unsigned long addr);
>  extern int __kernel_text_address(unsigned long addr);
>  extern int kernel_text_address(unsigned long addr);
>  extern int func_ptr_is_kernel_text(void *ptr);
> +extern int core_kernel_ro_data(unsigned long addr);
> +extern int kernel_ro_address(unsigned long addr);
>
>  unsigned long int_sqrt(unsigned long);
>
> diff --git a/kernel/extable.c b/kernel/extable.c
> index 2676d7f8baf6..3c3a9f4e6250 100644
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -154,3 +154,32 @@ int func_ptr_is_kernel_text(void *ptr)
>                 return 1;
>         return is_module_text_address(addr);
>  }
> +
> +/**
> + * core_kernel_ro_data - Verify address points to read-only section
> + * @addr: address to test
> + *
> + */
> +int core_kernel_ro_data(unsigned long addr)
> +{
> +       if (addr >= (unsigned long)__start_rodata &&
> +           addr < (unsigned long)__end_rodata)
> +               return 1;
> +
> +       if (addr >= (unsigned long)__start_data_ro_after_init &&
> +           addr < (unsigned long)__end_data_ro_after_init)
> +               return 1;
> +
> +       return 0;
> +}
> +
> +/* Verify that address is const or ro_after_init. */
> +int kernel_ro_address(unsigned long addr)
> +{
> +       if (core_kernel_ro_data(addr))
> +               return 1;
> +       if (is_module_rodata_address(addr))
> +               return 1;
> +
> +       return 0;
> +}
> --
> 2.12.0



-- 
Kees Cook
Pixel Security

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

[Jessica Yu]

+++ 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 <ewk@edkovsky.org>
>---
>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

[kernel-hardening] Re: [PATCH v3 1/2] module: verify address is read-only

[Jessica Yu]

+++ 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 <ewk@edkovsky.org>
>---
>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

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

[Jessica Yu]

+++ Kees Cook [23/03/17 14:13 -0700]:
>On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <ewk@edkovsky.org> 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 <ewk@edkovsky.org>
>
>Awesome! I'll be glad to have these.
>
>Reviewed-by: Kees Cook <keescook@chromium.org>
>
>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

[kernel-hardening] Re: [PATCH v3 1/2] module: verify address is read-only

[Jessica Yu]

+++ Kees Cook [23/03/17 14:13 -0700]:
>On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <ewk@edkovsky.org> 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 <ewk@edkovsky.org>
>
>Awesome! I'll be glad to have these.
>
>Reviewed-by: Kees Cook <keescook@chromium.org>
>
>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

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

[Eddie Kovsky]

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 <ewk@edkovsky.org>
> > ---
> > 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

[kernel-hardening] Re: [PATCH v3 1/2] module: verify address is read-only

[Eddie Kovsky]

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 <ewk@edkovsky.org>
> > ---
> > 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

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

[Kees Cook]

On Fri, Mar 24, 2017 at 6:41 PM, Eddie Kovsky <ewk@edkovsky.org> 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
Pixel Security

[kernel-hardening] Re: [PATCH v3 1/2] module: verify address is read-only

[Kees Cook]

On Fri, Mar 24, 2017 at 6:41 PM, Eddie Kovsky <ewk@edkovsky.org> 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
Pixel Security

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

[Kees Cook]

On Fri, Mar 24, 2017 at 5:42 PM, Jessica Yu <jeyu@redhat.com> wrote:
> +++ Kees Cook [23/03/17 14:13 -0700]:
>>
>> On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <ewk@edkovsky.org> 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 <ewk@edkovsky.org>
>>
>>
>> Awesome! I'll be glad to have these.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>
>> 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

-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [PATCH v3 1/2] module: verify address is read-only

[Kees Cook]

On Fri, Mar 24, 2017 at 5:42 PM, Jessica Yu <jeyu@redhat.com> wrote:
> +++ Kees Cook [23/03/17 14:13 -0700]:
>>
>> On Wed, Mar 22, 2017 at 7:55 PM, Eddie Kovsky <ewk@edkovsky.org> 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 <ewk@edkovsky.org>
>>
>>
>> Awesome! I'll be glad to have these.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>
>> 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

-- 
Kees Cook
Pixel Security