[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Alexander Holler]

CONFIG_DEBUG_SET_MODULE_RONX sounds like a nice security feature, but
things might fail late (and unexpected) if module code is set to read-only
while CONFIG_JUMP_LABEL is enabled (e.g. modprobe bridge).

Avoid this.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 arch/arm/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 0531da8..6627b9e 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1197,7 +1197,7 @@ config PID_IN_CONTEXTIDR
 
 config DEBUG_SET_MODULE_RONX
 	bool "Set loadable kernel module data as NX and text as RO"
-	depends on MODULES
+	depends on MODULES && !JUMP_LABEL
 	---help---
 	  This option helps catch unintended modifications to loadable
 	  kernel module's text and read-only data. It also prevents execution
-- 
1.8.3.1

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Laura Abbott]

On 4/1/2014 3:04 AM, Alexander Holler wrote:
> CONFIG_DEBUG_SET_MODULE_RONX sounds like a nice security feature, but
> things might fail late (and unexpected) if module code is set to read-only
> while CONFIG_JUMP_LABEL is enabled (e.g. modprobe bridge).
> 
> Avoid this.
> 
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> ---
>  arch/arm/Kconfig.debug | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index 0531da8..6627b9e 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1197,7 +1197,7 @@ config PID_IN_CONTEXTIDR
>  
>  config DEBUG_SET_MODULE_RONX
>  	bool "Set loadable kernel module data as NX and text as RO"
> -	depends on MODULES
> +	depends on MODULES && !JUMP_LABEL
>  	---help---
>  	  This option helps catch unintended modifications to loadable
>  	  kernel module's text and read-only data. It also prevents execution
> 

Kees Cook has something similar[1] for not-module space as well, we probably want
this there as well. A shame we keep finding reasons these features will be turned
off. Looks good to me otherwise. 

Laura

[1]http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/232644.html

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Kees Cook]

On Tue, Apr 1, 2014 at 11:03 AM, Laura Abbott <lauraa@codeaurora.org> wrote:
> On 4/1/2014 3:04 AM, Alexander Holler wrote:
>> CONFIG_DEBUG_SET_MODULE_RONX sounds like a nice security feature, but
>> things might fail late (and unexpected) if module code is set to read-only
>> while CONFIG_JUMP_LABEL is enabled (e.g. modprobe bridge).

Isn't this a ordering problem? I thought jump labels got set up once,
and then after that, the memory could be made RO?

>>
>> Avoid this.
>>
>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>> ---
>>  arch/arm/Kconfig.debug | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
>> index 0531da8..6627b9e 100644
>> --- a/arch/arm/Kconfig.debug
>> +++ b/arch/arm/Kconfig.debug
>> @@ -1197,7 +1197,7 @@ config PID_IN_CONTEXTIDR
>>
>>  config DEBUG_SET_MODULE_RONX
>>       bool "Set loadable kernel module data as NX and text as RO"
>> -     depends on MODULES
>> +     depends on MODULES && !JUMP_LABEL
>>       ---help---
>>         This option helps catch unintended modifications to loadable
>>         kernel module's text and read-only data. It also prevents execution
>>
>
> Kees Cook has something similar[1] for not-module space as well, we probably want
> this there as well. A shame we keep finding reasons these features will be turned
> off. Looks good to me otherwise.
>
> Laura
>
> [1]http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/232644.html

I've solidly stumbled over ftrace with my series now. I can't figure
out why it doesn't work, though. I wrote code to flip the PMD flags
back to writable (via set_kernel_text_rw), but it seems like the
system is ignoring the changes, even though I call flush_pmd_entry.

