On Wed, 28 Nov 2018 18:59:30 +0000 Nadav Amit <[email protected]> wrote:
> > On Nov 20, 2018, at 12:35 PM, Nadav Amit <[email protected]> wrote: > > > > When modules and BPF filters are loaded, there is a time window in > > which some memory is both writable and executable. An attacker that has > > already found another vulnerability (e.g., a dangling pointer) might be > > able to exploit this behavior to overwrite kernel code. This patch > > prevents having writable executable PTEs in this stage. > > > > In addition, avoiding having R+X mappings can also slightly simplify the > > patching of modules code on initialization (e.g., by alternatives and > > static-key), as would be done in the next patch. > > > > To avoid having W+X mappings, set them initially as RW (NX) and after > > they are set as RO set them as X as well. Setting them as executable is > > done as a separate step to avoid one core in which the old PTE is cached > > (hence writable), and another which sees the updated PTE (executable), > > which would break the W^X protection. > > > > Cc: Andy Lutomirski <[email protected]> > > Cc: Kees Cook <[email protected]> > > Cc: Peter Zijlstra <[email protected]> > > Cc: Dave Hansen <[email protected]> > > Cc: Masami Hiramatsu <[email protected]> > > Suggested-by: Thomas Gleixner <[email protected]> > > Suggested-by: Andy Lutomirski <[email protected]> > > Signed-off-by: Nadav Amit <[email protected]> > > --- > > arch/x86/kernel/alternative.c | 28 +++++++++++++++++++++------- > > arch/x86/kernel/module.c | 2 +- > > include/linux/filter.h | 6 ++++++ > > kernel/module.c | 10 ++++++++++ > > 4 files changed, 38 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > > index 8fc4685f3117..18415e3b6000 100644 > > --- a/arch/x86/kernel/alternative.c > > +++ b/arch/x86/kernel/alternative.c > > @@ -667,15 +667,29 @@ void __init alternative_instructions(void) > > * handlers seeing an inconsistent instruction while you patch. > > */ > > void *__init_or_module text_poke_early(void *addr, const void *opcode, > > - size_t len) > > + size_t len) > > { > > unsigned long flags; > > - local_irq_save(flags); > > - memcpy(addr, opcode, len); > > - local_irq_restore(flags); > > - sync_core(); > > - /* Could also do a CLFLUSH here to speed up CPU recovery; but > > - that causes hangs on some VIA CPUs. */ > > + > > + if (static_cpu_has(X86_FEATURE_NX) && > > + is_module_text_address((unsigned long)addr)) { > > + /* > > + * Modules text is marked initially as non-executable, so the > > + * code cannot be running and speculative code-fetches are > > + * prevented. We can just change the code. > > + */ > > + memcpy(addr, opcode, len); > > + } else { > > + local_irq_save(flags); > > + memcpy(addr, opcode, len); > > + local_irq_restore(flags); > > + sync_core(); > > + > > + /* > > + * Could also do a CLFLUSH here to speed up CPU recovery; but > > + * that causes hangs on some VIA CPUs. > > + */ > > + } > > return addr; > > } > > > > diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c > > index b052e883dd8c..cfa3106faee4 100644 > > --- a/arch/x86/kernel/module.c > > +++ b/arch/x86/kernel/module.c > > @@ -87,7 +87,7 @@ void *module_alloc(unsigned long size) > > p = __vmalloc_node_range(size, MODULE_ALIGN, > > MODULES_VADDR + get_module_load_offset(), > > MODULES_END, GFP_KERNEL, > > - PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE, > > + PAGE_KERNEL, 0, NUMA_NO_NODE, > > __builtin_return_address(0)); > > if (p && (kasan_module_alloc(p, size) < 0)) { > > vfree(p); > > diff --git a/include/linux/filter.h b/include/linux/filter.h > > index de629b706d1d..ee9ae03c5f56 100644 > > --- a/include/linux/filter.h > > +++ b/include/linux/filter.h > > @@ -704,7 +704,13 @@ static inline void bpf_prog_unlock_ro(struct bpf_prog > > *fp) > > > > static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr) > > { > > + /* > > + * Perform mapping changes in two stages to avoid opening a time-window > > + * in which a PTE is cached in any TLB as writable, but marked as > > + * executable in the memory-resident mappings (e.g., page-tables). > > + */ > > set_memory_ro((unsigned long)hdr, hdr->pages); > > + set_memory_x((unsigned long)hdr, hdr->pages); > > } > > > > static inline void bpf_jit_binary_unlock_ro(struct bpf_binary_header *hdr) > > diff --git a/kernel/module.c b/kernel/module.c > > index 49a405891587..7cb207249437 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -1946,9 +1946,19 @@ void module_enable_ro(const struct module *mod, bool > > after_init) > > if (!rodata_enabled) > > return; > > > > + /* > > + * Perform mapping changes in two stages to avoid opening a time-window > > + * in which a PTE is cached in any TLB as writable, but marked as > > + * executable in the memory-resident mappings (e.g., page-tables). > > + */ > > frob_text(&mod->core_layout, set_memory_ro); > > + frob_text(&mod->core_layout, set_memory_x); > > + > > frob_rodata(&mod->core_layout, set_memory_ro); > > + > > frob_text(&mod->init_layout, set_memory_ro); > > + frob_text(&mod->init_layout, set_memory_x); > > + > > frob_rodata(&mod->init_layout, set_memory_ro); > > > > if (after_init) > > -- > > 2.17.1 > > Rick pointed out that I screwed up ftrace and kprobes. > > For kprobes, I think I need to add set_memory_x() to alloc_insn_page() and > change arch_ftrace_update_trampoline(). Oops, right. It should be easy to fix for kprobe :) what you need is below. diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index c33b06f..51818f3 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -431,8 +431,10 @@ void *alloc_insn_page(void) void *page; page = module_alloc(PAGE_SIZE); - if (page) + if (page) { set_memory_ro((unsigned long)page & PAGE_MASK, 1); + set_memory_x((unsigned long)page & PAGE_MASK, 1); + } return page; } > > For arch_ftrace_update_trampoline(), I think I should remove not use > set_memory_rw() when patching in __probe_kernel_write() should be done > through text_poke(). right. > > I’ll give it another look and send another version later. Thank you, > > Regards, > Nadav > > -- Masami Hiramatsu <[email protected]>
