linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] provide check for ro_after_init memory sections
@ 2017-03-26 21:08 Eddie Kovsky
  2017-03-26 21:08 ` [PATCH v4 1/2] module: verify address is read-only Eddie Kovsky
  2017-03-26 21:08 ` [PATCH v4 2/2] extable: " Eddie Kovsky
  0 siblings, 2 replies; 11+ messages in thread
From: Eddie Kovsky @ 2017-03-26 21:08 UTC (permalink / raw)
  To: jeyu, rusty, keescook; +Cc: linux-kernel, kernel-hardening

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-20170324 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.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v4 1/2] module: verify address is read-only
  2017-03-26 21:08 [PATCH v4 0/2] provide check for ro_after_init memory sections Eddie Kovsky
@ 2017-03-26 21:08 ` Eddie Kovsky
  2017-03-30  3:31   ` Jessica Yu
  2017-03-26 21:08 ` [PATCH v4 2/2] extable: " Eddie Kovsky
  1 sibling, 1 reply; 11+ messages in thread
From: Eddie Kovsky @ 2017-03-26 21:08 UTC (permalink / raw)
  To: jeyu, rusty, keescook; +Cc: linux-kernel, kernel-hardening

Implement a mechanism to check if a module's address is in
the rodata or ro_after_init sections. It mimics the existing 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 address is in a read-only section. The idea is to prevent structures
(including modules) that are not read-only from being passed to functions.

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

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Eddie Kovsky <ewk@edkovsky.org>
---
Changes in v4:
 - Rename function __module_ro_address() to __module_rodata_address()
 - Move module_rodata_address() stub below is_module_address()
 - Minor comment fixes
 - Verify that mod is not NULL before setting up layout variables
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..a3d17b081de3 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_rodata_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,11 @@ static inline struct module *__module_address(unsigned long addr)
 	return NULL;
 }
 
+static inline struct module *__module_rodata_address(unsigned long addr)
+{
+	return NULL;
+}
+
 static inline struct module *__module_text_address(unsigned long addr)
 {
 	return NULL;
@@ -656,6 +663,11 @@ static inline bool is_module_address(unsigned long addr)
 	return false;
 }
 
+static inline bool is_module_rodata_address(unsigned long addr)
+{
+	return false;
+}
+
 static inline bool is_module_percpu_address(unsigned long addr)
 {
 	return false;
diff --git a/kernel/module.c b/kernel/module.c
index 8ffcd29a4245..26bd62c61d87 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 data?
+ * @addr: the address to check.
+ *
+ */
+bool is_module_rodata_address(unsigned long addr)
+{
+	bool ret;
+
+	preempt_disable();
+	ret = __module_rodata_address(addr) != NULL;
+	preempt_enable();
+
+	return ret;
+}
+
+/*
+ * __module_rodata_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_rodata_address(unsigned long addr)
+{
+	struct module *mod = __module_address(addr);
+
+	/*
+	 * 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) {
+		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;
+
+		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_rodata_address);
+
 /* Don't grab lock, we're oopsing. */
 void print_modules(void)
 {
-- 
2.12.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 2/2] extable: verify address is read-only
  2017-03-26 21:08 [PATCH v4 0/2] provide check for ro_after_init memory sections Eddie Kovsky
  2017-03-26 21:08 ` [PATCH v4 1/2] module: verify address is read-only Eddie Kovsky
@ 2017-03-26 21:08 ` Eddie Kovsky
  2017-03-27  8:43   ` kbuild test robot
  1 sibling, 1 reply; 11+ messages in thread
From: Eddie Kovsky @ 2017-03-26 21:08 UTC (permalink / raw)
  To: jeyu, rusty, keescook; +Cc: linux-kernel, kernel-hardening

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.

The idea is to prevent structures that are not read-only from being
passed to functions. Other functions inside the kernel could then use
this capability 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()

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Eddie Kovsky <ewk@edkovsky.org>
---
Changes in v4:
 - Rename function core_kernel_ro_data() to core_kernel_rodata().
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..5748784ca209 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_rodata(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..22562cfc6ac3 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_rodata - Verify address points to read-only section
+ * @addr: address to test
+ *
+ */
+int core_kernel_rodata(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_rodata(addr))
+		return 1;
+	if (is_module_rodata_address(addr))
+		return 1;
+
+	return 0;
+}
-- 
2.12.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] extable: verify address is read-only
  2017-03-26 21:08 ` [PATCH v4 2/2] extable: " Eddie Kovsky
@ 2017-03-27  8:43   ` kbuild test robot
  2017-03-27 18:42     ` Kees Cook
  0 siblings, 1 reply; 11+ messages in thread
From: kbuild test robot @ 2017-03-27  8:43 UTC (permalink / raw)
  To: Eddie Kovsky
  Cc: kbuild-all, jeyu, rusty, keescook, linux-kernel, kernel-hardening

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

Hi Eddie,

[auto build test ERROR on next-20170323]
[cannot apply to linus/master linux/master jeyu/modules-next v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.11-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eddie-Kovsky/module-verify-address-is-read-only/20170327-142922
config: blackfin-BF561-EZKIT-SMP_defconfig (attached as .config)
compiler: bfin-uclinux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=blackfin 

All errors (new ones prefixed by >>):

   kernel/built-in.o: In function `core_kernel_rodata':