struct section_perm section_perms[] = {
       /* Make kernel code and rodata RX (set RO). */
       [ARM_TEXT_SECTION] = {
              .start       = (unsigned long)_stext,
              .end       = (unsigned long)__init_begin,
#ifdef CONFIG_ARM_LPAE
              .mask       = ~PMD_SECT_RDONLY,
              .prot       = PMD_SECT_RDONLY,
#else
              .mask       = ~(PMD_SECT_APX | PMD_SECT_AP_WRITE),
              .prot       = PMD_SECT_APX | PMD_SECT_AP_WRITE,
              .clear       = PMD_SECT_AP_WRITE,
#endif
       },

...

static inline void section_update(unsigned long addr, pmdval_t mask,
                              pmdval_t prot)
{
       pmd_t *pmd = pmd_off_k(addr);

#ifdef CONFIG_ARM_LPAE
       pmd[0] = __pmd((pmd_val(pmd[0]) & mask) | prot);
#else
       if (addr & SECTION_SIZE)
              pmd[1] = __pmd((pmd_val(pmd[1]) & mask) | prot);
       else
              pmd[0] = __pmd((pmd_val(pmd[0]) & mask) | prot);
#endif
       flush_pmd_entry(pmd);
}

...

void set_kernel_text_rw(void)
{
       unsigned long addr;

       if (!arch_can_set_perms())
              return;

       for (addr = section_perms[ARM_TEXT_SECTION].start;
            addr < section_perms[ARM_TEXT_SECTION].end;
            addr += SECTION_SIZE)
              section_update(addr, section_perms[ARM_TEXT_SECTION].mask,
                            section_perms[ARM_TEXT_SECTION].clear);
}

Is there something "sticky" about PMD sections that I'm not aware of?
Even after calling set_kernel_text_rw(), any writes to kernel memory
fault. :(

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Alexander Holler]

Am 01.04.2014 20:36, schrieb Kees Cook:
> On Tue, Apr 1, 2014 at 11:03 AM, Laura Abbott <lauraa@codeaurora.org> wrote:
>> On 4/1/2014 3:04 AM, Alexander Holler wrote:
>>> CONFIG_DEBUG_SET_MODULE_RONX sounds like a nice security feature, but
>>> things might fail late (and unexpected) if module code is set to read-only
>>> while CONFIG_JUMP_LABEL is enabled (e.g. modprobe bridge).
>
> Isn't this a ordering problem? I thought jump labels got set up once,
> and then after that, the memory could be made RO?

I basically just run into that and looked up what happened. But the 
problem appears e.g. in netfiler/core.c function nf_register_hook() 
which calls static_key_slow_inc(). So you would have to make sure 
nf_register_hook() will be called before the code is set ro. Something 
that doesn't look easy to do.

I would have to look up when that might be called, but I assume there 
are many ways to register and unregister hooks in netfilter and some of 
them might happen outside any init, probe or whatever one might set the 
code read-only afterwards. You would have to set the code rw too, before 
nf_unregister_hook() happens.

Maybe it's possible to mark some modules to not become ro at all, I 
don't know. And doing so would make them a prefered target for exploits. 
So I'm not sure if it would make sense.

Regards,

Alexander Holler

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Rabin Vincent]

