Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM
Subsystems (2)
cover Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM
Hi,
This is an RFC to allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM. The current config description from x86 describes it best:
This option helps catch unintended modifications to loadable kernel module's text and read-only data. It also prevents execution of module data. Such protection may interfere with run-time code patching and dynamic kernel tracing - and they might also protect against certain classes of kernel exploits.
ARM was missing a few functions to modify the page tables so those have been added. I believe modules are always mapped with pages so changing them at map time should be acceptable. Comments/concerns are appreciated.
Thanks,
Laura
---
arch/arm/Kconfig.debug | 11 +++++ arch/arm/include/asm/cacheflush.h | 5 ++ arch/arm/include/asm/pgtable.h | 2 + arch/arm/mm/mmu.c | 86 +++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 0 deletions(-)
RFC 1/3 arm: Add definitions for pte_mkexec/pte_mknexec
Other architectures define pte_mkexec to mark a pte as executable. Add pte_mkexec for ARM to get the same functionality. Although no other architectures currently define it, also add pte_mknexec to explicitly allow a pte to be marked as non executable.
--- arch/arm/include/asm/pgtable.h | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
arch/arm/include/asm/pgtable.h +2 -0
@@ -251,6 +251,8 @@ PTE_BIT_FUNC(mkclean, &= ~L_PTE_DIRTY); PTE_BIT_FUNC(mkdirty, |= L_PTE_DIRTY); PTE_BIT_FUNC(mkold, &= ~L_PTE_YOUNG); PTE_BIT_FUNC(mkyoung, |= L_PTE_YOUNG); +PTE_BIT_FUNC(mkexec, &= ~L_PTE_XN); +PTE_BIT_FUNC(mknexec, |= L_PTE_XN);static inline pte_t pte_mkspecial (pte_t pte) {return pte; }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
RFC 2/3 arm: mm: Define set_memory_* functions for ARM
In reply to Laura AbbottOther architectures define various set_memory functions to allow attributes to be changed (e.g. set_memory_x, set_memory_rw, etc.) Currently, these functions are missing on ARM. Define these in an appropriate manner for ARM.
--- arch/arm/include/asm/cacheflush.h | 5 ++ arch/arm/mm/mmu.c | 86 +++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 0 deletions(-)
arch/arm/include/asm/cacheflush.h +5 -0
@@ -438,4 +438,9 @@ static inline void __sync_cache_range_r(volatile void *p, size_t size)#define sync_cache_w (ptr) __sync_cache_range_w(ptr, sizeof *(ptr))#define sync_cache_r (ptr) __sync_cache_range_r(ptr, sizeof *(ptr)) +int set_memory_ro (unsigned long addr,int numpages); +int set_memory_rw (unsigned long addr,int numpages); +int set_memory_x (unsigned long addr,int numpages); +int set_memory_nx (unsigned long addr,int numpages); +#endif
arch/arm/mm/mmu.c +86 -0
92 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
RFC 2/3 arm: mm: Define set_memory_* functions for ARM
Quoted 4 lines · Laura Abbott
Other architectures define various
set_memoryfunctions to allow attributes to be changed (e.g.set_memory_x,set_memory_rw, etc.) Currently, these functions are missing on ARM. Define these in an appropriate manner for ARM.
Please ensure that these functions only accept arguments for the module range; they will fail probably very noisily and destructively if they encounter the section mappings which we use for the direct RAM mapping.
RFC 2/3 arm: mm: Define set_memory_* functions for ARM
In reply to Laura Abbottarch/arm/mm/mmu.c const struct mem_type *get_mem_type(unsigned int type) · line 355
Quoted 9 lines · Laura Abbott
arch/arm/mm/mmu.c +9
-0+int set_memory_ro (unsigned long addr,int numpages) +{ +unsigned long start = addr; +unsigned long size =PAGE_SIZE *numpages; +unsigned end = start + size; + +apply_to_page_range (&init_mm, start, size, pte_set_ro,NULL ); +dsb (); +flush_tlb_kernel_range (start, end);
I think flush_tlb_kernel_range() already has a DSB.
Signature · 1 line
Catalin
RFC 2/3 arm: mm: Define set_memory_* functions for ARM
In reply to Laura AbbottQuoted 16 lines · Laura Abbott
Other architectures define various
set_memoryfunctions to allow attributes to be changed (e.g.set_memory_x,set_memory_rw, etc.) Currently, these functions are missing on ARM. Define these in an appropriate manner for ARM.Signed-off-by Laura Abbott <[email protected]>--- arch/arm/include/asm/cacheflush.h | 5 ++ arch/arm/mm/mmu.c | 86 +++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 0 deletions(-)arch/arm/include/asm/cacheflush.h +0
-0
arch/arm/include/asm/cacheflush.h static inline void __sync_cache_range_r(volatile void *p, size_t size) · line 444
Quoted 7 lines
arch/arm/include/asm/cacheflush.h +4
-0@@ -438,4 +438,9 @@ static inline void __sync_cache_range_r(volatile void *p, size_t size)#define sync_cache_w (ptr) __sync_cache_range_w(ptr, sizeof *(ptr))#define sync_cache_r (ptr) __sync_cache_range_r(ptr, sizeof *(ptr)) +int set_memory_ro (unsigned long addr,int numpages); +int set_memory_rw (unsigned long addr,int numpages); +int set_memory_x (unsigned long addr,int numpages); +int set_memory_nx (unsigned long addr,int numpages);
This seems like a pretty clunky interface with a horribly generic name, but that seems to be what x86 and s390 are using. I wonder if there would be any interest in tidying it up a bit? It really looks like something that is x86-specific but has started to grow users in core code (set_memory_4k?!).
Will
RFC 2/3 arm: mm: Define set_memory_* functions for ARM
Quoted 20 lines · Will Deacon
Quoted 19 lines · Laura Abbott
Other architectures define various
set_memoryfunctions to allow attributes to be changed (e.g.set_memory_x,set_memory_rw, etc.) Currently, these functions are missing on ARM. Define these in an appropriate manner for ARM.Signed-off-by Laura Abbott <[email protected]>--- arch/arm/include/asm/cacheflush.h | 5 ++ arch/arm/mm/mmu.c | 86 +++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 0 deletions(-)arch/arm/include/asm/cacheflush.h +0
-0@@ -438,4 +438,9 @@ static inline void __sync_cache_range_r(volatile void *p, size_t size)#define sync_cache_w (ptr) __sync_cache_range_w(ptr, sizeof *(ptr))#define sync_cache_r (ptr) __sync_cache_range_r(ptr, sizeof *(ptr))
Quoted 10 lines
arch/arm/include/asm/cacheflush.h static inline void __sync_cache_range_r(volatile void *p, size_t size) · line 444
Quoted 4 lines
arch/arm/include/asm/cacheflush.h +4
-0+int set_memory_ro (unsigned long addr,int numpages); +int set_memory_rw (unsigned long addr,int numpages); +int set_memory_x (unsigned long addr,int numpages); +int set_memory_nx (unsigned long addr,int numpages);This seems like a pretty clunky interface with a horribly generic name, but that seems to be what x86 and s390 are using. I wonder if there would be any interest in tidying it up a bit? It really looks like something that is x86-specific but has started to grow users in core code (
set_memory_4k?!).
I think cleanup would be beneficial. Nothing else really uses the set_memory_* functions and s390 explicitly defined them so they could use CONFIG_DEBUG_SET_MODULE_RONX as well. Perhaps the work I did with apply_to_page_range could apply across all architectures?
Thanks,
Laura
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
RFC 2/3 arm: mm: Define set_memory_* functions for ARM
Quoted 32 lines · Laura Abbott
Quoted 20 lines · Will Deacon
Quoted 19 lines · Laura Abbott
Other architectures define various
set_memoryfunctions to allow attributes to be changed (e.g.set_memory_x,set_memory_rw, etc.) Currently, these functions are missing on ARM. Define these in an appropriate manner for ARM.Signed-off-by Laura Abbott <[email protected]>--- arch/arm/include/asm/cacheflush.h | 5 ++ arch/arm/mm/mmu.c | 86 +++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 0 deletions(-)arch/arm/include/asm/cacheflush.h +0
-0@@ -438,4 +438,9 @@ static inline void __sync_cache_range_r(volatile void *p, size_t size)#define sync_cache_w (ptr) __sync_cache_range_w(ptr, sizeof *(ptr))#define sync_cache_r (ptr) __sync_cache_range_r(ptr, sizeof *(ptr))Quoted 10 lines
arch/arm/include/asm/cacheflush.h static inline void __sync_cache_range_r(volatile void *p, size_t size) · line 444
Quoted 4 lines
arch/arm/include/asm/cacheflush.h +4
-0+int set_memory_ro (unsigned long addr,int numpages); +int set_memory_rw (unsigned long addr,int numpages); +int set_memory_x (unsigned long addr,int numpages); +int set_memory_nx (unsigned long addr,int numpages);This seems like a pretty clunky interface with a horribly generic name, but that seems to be what x86 and s390 are using. I wonder if there would be any interest in tidying it up a bit? It really looks like something that is x86-specific but has started to grow users in core code (
set_memory_4k?!).
Quoted 4 lines
I think cleanup would be beneficial. Nothing else really uses the set_memory_* functions and s390 explicitly defined them so they could use
CONFIG_DEBUG_SET_MODULE_RONXas well. Perhaps the work I did withapply_to_page_rangecould apply across all architectures?
It's certainly worth a look, and would probably warrant changing the function prototypes to take the size rather than the number of pages too.
Will
RFC 2/3 arm: mm: Define set_memory_* functions for ARM
In reply to Laura AbbottHi Laura,
Quoted 4 lines · Laura Abbott
Other architectures define various
set_memoryfunctions to allow attributes to be changed (e.g.set_memory_x,set_memory_rw, etc.) Currently, these functions are missing on ARM. Define these in an appropriate manner for ARM.
[...]
arch/arm/mm/mmu.c const struct mem_type *get_mem_type(unsigned int type) · line 355
Quoted 9 lines
arch/arm/mm/mmu.c +9
-0+int set_memory_ro (unsigned long addr,int numpages) +{ +unsigned long start = addr; +unsigned long size =PAGE_SIZE *numpages; +unsigned end = start + size; + +apply_to_page_range (&init_mm, start, size, pte_set_ro,NULL ); +dsb (); +flush_tlb_kernel_range (start, end);
The TLB_WB flag gives you the dsb, so you don't need to code it explicitly here (same comment for the other occurrences in this patch).
Will
RFC 2/3 arm: mm: Define set_memory_* functions for ARM
Quoted 10 lines · Will Deacon
Hi Laura,
Quoted 4 lines · Laura Abbott
Other architectures define various
set_memoryfunctions to allow attributes to be changed (e.g.set_memory_x,set_memory_rw, etc.) Currently, these functions are missing on ARM. Define these in an appropriate manner for ARM.[...]
Quoted 12 lines
arch/arm/mm/mmu.c const struct mem_type *get_mem_type(unsigned int type) · line 355
Quoted 9 lines
arch/arm/mm/mmu.c +9
-0+int set_memory_ro (unsigned long addr,int numpages) +{ +unsigned long start = addr; +unsigned long size =PAGE_SIZE *numpages; +unsigned end = start + size; + +apply_to_page_range (&init_mm, start, size, pte_set_ro,NULL ); +dsb (); +flush_tlb_kernel_range (start, end);The
TLB_WBflag gives you the dsb, so you don't need to code it explicitly here (same comment for the other occurrences in this patch).
The version I have here doesn't have the above as a plain function, but uses a macro to create these. It also checks that the memory start and end are within the modules area to avoid any problems with it being used elsewhere. The date on those patches was the 17th June.
RFC 3/3 arm: add DEBUG_SET_MODULE_RONX option to Kconfig
In reply to Laura AbbottNow that all the page setting infrastructure is in place, Add the DEBUG_SET_MODULE_RONX to the ARM debugging Kconfig. When turned on, data sections for modules will be marked as NX and read only sections will be marked as such.
--- arch/arm/Kconfig.debug | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
arch/arm/Kconfig.debug +11 -0
@@ -692,4 +692,15 @@ config PID_IN_CONTEXTIDR additional instructions during context switch. Say Y here only if you are planning to use hardware trace tools with this kernel. +config DEBUG_SET_MODULE_RONX + bool "Set loadable kernel module data as NX and text as RO" + depends on MODULES + ---help--- + This option helps catch unintended modifications to loadable + kernel module's text and read-only data. It also prevents execution + of module data. Such protection may interfere with run-time code + patching and dynamic kernel tracing - and they might also protect + against certain classes of kernel exploits. + If in doubt, say "N". + endmenu
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
cover Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM
In reply to Laura AbbottQuoted 11 lines · Laura Abbott
Hi,
This is an RFC to allow
CONFIG_DEBUG_SET_MODULE_RONXto be used on ARM. The current config description from x86 describes it best:This option helps catch unintended modifications to loadable kernel module's text and read-only data. It also prevents execution of module data. Such protection may interfere with run-time code patching and dynamic kernel tracing - and they might also protect against certain classes of kernel exploits.
Quoted 3 lines
ARM was missing a few functions to modify the page tables so those have been added. I believe modules are always mapped with pages so changing them at map time should be acceptable. Comments/concerns are appreciated.
I've just tested this and it seems to work:
---[ Modules ]---
0xbf000000-0xbf002000 8K ro x MEM/CACHED/WBRA 0xbf002000-0xbf003000 4K ro NX MEM/CACHED/WBRA 0xbf003000-0xbf005000 8K RW NX MEM/CACHED/WBRA 0xbf009000-0xbf00b000 8K ro x MEM/CACHED/WBRA 0xbf00b000-0xbf00c000 4K ro NX MEM/CACHED/WBRA 0xbf00c000-0xbf00e000 8K RW NX MEM/CACHED/WBRA 0xbf012000-0xbf013000 4K ro x MEM/CACHED/WBRA 0xbf013000-0xbf014000 4K ro NX MEM/CACHED/WBRA 0xbf014000-0xbf016000 8K RW NX MEM/CACHED/WBRA 0xbf01a000-0xbf01c000 8K ro x MEM/CACHED/WBRA 0xbf01c000-0xbf01d000 4K ro NX MEM/CACHED/WBRA 0xbf01d000-0xbf01f000 8K RW NX MEM/CACHED/WBRA 0xbf024000-0xbf025000 4K ro x MEM/CACHED/WBRA 0xbf025000-0xbf026000 4K ro NX MEM/CACHED/WBRA 0xbf026000-0xbf028000 8K RW NX MEM/CACHED/WBRA 0xbf02c000-0xbf033000 28K ro x MEM/CACHED/WBRA 0xbf033000-0xbf035000 8K ro NX MEM/CACHED/WBRA 0xbf035000-0xbf03a000 20K RW NX MEM/CACHED/WBRA 0xbf041000-0xbf043000 8K ro x MEM/CACHED/WBRA 0xbf043000-0xbf045000 8K ro NX MEM/CACHED/WBRA 0xbf045000-0xbf048000 12K RW NX MEM/CACHED/WBRA 0xbf04e000-0xbf04f000 4K ro x MEM/CACHED/WBRA 0xbf04f000-0xbf050000 4K ro NX MEM/CACHED/WBRA 0xbf050000-0xbf052000 8K RW NX MEM/CACHED/WBRA 0xbf056000-0xbf05f000 36K ro x MEM/CACHED/WBRA 0xbf05f000-0xbf061000 8K ro NX MEM/CACHED/WBRA 0xbf061000-0xbf064000 12K RW NX MEM/CACHED/WBRA
cover Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM
Quoted 12 lines · Russell King - ARM Linux
Quoted 11 lines · Laura Abbott
Hi,
This is an RFC to allow
CONFIG_DEBUG_SET_MODULE_RONXto be used on ARM. The current config description from x86 describes it best:This option helps catch unintended modifications to loadable kernel module's text and read-only data. It also prevents execution of module data. Such protection may interfere with run-time code patching and dynamic kernel tracing - and they might also protect against certain classes of kernel exploits.
Quoted 5 lines
Quoted 3 lines
ARM was missing a few functions to modify the page tables so those have been added. I believe modules are always mapped with pages so changing them at map time should be acceptable. Comments/concerns are appreciated.
I've just tested this and it seems to work:
The only remaining question is whether DEBUG_SET_MODULE_RONX should be by default enabled. At the moment, the text says "if unsure, say N" but is that the right advice? Shouldn't we be encouraging people to have this option turned on unless there's a reason not to (eg, kprobes?)
How about adding:
default y if !(FTRACE || KPROBES || JUMP_LABEL)
as KPROBES and JUMP_LABEL both use the text patching, and FTRACE uses probe_kernel_write(). We may need to add kgdb to that later too. Or maybe a dependency on the above?
One thing which comes to mind while looking at this: should arch/arm/kernel/patch.c be using the probe_kernel_* functions in mm/maccess.c? Also, should we look at improving this code so we can have RONX modules and still have working ftrace/kprobes etc?
cover Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM
Quoted 28 lines · Russell King - ARM Linux
Quoted 12 lines · Russell King - ARM Linux
Quoted 11 lines · Laura Abbott
Hi,
This is an RFC to allow
CONFIG_DEBUG_SET_MODULE_RONXto be used on ARM. The current config description from x86 describes it best:This option helps catch unintended modifications to loadable kernel module's text and read-only data. It also prevents execution of module data. Such protection may interfere with run-time code patching and dynamic kernel tracing - and they might also protect against certain classes of kernel exploits.
Quoted 5 lines
Quoted 3 lines
ARM was missing a few functions to modify the page tables so those have been added. I believe modules are always mapped with pages so changing them at map time should be acceptable. Comments/concerns are appreciated.
I've just tested this and it seems to work:
The only remaining question is whether
DEBUG_SET_MODULE_RONXshould be by default enabled. At the moment, the text says "if unsure, say N" but is that the right advice? Shouldn't we be encouraging people to have this option turned on unless there's a reason not to (eg, kprobes?)How about adding:
default y if !(FTRACE || KPROBES || JUMP_LABEL)
Quoted 8 lines
as KPROBES and
JUMP_LABELboth use the text patching, and FTRACE usesprobe_kernel_write(). We may need to add kgdb to that later too. Or maybe a dependency on the above?One thing which comes to mind while looking at this: should
arch/arm/kernel/patch.cbe using the probe_kernel_* functions inmm/maccess.c? Also, should we look at improving this code so we can have RONX modules and still have working ftrace/kprobes etc?
Further to this...
probe_kernel_write() will return failure if it can't write without oopsing the kernel, so arguably this is "okay" in that it won't cause a kernel oops. x86 appears to do no different here, though see below.
The real problem case is ARMs patch_text() function which directly dereferences a virtual address, and so will oops against a kernel RO page.
x86 takes a different approach: they remap the page using a fixmap mapping so that they can bypass the RO permissions on text pages. I don't see any reason we can't do the same, though of course it makes it slightly more heavy-weight to do code patching. I think this is something we should do before introducing RO text of any kind on ARM, otherwise people will turn these options off when first presented with them and probably won't enable them later once such issues are fixed.
That argument can be applied to ftrace using probe_kernel_write() as well - I think we should avoid having to turn off RO(NX) support if we have a kernel feature which needs to write to text as much as possible.
A further idea is to have a separate config option which enables the presence of the code which bypasses the RO attribute: we don't want to build that into the kernel unless we have a reason for it to be there.
As far as merging this stuff, I know it's late in the cycle, but I think that the NX lowmem patches can go in during the next merge window as all they're doing is fixing the NX attribute setting on various mappings which should never have had them, and marking lowmem sections which do not need to be executable as such. I see this as very low risk as we should have nothing poking code directly to lowmem pages and trying to directly execute: the bpf stuff uses module_alloc() rather than grabbing a lowmem allocation, and we're not (yet) marking any lowmem as read-only. Also the kernel page table dumping code can go in too.
Expect to see those in linux-next very soon (unless someone objects?)