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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Tue, Apr 1, 2014 at 11:03 AM, Laura Abbott <[email protected]> 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 <[email protected]>
>> ---
>> 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
Am 01.04.2014 20:36, schrieb Kees Cook:
> On Tue, Apr 1, 2014 at 11:03 AM, Laura Abbott <[email protected]> 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
2014-04-01 20:36 GMT+02:00 Kees Cook <[email protected]>:
> 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 <[email protected]>
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
2014-04-01 20:36 GMT+02:00 Kees Cook <[email protected]>:
> On Tue, Apr 1, 2014 at 11:03 AM, Laura Abbott <[email protected]> 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}()
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
On Tue, Apr 1, 2014 at 4:08 PM, Rabin Vincent <[email protected]> wrote:
> 2014-04-01 20:36 GMT+02:00 Kees Cook <[email protected]>:
>> 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
On Thu, Apr 03, 2014 at 04:14:54PM -0700, Kees Cook wrote:
> On Tue, Apr 1, 2014 at 4:08 PM, Rabin Vincent <[email protected]> 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
On Thu, Apr 3, 2014 at 4:48 PM, Rabin Vincent <[email protected]> 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 <[email protected]> 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