2014-04-01 20:36 GMT+02:00 Kees Cook <keescook@chromium.org>:
> Is there something "sticky" about PMD sections that I'm not aware of?
> Even after calling set_kernel_text_rw(), any writes to kernel memory
> fault. :(

section_update() updates init_mm, but each mm has a copy of the first
level page tables.  So your updates to init_mm won't be visibile to
currently running processes.  (Have a look at arch/arm/mm/ioremap.c's
vmalloc_seq stuff for some background on how section support is
handled on non-SMP; the vmalloc code doesn't use sections on SMP.)

Here's a patch (probably whitespace damaged, hence also attached) with
which dynamic ftrace works for me on top your other paches.  Tested on
a non-LPAE SMP.

Notes:
- I commented out the other entries in section_perm except from the
kernel text only because I didn't want to figure out the mask values
to use
- I didn't/couldn't call set_kernel_text_rw in
ftrace_arch_code_modify_prepare() because that is called outside of
stop_machine(), and stop_machine() triggers another thread which
actually runs ftrace_modify_all_code() with the machine stopped.


>From 91d92b8e241835013cefaca0c5121b9d6df9d500 Mon Sep 17 00:00:00 2001
From: Rabin Vincent <rabin@rab.in>
Date: Wed, 2 Apr 2014 00:57:09 +0200
Subject: [PATCH] ftrace

---
 arch/arm/kernel/ftrace.c | 21 +++++++++++++
 arch/arm/mm/init.c       | 77 ++++++++++++++++++++++++++++++++++--------------
 2 files changed, 76 insertions(+), 22 deletions(-)

diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 34e5664..61cb8ef 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -14,6 +14,7 @@

 #include <linux/ftrace.h>
 #include <linux/uaccess.h>
+#include <linux/stop_machine.h>

 #include <asm/cacheflush.h>
 #include <asm/opcodes.h>
@@ -34,6 +35,26 @@

 #define OLD_NOP 0xe1a00000 /* mov r0, r0 */

+static int __ftrace_modify_code(void *data)
+{
+ extern void set_kernel_text_rw(struct mm_struct *mm);
+ extern void set_kernel_text_ro(struct mm_struct *mm);
+
+ struct mm_struct *mm = current->active_mm;
+ int *command = data;
+
+ set_kernel_text_rw(mm);
+ ftrace_modify_all_code(*command);
+ set_kernel_text_ro(mm);
+
+ return 0;
+}
+
+void arch_ftrace_update_code(int command)
+{
+ stop_machine(__ftrace_modify_code, &command, NULL);
+}
+
 static unsigned long ftrace_nop_replace(struct dyn_ftrace *rec)
 {
  return rec->arch.old_mcount ? OLD_NOP : NOP;
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 5b1b049..c4da92d 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -630,22 +630,24 @@ void __init mem_init(void)
 struct section_perm {
  unsigned long start;
  unsigned long end;
+ pmdval_t mask;
  pmdval_t prot;
+ pmdval_t clear;
 };

-struct section_perm __initdata section_perms[] = {
+struct section_perm section_perms[] = {
  /* Make pages tables, etc before _stext RW (set NX). */
- {
- .start = PAGE_OFFSET,
- .end = (unsigned long)_stext,
- .prot = PMD_SECT_XN,
- },
- /* Make init RW (set NX). */
- {
- .start = (unsigned long)__init_begin,
- .end = (unsigned long)_sdata,
- .prot = PMD_SECT_XN,
- },
+ // {
+ // .start = PAGE_OFFSET,
+ // .end = (unsigned long)_stext,
+ // .prot = PMD_SECT_XN,
+ // },
+ // /* Make init RW (set NX). */
+ // {
+ // .start = (unsigned long)__init_begin,
+ // .end = (unsigned long)_sdata,
+ // .prot = PMD_SECT_XN,
+ // },
  /* Make kernel code and rodata RX (set RO). */
  {
  .start = (unsigned long)_stext,
@@ -653,30 +655,37 @@ struct section_perm __initdata section_perms[] = {
 #ifdef CONFIG_ARM_LPAE
  .prot = PMD_SECT_RDONLY,
 #else
+ .mask       = ~(PMD_SECT_APX | PMD_SECT_AP_WRITE),
  .prot = PMD_SECT_APX | PMD_SECT_AP_WRITE,
+ .clear       = PMD_SECT_AP_WRITE,
 #endif
  },
 #ifdef CONFIG_DEBUG_RODATA
  /* Make rodata RO (set NX). */
- {
- .start = (unsigned long)__start_rodata,
- .end = (unsigned long)__init_begin,
- .prot = PMD_SECT_XN,
- }
+ // {
+ // .start = (unsigned long)__start_rodata,
+ // .end = (unsigned long)__init_begin,
+ // .prot = PMD_SECT_XN,
+ // }
 #endif
 };

-static inline void section_update(unsigned long addr, pmdval_t prot)
+static inline pmd_t *pmd_off(struct mm_struct *mm, unsigned long virt)
+{
+ return pmd_offset(pud_offset(pgd_offset(mm, virt), virt), virt);
+}
+
+static inline void section_update(struct mm_struct *mm, unsigned long
addr, pmdval_t mask, pmdval_t prot)
 {
- pmd_t *pmd = pmd_off_k(addr);
+ pmd_t *pmd = pmd_off(mm, addr);

 #ifdef CONFIG_ARM_LPAE
  pmd[0] = __pmd(pmd_val(pmd[0]) | prot);
 #else
  if (addr & SECTION_SIZE)
- pmd[1] = __pmd(pmd_val(pmd[1]) | prot);
+ pmd[1] = __pmd((pmd_val(pmd[1]) & mask) | prot);
  else
- pmd[0] = __pmd(pmd_val(pmd[0]) | prot);
+ pmd[0] = __pmd((pmd_val(pmd[0]) & mask) | prot);
 #endif
  flush_pmd_entry(pmd);
 }
@@ -702,13 +711,37 @@ static inline void fix_kernmem_perms(void)
  for (addr = section_perms[i].start;
      addr < section_perms[i].end;
      addr += SECTION_SIZE)
- section_update(addr, section_perms[i].prot);
+ section_update(&init_mm, addr, section_perms[i].mask, section_perms[i].prot);
  }
 }
 #else
 static inline void fix_kernmem_perms(void) { }
 #endif /* CONFIG_ARM_KERNMEM_PERMS */

+void set_kernel_text_rw(struct mm_struct *mm)
+{
+       unsigned long addr;
+
+       for (addr = section_perms[0].start;
+            addr < section_perms[0].end;
+            addr += SECTION_SIZE)
+              section_update(mm, addr, section_perms[0].mask,
+                            section_perms[0].clear);
+
+       flush_tlb_all();
+}
+
+void set_kernel_text_ro(struct mm_struct *mm)
+{
+       unsigned long addr;
+
+       for (addr = section_perms[0].start;
+            addr < section_perms[0].end;
+            addr += SECTION_SIZE)
+              section_update(mm, addr, section_perms[0].mask,
+                            section_perms[0].prot);
+}
+
 void free_initmem(void)
 {
 #ifdef CONFIG_HAVE_TCM
-- 
1.9.0
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ftrace.patch
Type: text/x-patch
Size: 4803 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140402/a3b57122/attachment.bin>

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Alexander Holler]

Am 02.04.2014 01:08, schrieb Rabin Vincent:

> +void set_kernel_text_rw(struct mm_struct *mm)

(...)

> +void set_kernel_text_ro(struct mm_struct *mm)

(...)


Hmm, maybe such is doable for jump labels too. But I definitely can't 
discuss on that topic further through missing knowledge. ;)

Regards,

Alexander Holler

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Kees Cook]

On Tue, Apr 1, 2014 at 4:08 PM, Rabin Vincent <rabin@rab.in> wrote:
> 2014-04-01 20:36 GMT+02:00 Kees Cook <keescook@chromium.org>:
>> Is there something "sticky" about PMD sections that I'm not aware of?
>> Even after calling set_kernel_text_rw(), any writes to kernel memory
>> fault. :(
>
> section_update() updates init_mm, but each mm has a copy of the first
> level page tables.  So your updates to init_mm won't be visibile to
> currently running processes.  (Have a look at arch/arm/mm/ioremap.c's
> vmalloc_seq stuff for some background on how section support is
> handled on non-SMP; the vmalloc code doesn't use sections on SMP.)

Ah-ha! This is the missing piece of information for me. :)

> Here's a patch (probably whitespace damaged, hence also attached) with
> which dynamic ftrace works for me on top your other paches.  Tested on
> a non-LPAE SMP.

Thanks! I'll incorporate calls to set_all_modules_text_rw/ro and see
what I can make work.

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Rabin Vincent]

On Thu, Apr 03, 2014 at 04:14:54PM -0700, Kees Cook wrote:
> On Tue, Apr 1, 2014 at 4:08 PM, Rabin Vincent <rabin@rab.in> wrote:
> > Here's a patch (probably whitespace damaged, hence also attached) with
> > which dynamic ftrace works for me on top your other paches.  Tested on
> > a non-LPAE SMP.
> 
> Thanks! I'll incorporate calls to set_all_modules_text_rw/ro and see
> what I can make work.

You can use the patch I posted yesterday for module support:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/244558.html

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Kees Cook]