>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'

vim +169 kernel/extable.c

   163	int core_kernel_rodata(unsigned long addr)
   164	{
   165		if (addr >= (unsigned long)__start_rodata &&
   166		    addr < (unsigned long)__end_rodata)
   167			return 1;
   168	
 > 169		if (addr >= (unsigned long)__start_data_ro_after_init &&
   170		    addr < (unsigned long)__end_data_ro_after_init)
   171			return 1;
   172	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10729 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] extable: verify address is read-only
  2017-03-27  8:43   ` kbuild test robot
@ 2017-03-27 18:42     ` Kees Cook
  2017-03-29  3:28       ` Eddie Kovsky
  0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2017-03-27 18:42 UTC (permalink / raw)
  To: Jakub Kicinski, Catalin Marinas, Heiko Carstens, Eddie Kovsky
  Cc: kbuild-all, kbuild test robot, Jessica Yu, Rusty Russell, LKML,
	kernel-hardening

On Mon, Mar 27, 2017 at 1:43 AM, kbuild test robot <lkp@intel.com> wrote:
> Hi Eddie,
>
> [auto build test ERROR on next-20170323]
> [cannot apply to linus/master linux/master jeyu/modules-next v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.11-rc4]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Eddie-Kovsky/module-verify-address-is-read-only/20170327-142922
> config: blackfin-BF561-EZKIT-SMP_defconfig (attached as .config)
> compiler: bfin-uclinux-gcc (GCC) 6.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=blackfin
>
> All errors (new ones prefixed by >>):
>
>    kernel/built-in.o: In function `core_kernel_rodata':
>>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'

