[RFC 0/3] Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM

[RFC 0/3] Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM

[Laura Abbott]

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 2/3] arm: mm: Define set_memory_* functions for ARM

[Russell King - ARM Linux]

On Wed, Jun 12, 2013 at 10:23:29AM -0700, Laura Abbott wrote:
> Other 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.

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

[Catalin Marinas]

On Wed, Jun 12, 2013 at 06:23:29PM +0100, Laura Abbott wrote:
> +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.

-- 
Catalin

[RFC 2/3] arm: mm: Define set_memory_* functions for ARM

[Will Deacon]

On Wed, Jun 12, 2013 at 06:23:29PM +0100, Laura Abbott wrote:
> Other 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.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm/include/asm/cacheflush.h |    5 ++
>  arch/arm/mm/mmu.c                 |   86 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> index bff7138..55ed26b 100644
> --- a/arch/arm/include/asm/cacheflush.h
> +++ b/arch/arm/include/asm/cacheflush.h
> @@ -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

[Laura Abbott]

On 6/18/2013 4:09 AM, Will Deacon wrote:
> On Wed, Jun 12, 2013 at 06:23:29PM +0100, Laura Abbott wrote:
>> Other 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.
>>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>> ---
>>   arch/arm/include/asm/cacheflush.h |    5 ++
>>   arch/arm/mm/mmu.c                 |   86 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 91 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> index bff7138..55ed26b 100644
>> --- a/arch/arm/include/asm/cacheflush.h
>> +++ b/arch/arm/include/asm/cacheflush.h
>> @@ -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?!).
>

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


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

[Will Deacon]

On Wed, Jun 19, 2013 at 02:48:28AM +0100, Laura Abbott wrote:
> On 6/18/2013 4:09 AM, Will Deacon wrote:
> > On Wed, Jun 12, 2013 at 06:23:29PM +0100, Laura Abbott wrote:
> >> Other 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.
> >>
> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> >> ---
> >>   arch/arm/include/asm/cacheflush.h |    5 ++
> >>   arch/arm/mm/mmu.c                 |   86 +++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 91 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> >> index bff7138..55ed26b 100644
> >> --- a/arch/arm/include/asm/cacheflush.h
> >> +++ b/arch/arm/include/asm/cacheflush.h
> >> @@ -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?!).
> >
> 
> 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?

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

[Will Deacon]

Hi Laura,

On Wed, Jun 12, 2013 at 06:23:29PM +0100, Laura Abbott wrote:
> Other 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.

[...]

> +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

[Russell King - ARM Linux]

On Fri, Oct 25, 2013 at 02:08:40PM +0100, Will Deacon wrote:
> Hi Laura,
> 
> On Wed, Jun 12, 2013 at 06:23:29PM +0100, Laura Abbott wrote:
> > Other 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.
> 
> [...]
> 
> > +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).

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 0/3] Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM

[Russell King - ARM Linux]

On Wed, Jun 12, 2013 at 10:23:27AM -0700, Laura Abbott wrote:
> 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.

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

[RFC 0/3] Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM

[Russell King - ARM Linux]

On Thu, Oct 24, 2013 at 02:03:46PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 12, 2013 at 10:23:27AM -0700, Laura Abbott wrote:
> > 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.
> 
> 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?

[RFC 0/3] Allow CONFIG_DEBUG_SET_MODULE_RONX to be used on ARM

[Russell King - ARM Linux]

On Sun, Oct 27, 2013 at 10:34:52AM +0000, Russell King - ARM Linux wrote:
> On Thu, Oct 24, 2013 at 02:03:46PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jun 12, 2013 at 10:23:27AM -0700, Laura Abbott wrote:
> > > 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.
> > 
> > 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?

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