On Thu, Apr 3, 2014 at 4:48 PM, Rabin Vincent <rabin@rab.in> wrote:
> On Thu, Apr 03, 2014 at 04:14:54PM -0700, Kees Cook wrote:
>> On Tue, Apr 1, 2014 at 4:08 PM, Rabin Vincent <rabin@rab.in> wrote:
>> > Here's a patch (probably whitespace damaged, hence also attached) with
>> > which dynamic ftrace works for me on top your other paches.  Tested on
>> > a non-LPAE SMP.
>>
>> Thanks! I'll incorporate calls to set_all_modules_text_rw/ro and see
>> what I can make work.
>
> You can use the patch I posted yesterday for module support:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/244558.html

Yup, that's basically what I had too.

So, even with your patch, I still get faults with ftrace.
probe_kernel_write still errors for me, and I don't see why. I will
send an updated patch series with my current CONFIG_DEBUG_RODATA work
shortly...

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH] arm: don't allow CONFIG_DEBUG_SET_MODULE_RONX if CONFIG_JUMP_LABEL is enabled

[Rabin Vincent]

2014-04-01 20:36 GMT+02:00 Kees Cook <keescook@chromium.org>:
> On Tue, Apr 1, 2014 at 11:03 AM, Laura Abbott <lauraa@codeaurora.org> wrote:
>> On 4/1/2014 3:04 AM, Alexander Holler wrote:
>>> CONFIG_DEBUG_SET_MODULE_RONX sounds like a nice security feature, but
>>> things might fail late (and unexpected) if module code is set to read-only
>>> while CONFIG_JUMP_LABEL is enabled (e.g. modprobe bridge).
>
> Isn't this a ordering problem? I thought jump labels got set up once,
> and then after that, the memory could be made RO?

The code gets patched every time the jump label is enabled/disabled,
i.e. every time the static_key is turned on/off based on
static_key_slow_{inc/dec}()