Hm, I'm confused about this. blackfin includes
include/asm-generic-vmlinux.lds.h and uses the RO_DATA macro (which
resolves to RO_DATA_SECTION to RO_AFTER_INIT_DATA which defines
__[start|end]_data_ro_after_init.

Also, it seems that commit d7c19b066dcf4bd19c4385e8065558d4e74f9e73
("mm: kmemleak: scan .data.ro_after_init") added a potentially
redundant section name (s390 already calls this
__[start|end]_ro_after_init). I'd like to get this cleaned up, since
having multiple names for the same thing is confusing:

diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
index 000e6e91f6a0..3667d20e997f 100644
--- a/arch/s390/kernel/vmlinux.lds.S
+++ b/arch/s390/kernel/vmlinux.lds.S
@@ -62,9 +62,11 @@ SECTIONS

        . = ALIGN(PAGE_SIZE);
        __start_ro_after_init = .;
+       __start_data_ro_after_init = .;
        .data..ro_after_init : {
                 *(.data..ro_after_init)
        }
+       __end_data_ro_after_init = .;
        EXCEPTION_TABLE(16)
        . = ALIGN(PAGE_SIZE);
        __end_ro_after_init = .;

And it seems that this hunk is wrong (__end_ro_after_init includes
s390's exception table, etc). I think we should remove the
..._data_... name and use s390's name.

I'll send an adjustment patch, but we'll still need to deal with blackfin.

-Kees

>
> vim +169 kernel/extable.c
>
>    163  int core_kernel_rodata(unsigned long addr)
>    164  {
>    165          if (addr >= (unsigned long)__start_rodata &&
>    166              addr < (unsigned long)__end_rodata)
>    167                  return 1;
>    168
>  > 169          if (addr >= (unsigned long)__start_data_ro_after_init &&
>    170              addr < (unsigned long)__end_data_ro_after_init)
>    171                  return 1;
>    172
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation



-- 
Kees Cook
Pixel Security

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] extable: verify address is read-only
  2017-03-27 18:42     ` Kees Cook
@ 2017-03-29  3:28       ` Eddie Kovsky
  2017-03-30  3:27         ` Jessica Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Eddie Kovsky @ 2017-03-29  3:28 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jakub Kicinski, Catalin Marinas, Heiko Carstens, kbuild-all,
	kbuild test robot, Jessica Yu, Rusty Russell, LKML,
	kernel-hardening

On 03/27/17, Kees Cook wrote:
> On Mon, Mar 27, 2017 at 1:43 AM, kbuild test robot <lkp@intel.com> wrote:
> > Hi Eddie,
> >
> > [auto build test ERROR on next-20170323]
> > [cannot apply to linus/master linux/master jeyu/modules-next v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.11-rc4]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url:    https://github.com/0day-ci/linux/commits/Eddie-Kovsky/module-verify-address-is-read-only/20170327-142922
> > config: blackfin-BF561-EZKIT-SMP_defconfig (attached as .config)
> > compiler: bfin-uclinux-gcc (GCC) 6.2.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         make.cross ARCH=blackfin
> >
> > All errors (new ones prefixed by >>):
> >
> >    kernel/built-in.o: In function `core_kernel_rodata':
> >>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
> >>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
> >>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
> >>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
> >>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
> >>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
> >>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
> >>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
> 
> Hm, I'm confused about this. blackfin includes
> include/asm-generic-vmlinux.lds.h and uses the RO_DATA macro (which
> resolves to RO_DATA_SECTION to RO_AFTER_INIT_DATA which defines
> __[start|end]_data_ro_after_init.
> 
> Also, it seems that commit d7c19b066dcf4bd19c4385e8065558d4e74f9e73
> ("mm: kmemleak: scan .data.ro_after_init") added a potentially
> redundant section name (s390 already calls this
> __[start|end]_ro_after_init). I'd like to get this cleaned up, since
> having multiple names for the same thing is confusing:
> 
> diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
> index 000e6e91f6a0..3667d20e997f 100644
> --- a/arch/s390/kernel/vmlinux.lds.S
> +++ b/arch/s390/kernel/vmlinux.lds.S
> @@ -62,9 +62,11 @@ SECTIONS
> 
>         . = ALIGN(PAGE_SIZE);
>         __start_ro_after_init = .;
> +       __start_data_ro_after_init = .;
>         .data..ro_after_init : {
>                  *(.data..ro_after_init)
>         }
> +       __end_data_ro_after_init = .;
>         EXCEPTION_TABLE(16)
>         . = ALIGN(PAGE_SIZE);
>         __end_ro_after_init = .;
> 
> And it seems that this hunk is wrong (__end_ro_after_init includes
> s390's exception table, etc). I think we should remove the
> ..._data_... name and use s390's name.
> 
> I'll send an adjustment patch, but we'll still need to deal with blackfin.
> 
> -Kees
> 

Kees

I applied your patch (mm: fix section name for .data..ro_after_init) and
changed the new function in extable.c to use __[start|end]_ro_after_init
instead. The new version still builds without errors on x86, which isn't
surprising.

I've cross compiled this for blackfin and I'm able to reproduce the
build error. I'm still not sure why. As you pointed out, blackfin does
appear to use 'include/asm-generic/vmlinux.lds.h'. 

Eddie

> >
> > vim +169 kernel/extable.c
> >
> >    163  int core_kernel_rodata(unsigned long addr)
> >    164  {
> >    165          if (addr >= (unsigned long)__start_rodata &&
> >    166              addr < (unsigned long)__end_rodata)
> >    167                  return 1;
> >    168
> >  > 169          if (addr >= (unsigned long)__start_data_ro_after_init &&
> >    170              addr < (unsigned long)__end_data_ro_after_init)
> >    171                  return 1;
> >    172
> >
> > ---
> > 0-DAY kernel test infrastructure                Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] extable: verify address is read-only
  2017-03-29  3:28       ` Eddie Kovsky
@ 2017-03-30  3:27         ` Jessica Yu
  2017-03-30 16:24           ` Kees Cook
  2017-04-03 22:10           ` Kees Cook
  0 siblings, 2 replies; 11+ messages in thread
From: Jessica Yu @ 2017-03-30  3:27 UTC (permalink / raw)
  To: Kees Cook, Jakub Kicinski, Catalin Marinas, Heiko Carstens,
	kbuild-all, kbuild test robot, Rusty Russell, LKML,
	kernel-hardening

+++ Eddie Kovsky [28/03/17 21:28 -0600]:
>On 03/27/17, Kees Cook wrote:
>> On Mon, Mar 27, 2017 at 1:43 AM, kbuild test robot <lkp@intel.com> wrote:
>> > Hi Eddie,
>> >
>> > [auto build test ERROR on next-20170323]
>> > [cannot apply to linus/master linux/master jeyu/modules-next v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.11-rc4]
>> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>> >
>> > url:    https://github.com/0day-ci/linux/commits/Eddie-Kovsky/module-verify-address-is-read-only/20170327-142922
>> > config: blackfin-BF561-EZKIT-SMP_defconfig (attached as .config)
>> > compiler: bfin-uclinux-gcc (GCC) 6.2.0
>> > reproduce:
>> >         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> >         chmod +x ~/bin/make.cross
>> >         # save the attached .config to linux build tree
>> >         make.cross ARCH=blackfin
>> >
>> > All errors (new ones prefixed by >>):
>> >
>> >    kernel/built-in.o: In function `core_kernel_rodata':
>> >>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>> >>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>> >>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>> >>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>> >>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>> >>> kernel/extable.c:169: undefined reference to `__start_data_ro_after_init'
>> >>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>> >>> kernel/extable.c:169: undefined reference to `__end_data_ro_after_init'
>>
>> Hm, I'm confused about this. blackfin includes
>> include/asm-generic-vmlinux.lds.h and uses the RO_DATA macro (which
>> resolves to RO_DATA_SECTION to RO_AFTER_INIT_DATA which defines
>> __[start|end]_data_ro_after_init.
>>
>> Also, it seems that commit d7c19b066dcf4bd19c4385e8065558d4e74f9e73
>> ("mm: kmemleak: scan .data.ro_after_init") added a potentially
>> redundant section name (s390 already calls this
>> __[start|end]_ro_after_init). I'd like to get this cleaned up, since
>> having multiple names for the same thing is confusing:
>>
>> diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
>> index 000e6e91f6a0..3667d20e997f 100644
>> --- a/arch/s390/kernel/vmlinux.lds.S
>> +++ b/arch/s390/kernel/vmlinux.lds.S
>> @@ -62,9 +62,11 @@ SECTIONS
>>
>>         . = ALIGN(PAGE_SIZE);
>>         __start_ro_after_init = .;
>> +       __start_data_ro_after_init = .;
>>         .data..ro_after_init : {
>>                  *(.data..ro_after_init)
>>         }
>> +       __end_data_ro_after_init = .;
>>         EXCEPTION_TABLE(16)
>>         . = ALIGN(PAGE_SIZE);
>>         __end_ro_after_init = .;
>>
>> And it seems that this hunk is wrong (__end_ro_after_init includes
>> s390's exception table, etc). I think we should remove the
>> ..._data_... name and use s390's name.
>>
>> I'll send an adjustment patch, but we'll still need to deal with blackfin.
>>
>> -Kees
>>
>
>Kees
>
>I applied your patch (mm: fix section name for .data..ro_after_init) and
>changed the new function in extable.c to use __[start|end]_ro_after_init
>instead. The new version still builds without errors on x86, which isn't
>surprising.
>
>I've cross compiled this for blackfin and I'm able to reproduce the
>build error. I'm still not sure why. As you pointed out, blackfin does
>appear to use 'include/asm-generic/vmlinux.lds.h'.

This appears to be because blackfin is one of the 2 arches that
prepends an underscore '_' to all symbols defined in C. I noticed that
__{start,end}_data_ro_after_init in vmlinux.lds.h are not wrapped with
VMLINUX_SYMBOL() which adds the necessary prefix for arches that have
HAVE_UNDERSCORE_SYMBOL_PREFIX, hence the undefined reference.

The below patch fixed the build error for me, if it works for you then
I can send a formal patch.

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 4e09b28..7b262f7 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -260,9 +260,9 @@
  */
 #ifndef RO_AFTER_INIT_DATA
 #define RO_AFTER_INIT_DATA						\
-	__start_data_ro_after_init = .;					\
+	VMLINUX_SYMBOL(__start_data_ro_after_init) = .;			\
 	*(.data..ro_after_init)						\
-	__end_data_ro_after_init = .;
+	VMLINUX_SYMBOL(__end_data_ro_after_init) = .;
 #endif
 
 /*

Jessica

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/2] module: verify address is read-only
  2017-03-26 21:08 ` [PATCH v4 1/2] module: verify address is read-only Eddie Kovsky
@ 2017-03-30  3:31   ` Jessica Yu
  0 siblings, 0 replies; 11+ messages in thread
From: Jessica Yu @ 2017-03-30  3:31 UTC (permalink / raw)
  To: Eddie Kovsky; +Cc: rusty, keescook, linux-kernel, kernel-hardening

+++ Eddie Kovsky [26/03/17 15:08 -0600]:
>Implement a mechanism to check if a module's address is in
>the rodata or ro_after_init sections. It mimics the existing 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 address is in a read-only section. The idea is to prevent structures
>(including modules) that are not read-only from being passed to functions.
>
>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()
>
>Suggested-by: Kees Cook <keescook@chromium.org>
>Signed-off-by: Eddie Kovsky <ewk@edkovsky.org>

Thanks for the respin, this looks much better.

Acked-by: Jessica Yu <jeyu@redhat.com>

>---
>Changes in v4:
> - Rename function __module_ro_address() to __module_rodata_address()
> - Move module_rodata_address() stub below is_module_address()
> - Minor comment fixes
> - Verify that mod is not NULL before setting up layout variables
>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..a3d17b081de3 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_rodata_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,11 @@ static inline struct module *__module_address(unsigned long addr)
> 	return NULL;
> }
>
>+static inline struct module *__module_rodata_address(unsigned long addr)
>+{
>+	return NULL;
>+}
>+
> static inline struct module *__module_text_address(unsigned long addr)
> {
> 	return NULL;
>@@ -656,6 +663,11 @@ static inline bool is_module_address(unsigned long addr)
> 	return false;
> }
>
>+static inline bool is_module_rodata_address(unsigned long addr)
>+{
>+	return false;
>+}
>+
> static inline bool is_module_percpu_address(unsigned long addr)
> {
> 	return false;
>diff --git a/kernel/module.c b/kernel/module.c
>index 8ffcd29a4245..26bd62c61d87 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 data?
>+ * @addr: the address to check.
>+ *
>+ */
>+bool is_module_rodata_address(unsigned long addr)
>+{
>+	bool ret;
>+
>+	preempt_disable();
>+	ret = __module_rodata_address(addr) != NULL;
>+	preempt_enable();
>+
>+	return ret;
>+}
>+
>+/*
>+ * __module_rodata_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_rodata_address(unsigned long addr)
>+{
>+	struct module *mod = __module_address(addr);
>+
>+	/*
>+	 * 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) {
>+		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;
>+
>+		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_rodata_address);
>+
> /* Don't grab lock, we're oopsing. */
> void print_modules(void)
> {
>-- 
>2.12.1
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] extable: verify address is read-only
  2017-03-30  3:27         ` Jessica Yu
@ 2017-03-30 16:24           ` Kees Cook
  2017-03-31  3:09             ` Eddie Kovsky
  2017-04-03 22:10           ` Kees Cook
  1 sibling, 1 reply; 11+ messages in thread
From: Kees Cook @ 2017-03-30 16:24 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Jakub Kicinski, Catalin Marinas, Heiko Carstens, kbuild-all,
	kbuild test robot, Rusty Russell, LKML, kernel-hardening,
	Andrew Morton

On Wed, Mar 29, 2017 at 8:27 PM, Jessica Yu <jeyu@redhat.com> wrote:
> +++ Eddie Kovsky [28/03/17 21:28 -0600]:
>
>> On 03/27/17, Kees Cook wrote:
>>>
>>> On Mon, Mar 27, 2017 at 1:43 AM, kbuild test robot <lkp@intel.com> wrote:
>>> > Hi Eddie,
>>> >
>>> > [auto build test ERROR on next-20170323]
>>> > [cannot apply to linus/master linux/master jeyu/modules-next v4.9-rc8
>>> > v4.9-rc7 v4.9-rc6 v4.11-rc4]
>>> > [if your patch is applied to the wrong git tree, please drop us a note
>>> > to help improve the system]
>>> >
>>> > url:
>>> > https://github.com/0day-ci/linux/commits/Eddie-Kovsky/module-verify-address-is-read-only/20170327-142922
>>> > config: blackfin-BF561-EZKIT-SMP_defconfig (attached as .config)
>>> > compiler: bfin-uclinux-gcc (GCC) 6.2.0
>>> > reproduce:
>>> >         wget
>>> > https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O
>>> > ~/bin/make.cross
>>> >         chmod +x ~/bin/make.cross
>>> >         # save the attached .config to linux build tree
>>> >         make.cross ARCH=blackfin
>>> >
>>> > All errors (new ones prefixed by >>):
>>> >
>>> >    kernel/built-in.o: In function `core_kernel_rodata':
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>>
>>> Hm, I'm confused about this. blackfin includes
>>> include/asm-generic-vmlinux.lds.h and uses the RO_DATA macro (which
>>> resolves to RO_DATA_SECTION to RO_AFTER_INIT_DATA which defines
>>> __[start|end]_data_ro_after_init.
>>>
>>> Also, it seems that commit d7c19b066dcf4bd19c4385e8065558d4e74f9e73
>>> ("mm: kmemleak: scan .data.ro_after_init") added a potentially
>>> redundant section name (s390 already calls this
>>> __[start|end]_ro_after_init). I'd like to get this cleaned up, since
>>> having multiple names for the same thing is confusing:
>>>
>>> diff --git a/arch/s390/kernel/vmlinux.lds.S
>>> b/arch/s390/kernel/vmlinux.lds.S
>>> index 000e6e91f6a0..3667d20e997f 100644
>>> --- a/arch/s390/kernel/vmlinux.lds.S
>>> +++ b/arch/s390/kernel/vmlinux.lds.S
>>> @@ -62,9 +62,11 @@ SECTIONS
>>>
>>>         . = ALIGN(PAGE_SIZE);
>>>         __start_ro_after_init = .;
>>> +       __start_data_ro_after_init = .;
>>>         .data..ro_after_init : {
>>>                  *(.data..ro_after_init)
>>>         }
>>> +       __end_data_ro_after_init = .;
>>>         EXCEPTION_TABLE(16)
>>>         . = ALIGN(PAGE_SIZE);
>>>         __end_ro_after_init = .;
>>>
>>> And it seems that this hunk is wrong (__end_ro_after_init includes
>>> s390's exception table, etc). I think we should remove the
>>> ..._data_... name and use s390's name.
>>>
>>> I'll send an adjustment patch, but we'll still need to deal with
>>> blackfin.
>>>
>>> -Kees
>>>
>>
>> Kees
>>
>> I applied your patch (mm: fix section name for .data..ro_after_init) and
>> changed the new function in extable.c to use __[start|end]_ro_after_init
>> instead. The new version still builds without errors on x86, which isn't
>> surprising.
>>
>> I've cross compiled this for blackfin and I'm able to reproduce the
>> build error. I'm still not sure why. As you pointed out, blackfin does
>> appear to use 'include/asm-generic/vmlinux.lds.h'.
>
>
> This appears to be because blackfin is one of the 2 arches that
> prepends an underscore '_' to all symbols defined in C. I noticed that
> __{start,end}_data_ro_after_init in vmlinux.lds.h are not wrapped with
> VMLINUX_SYMBOL() which adds the necessary prefix for arches that have
> HAVE_UNDERSCORE_SYMBOL_PREFIX, hence the undefined reference.

Argh. Thank you for catching this! Yeah, that would have taken me
forever to find.

> The below patch fixed the build error for me, if it works for you then
> I can send a formal patch.
>
> diff --git a/include/asm-generic/vmlinux.lds.h
> b/include/asm-generic/vmlinux.lds.h
> index 4e09b28..7b262f7 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -260,9 +260,9 @@
>  */
> #ifndef RO_AFTER_INIT_DATA
> #define RO_AFTER_INIT_DATA                                              \
> -       __start_data_ro_after_init = .;                                 \
> +       VMLINUX_SYMBOL(__start_data_ro_after_init) = .;                 \
>         *(.data..ro_after_init)                                         \
> -       __end_data_ro_after_init = .;
> +       VMLINUX_SYMBOL(__end_data_ro_after_init) = .;
> #endif

I don't have a blackfin cross-compiler set up, but I'm sure that'll
fix it. If you can, please base it on -next, since I rename
__[start|end]_data_ro_after_init to __[start|end]_ro_after_init (to
match the existing s390 symbols of the same purpose):

https://lkml.org/lkml/2017/3/27/685

akpm is carrying that patch, so this follow-up should likely go to him too.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] extable: verify address is read-only
  2017-03-30 16:24           ` Kees Cook
@ 2017-03-31  3:09             ` Eddie Kovsky
  0 siblings, 0 replies; 11+ messages in thread
From: Eddie Kovsky @ 2017-03-31  3:09 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jessica Yu, Jakub Kicinski, Catalin Marinas, Heiko Carstens,
	kbuild-all, kbuild test robot, Rusty Russell, LKML,
	kernel-hardening, Andrew Morton

On 03/30/17, Kees Cook wrote:
> On Wed, Mar 29, 2017 at 8:27 PM, Jessica Yu <jeyu@redhat.com> wrote:
> > +++ Eddie Kovsky [28/03/17 21:28 -0600]:
> >
> >> On 03/27/17, Kees Cook wrote:
> >>>
> >>> On Mon, Mar 27, 2017 at 1:43 AM, kbuild test robot <lkp@intel.com> wrote:
> >>> > Hi Eddie,
> >>> >
> >>> > [auto build test ERROR on next-20170323]
> >>> > [cannot apply to linus/master linux/master jeyu/modules-next v4.9-rc8
> >>> > v4.9-rc7 v4.9-rc6 v4.11-rc4]
> >>> > [if your patch is applied to the wrong git tree, please drop us a note
> >>> > to help improve the system]
> >>> >
> >>> > url:
> >>> > https://github.com/0day-ci/linux/commits/Eddie-Kovsky/module-verify-address-is-read-only/20170327-142922
> >>> > config: blackfin-BF561-EZKIT-SMP_defconfig (attached as .config)
> >>> > compiler: bfin-uclinux-gcc (GCC) 6.2.0
> >>> > reproduce:
> >>> >         wget
> >>> > https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O
> >>> > ~/bin/make.cross
> >>> >         chmod +x ~/bin/make.cross
> >>> >         # save the attached .config to linux build tree
> >>> >         make.cross ARCH=blackfin
> >>> >
> >>> > All errors (new ones prefixed by >>):
> >>> >
> >>> >    kernel/built-in.o: In function `core_kernel_rodata':
> >>> >>> kernel/extable.c:169: undefined reference to
> >>> >>> `__start_data_ro_after_init'
> >>> >>> kernel/extable.c:169: undefined reference to
> >>> >>> `__start_data_ro_after_init'
> >>> >>> kernel/extable.c:169: undefined reference to
> >>> >>> `__end_data_ro_after_init'
> >>> >>> kernel/extable.c:169: undefined reference to
> >>> >>> `__end_data_ro_after_init'
> >>> >>> kernel/extable.c:169: undefined reference to
> >>> >>> `__start_data_ro_after_init'
> >>> >>> kernel/extable.c:169: undefined reference to
> >>> >>> `__start_data_ro_after_init'
> >>> >>> kernel/extable.c:169: undefined reference to
> >>> >>> `__end_data_ro_after_init'
> >>> >>> kernel/extable.c:169: undefined reference to
> >>> >>> `__end_data_ro_after_init'
> >>>
> >>> Hm, I'm confused about this. blackfin includes
> >>> include/asm-generic-vmlinux.lds.h and uses the RO_DATA macro (which
> >>> resolves to RO_DATA_SECTION to RO_AFTER_INIT_DATA which defines
> >>> __[start|end]_data_ro_after_init.
> >>>
> >>> Also, it seems that commit d7c19b066dcf4bd19c4385e8065558d4e74f9e73
> >>> ("mm: kmemleak: scan .data.ro_after_init") added a potentially
> >>> redundant section name (s390 already calls this
> >>> __[start|end]_ro_after_init). I'd like to get this cleaned up, since
> >>> having multiple names for the same thing is confusing:
> >>>
> >>> diff --git a/arch/s390/kernel/vmlinux.lds.S
> >>> b/arch/s390/kernel/vmlinux.lds.S
> >>> index 000e6e91f6a0..3667d20e997f 100644
> >>> --- a/arch/s390/kernel/vmlinux.lds.S
> >>> +++ b/arch/s390/kernel/vmlinux.lds.S
> >>> @@ -62,9 +62,11 @@ SECTIONS
> >>>
> >>>         . = ALIGN(PAGE_SIZE);
> >>>         __start_ro_after_init = .;
> >>> +       __start_data_ro_after_init = .;
> >>>         .data..ro_after_init : {
> >>>                  *(.data..ro_after_init)
> >>>         }
> >>> +       __end_data_ro_after_init = .;
> >>>         EXCEPTION_TABLE(16)
> >>>         . = ALIGN(PAGE_SIZE);
> >>>         __end_ro_after_init = .;
> >>>
> >>> And it seems that this hunk is wrong (__end_ro_after_init includes
> >>> s390's exception table, etc). I think we should remove the
> >>> ..._data_... name and use s390's name.
> >>>
> >>> I'll send an adjustment patch, but we'll still need to deal with
> >>> blackfin.
> >>>
> >>> -Kees
> >>>
> >>
> >> Kees
> >>
> >> I applied your patch (mm: fix section name for .data..ro_after_init) and
> >> changed the new function in extable.c to use __[start|end]_ro_after_init
> >> instead. The new version still builds without errors on x86, which isn't
> >> surprising.
> >>
> >> I've cross compiled this for blackfin and I'm able to reproduce the
> >> build error. I'm still not sure why. As you pointed out, blackfin does
> >> appear to use 'include/asm-generic/vmlinux.lds.h'.
> >
> >
> > This appears to be because blackfin is one of the 2 arches that
> > prepends an underscore '_' to all symbols defined in C. I noticed that
> > __{start,end}_data_ro_after_init in vmlinux.lds.h are not wrapped with
> > VMLINUX_SYMBOL() which adds the necessary prefix for arches that have
> > HAVE_UNDERSCORE_SYMBOL_PREFIX, hence the undefined reference.
> 
> Argh. Thank you for catching this! Yeah, that would have taken me
> forever to find.
> 
> > The below patch fixed the build error for me, if it works for you then
> > I can send a formal patch.
> >
> > diff --git a/include/asm-generic/vmlinux.lds.h
> > b/include/asm-generic/vmlinux.lds.h
> > index 4e09b28..7b262f7 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -260,9 +260,9 @@
> >  */
> > #ifndef RO_AFTER_INIT_DATA
> > #define RO_AFTER_INIT_DATA                                              \
> > -       __start_data_ro_after_init = .;                                 \
> > +       VMLINUX_SYMBOL(__start_data_ro_after_init) = .;                 \
> >         *(.data..ro_after_init)                                         \
> > -       __end_data_ro_after_init = .;
> > +       VMLINUX_SYMBOL(__end_data_ro_after_init) = .;
> > #endif
> 
> I don't have a blackfin cross-compiler set up, but I'm sure that'll
> fix it. If you can, please base it on -next, since I rename
> __[start|end]_data_ro_after_init to __[start|end]_ro_after_init (to
> match the existing s390 symbols of the same purpose):
> 
> https://lkml.org/lkml/2017/3/27/685
> 
I applied Jessica's patch and tested this again with a blackfin cross-compiler.
It fixes the error building extable.c.


> akpm is carrying that patch, so this follow-up should likely go to him too.
> 
> Thanks!
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 2/2] extable: verify address is read-only
  2017-03-30  3:27         ` Jessica Yu
  2017-03-30 16:24           ` Kees Cook
@ 2017-04-03 22:10           ` Kees Cook
  1 sibling, 0 replies; 11+ messages in thread
From: Kees Cook @ 2017-04-03 22:10 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Jakub Kicinski, Catalin Marinas, Heiko Carstens, kbuild-all,
	kbuild test robot, Rusty Russell, LKML, kernel-hardening

On Wed, Mar 29, 2017 at 8:27 PM, Jessica Yu <jeyu@redhat.com> wrote:
> +++ Eddie Kovsky [28/03/17 21:28 -0600]:
>
>> On 03/27/17, Kees Cook wrote:
>>>
>>> On Mon, Mar 27, 2017 at 1:43 AM, kbuild test robot <lkp@intel.com> wrote:
>>> > Hi Eddie,
>>> >
>>> > [auto build test ERROR on next-20170323]
>>> > [cannot apply to linus/master linux/master jeyu/modules-next v4.9-rc8
>>> > v4.9-rc7 v4.9-rc6 v4.11-rc4]
>>> > [if your patch is applied to the wrong git tree, please drop us a note
>>> > to help improve the system]
>>> >
>>> > url:
>>> > https://github.com/0day-ci/linux/commits/Eddie-Kovsky/module-verify-address-is-read-only/20170327-142922
>>> > config: blackfin-BF561-EZKIT-SMP_defconfig (attached as .config)
>>> > compiler: bfin-uclinux-gcc (GCC) 6.2.0
>>> > reproduce:
>>> >         wget
>>> > https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O
>>> > ~/bin/make.cross
>>> >         chmod +x ~/bin/make.cross
>>> >         # save the attached .config to linux build tree
>>> >         make.cross ARCH=blackfin
>>> >
>>> > All errors (new ones prefixed by >>):
>>> >
>>> >    kernel/built-in.o: In function `core_kernel_rodata':
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>>
>>> Hm, I'm confused about this. blackfin includes
>>> include/asm-generic-vmlinux.lds.h and uses the RO_DATA macro (which
>>> resolves to RO_DATA_SECTION to RO_AFTER_INIT_DATA which defines
>>> __[start|end]_data_ro_after_init.
>>>
>>> Also, it seems that commit d7c19b066dcf4bd19c4385e8065558d4e74f9e73
>>> ("mm: kmemleak: scan .data.ro_after_init") added a potentially
>>> redundant section name (s390 already calls this
>>> __[start|end]_ro_after_init). I'd like to get this cleaned up, since
>>> having multiple names for the same thing is confusing:
>>>
>>> diff --git a/arch/s390/kernel/vmlinux.lds.S
>>> b/arch/s390/kernel/vmlinux.lds.S
>>> index 000e6e91f6a0..3667d20e997f 100644
>>> --- a/arch/s390/kernel/vmlinux.lds.S
>>> +++ b/arch/s390/kernel/vmlinux.lds.S
>>> @@ -62,9 +62,11 @@ SECTIONS
>>>
>>>         . = ALIGN(PAGE_SIZE);
>>>         __start_ro_after_init = .;
>>> +       __start_data_ro_after_init = .;
>>>         .data..ro_after_init : {
>>>                  *(.data..ro_after_init)
>>>         }
>>> +       __end_data_ro_after_init = .;
>>>         EXCEPTION_TABLE(16)
>>>         . = ALIGN(PAGE_SIZE);
>>>         __end_ro_after_init = .;
>>>
>>> And it seems that this hunk is wrong (__end_ro_after_init includes
>>> s390's exception table, etc). I think we should remove the
>>> ..._data_... name and use s390's name.
>>>
>>> I'll send an adjustment patch, but we'll still need to deal with
>>> blackfin.
>>>
>>> -Kees
>>>
>>
>> Kees
>>
>> I applied your patch (mm: fix section name for .data..ro_after_init) and
>> changed the new function in extable.c to use __[start|end]_ro_after_init
>> instead. The new version still builds without errors on x86, which isn't
>> surprising.
>>
>> I've cross compiled this for blackfin and I'm able to reproduce the
>> build error. I'm still not sure why. As you pointed out, blackfin does
>> appear to use 'include/asm-generic/vmlinux.lds.h'.
>
>
> This appears to be because blackfin is one of the 2 arches that
> prepends an underscore '_' to all symbols defined in C. I noticed that
> __{start,end}_data_ro_after_init in vmlinux.lds.h are not wrapped with
> VMLINUX_SYMBOL() which adds the necessary prefix for arches that have
> HAVE_UNDERSCORE_SYMBOL_PREFIX, hence the undefined reference.
>
> The below patch fixed the build error for me, if it works for you then
> I can send a formal patch.
>
> diff --git a/include/asm-generic/vmlinux.lds.h
> b/include/asm-generic/vmlinux.lds.h
> index 4e09b28..7b262f7 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -260,9 +260,9 @@
>  */
> #ifndef RO_AFTER_INIT_DATA
> #define RO_AFTER_INIT_DATA                                              \
> -       __start_data_ro_after_init = .;                                 \
> +       VMLINUX_SYMBOL(__start_data_ro_after_init) = .;                 \
>         *(.data..ro_after_init)                                         \
> -       __end_data_ro_after_init = .;
> +       VMLINUX_SYMBOL(__end_data_ro_after_init) = .;
> #endif
>
> /*
>
> Jessica

Do you have a moment to send this? I'd like to get it in for v4.11 (my
renaming has landed, so you'll have to rebase). If not, I can do it.
:)

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-04-03 22:10 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-26 21:08 [PATCH v4 0/2] provide check for ro_after_init memory sections Eddie Kovsky
2017-03-26 21:08 ` [PATCH v4 1/2] module: verify address is read-only Eddie Kovsky
2017-03-30  3:31   ` Jessica Yu
2017-03-26 21:08 ` [PATCH v4 2/2] extable: " Eddie Kovsky
2017-03-27  8:43   ` kbuild test robot
2017-03-27 18:42     ` Kees Cook
2017-03-29  3:28       ` Eddie Kovsky
2017-03-30  3:27         ` Jessica Yu
2017-03-30 16:24           ` Kees Cook
2017-03-31  3:09             ` Eddie Kovsky
2017-04-03 22:10           ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).