diff mbox

[PATCHv2,1/2] arch: Move CONFIG_DEBUG_RODATA and CONFIG_SET_MODULE_RONX to be common

Message ID 1486144343-24998-2-git-send-email-labbott@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laura Abbott Feb. 3, 2017, 5:52 p.m. UTC
There are multiple architectures that support CONFIG_DEBUG_RODATA and
CONFIG_SET_MODULE_RONX. These options also now have the ability to be
turned off at runtime. Move these to an architecture independent
location and make these options def_bool y for almost all of those
arches.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: This patch is now doing just the refactor of the existing config options.
---
 arch/Kconfig              | 28 ++++++++++++++++++++++++++++
 arch/arm/Kconfig          |  3 +++
 arch/arm/Kconfig.debug    | 11 -----------
 arch/arm/mm/Kconfig       | 12 ------------
 arch/arm64/Kconfig        |  5 ++---
 arch/arm64/Kconfig.debug  | 11 -----------
 arch/parisc/Kconfig       |  1 +
 arch/parisc/Kconfig.debug | 11 -----------
 arch/s390/Kconfig         |  5 ++---
 arch/s390/Kconfig.debug   |  3 ---
 arch/x86/Kconfig          |  5 ++---
 arch/x86/Kconfig.debug    | 11 -----------
 12 files changed, 38 insertions(+), 68 deletions(-)

Comments

Mark Rutland Feb. 3, 2017, 6:16 p.m. UTC | #1
On Fri, Feb 03, 2017 at 09:52:21AM -0800, Laura Abbott wrote:
> There are multiple architectures that support CONFIG_DEBUG_RODATA and
> CONFIG_SET_MODULE_RONX. These options also now have the ability to be
> turned off at runtime. Move these to an architecture independent
> location and make these options def_bool y for almost all of those
> arches.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>

From my POV this looks good. FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
> v2: This patch is now doing just the refactor of the existing config options.
> ---
>  arch/Kconfig              | 28 ++++++++++++++++++++++++++++
>  arch/arm/Kconfig          |  3 +++
>  arch/arm/Kconfig.debug    | 11 -----------
>  arch/arm/mm/Kconfig       | 12 ------------
>  arch/arm64/Kconfig        |  5 ++---
>  arch/arm64/Kconfig.debug  | 11 -----------
>  arch/parisc/Kconfig       |  1 +
>  arch/parisc/Kconfig.debug | 11 -----------
>  arch/s390/Kconfig         |  5 ++---
>  arch/s390/Kconfig.debug   |  3 ---
>  arch/x86/Kconfig          |  5 ++---
>  arch/x86/Kconfig.debug    | 11 -----------
>  12 files changed, 38 insertions(+), 68 deletions(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 99839c2..22ee01e 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -781,4 +781,32 @@ config VMAP_STACK
>  	  the stack to map directly to the KASAN shadow map using a formula
>  	  that is incorrect if the stack is in vmalloc space.
>  
> +config ARCH_NO_STRICT_RWX_DEFAULTS
> +	def_bool n
> +
> +config ARCH_HAS_STRICT_KERNEL_RWX
> +	def_bool n
> +
> +config DEBUG_RODATA
> +	def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
> +	prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
> +	depends on ARCH_HAS_STRICT_KERNEL_RWX
> +	help
> +	  If this is set, kernel text and rodata memory will be made read-only,
> +	  and non-text memory will be made non-executable. This provides
> +	  protection against certain security exploits (e.g. executing the heap
> +	  or modifying text)
> +
> +config ARCH_HAS_STRICT_MODULE_RWX
> +	def_bool n
> +
> +config DEBUG_SET_MODULE_RONX
> +	def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
> +	prompt "Set loadable kenrel module data as NX and text as RO" if ARCH_NO_STRICT_RWX_DEFAULTS
> +	depends on ARCH_HAS_STRICT_MODULE_RWX && MODULES
> +	help
> +	  If this is set, module text and rodata memory will be made read-only,
> +	  and non-text memory will be made non-executable. This provides
> +	  protection against certain security exploits (e.g. writing to text)
> +
>  source "kernel/gcov/Kconfig"
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 186c4c2..aa73ca8 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -4,10 +4,13 @@ config ARM
>  	select ARCH_CLOCKSOURCE_DATA
>  	select ARCH_HAS_DEVMEM_IS_ALLOWED
>  	select ARCH_HAS_ELF_RANDOMIZE
> +	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> +	select ARCH_HAS_STRICT_MODULE_RWX if MMU
>  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>  	select ARCH_HAVE_CUSTOM_GPIO_H
>  	select ARCH_HAS_GCOV_PROFILE_ALL
>  	select ARCH_MIGHT_HAVE_PC_PARPORT
> +	select ARCH_NO_STRICT_RWX_DEFAULTS if !CPU_V7
>  	select ARCH_SUPPORTS_ATOMIC_RMW
>  	select ARCH_USE_BUILTIN_BSWAP
>  	select ARCH_USE_CMPXCHG_LOCKREF
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index d83f7c3..426d271 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1738,17 +1738,6 @@ 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 && MMU
> -	---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".
> -
>  source "drivers/hwtracing/coresight/Kconfig"
>  
>  endmenu
> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index f68e8ec..419a035 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -1051,18 +1051,6 @@ config ARCH_SUPPORTS_BIG_ENDIAN
>  	  This option specifies the architecture can support big endian
>  	  operation.
>  
> -config DEBUG_RODATA
> -	bool "Make kernel text and rodata read-only"
> -	depends on MMU && !XIP_KERNEL
> -	default y if CPU_V7
> -	help
> -	  If this is set, kernel text and rodata memory will be made
> -	  read-only, and non-text kernel memory will be made non-executable.
> -	  The tradeoff is that each region is padded to section-size (1MiB)
> -	  boundaries (because their permissions are different and splitting
> -	  the 1M pages into 4K ones causes TLB performance problems), which
> -	  can waste memory.
> -
>  config DEBUG_ALIGN_RODATA
>  	bool "Make rodata strictly non-executable"
>  	depends on DEBUG_RODATA
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1117421..e1efbcc 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -13,6 +13,8 @@ config ARM64
>  	select ARCH_HAS_GIGANTIC_PAGE
>  	select ARCH_HAS_KCOV
>  	select ARCH_HAS_SG_CHAIN
> +	select ARCH_HAS_STRICT_KERNEL_RWX
> +	select ARCH_HAS_STRICT_MODULE_RWX
>  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>  	select ARCH_USE_CMPXCHG_LOCKREF
>  	select ARCH_SUPPORTS_ATOMIC_RMW
> @@ -123,9 +125,6 @@ config ARCH_PHYS_ADDR_T_64BIT
>  config MMU
>  	def_bool y
>  
> -config DEBUG_RODATA
> -	def_bool y
> -
>  config ARM64_PAGE_SHIFT
>  	int
>  	default 16 if ARM64_64K_PAGES
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index d1ebd46..939815e 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -71,17 +71,6 @@ config DEBUG_WX
>  
>  	  If in doubt, say "Y".
>  
> -config DEBUG_SET_MODULE_RONX
> -	bool "Set loadable kernel module data as NX and text as RO"
> -	depends on MODULES
> -	default y
> -	help
> -	  Is this is set, kernel module text and rodata will be made read-only.
> -	  This is to help catch accidental or malicious attempts to change the
> -	  kernel's executable code.
> -
> -	  If in doubt, say Y.
> -
>  config DEBUG_ALIGN_RODATA
>  	depends on DEBUG_RODATA
>  	bool "Align linker sections up to SECTION_SIZE"
> diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
> index 3a71f38..ad294b3 100644
> --- a/arch/parisc/Kconfig
> +++ b/arch/parisc/Kconfig
> @@ -8,6 +8,7 @@ config PARISC
>  	select HAVE_SYSCALL_TRACEPOINTS
>  	select ARCH_WANT_FRAME_POINTERS
>  	select ARCH_HAS_ELF_RANDOMIZE
> +	select ARCH_HAS_STRICT_KERNEL_RWX
>  	select RTC_CLASS
>  	select RTC_DRV_GENERIC
>  	select INIT_ALL_POSSIBLE
> diff --git a/arch/parisc/Kconfig.debug b/arch/parisc/Kconfig.debug
> index 68b7cbd..0d856b9 100644
> --- a/arch/parisc/Kconfig.debug
> +++ b/arch/parisc/Kconfig.debug
> @@ -5,15 +5,4 @@ source "lib/Kconfig.debug"
>  config TRACE_IRQFLAGS_SUPPORT
>  	def_bool y
>  
> -config DEBUG_RODATA
> -       bool "Write protect kernel read-only data structures"
> -       depends on DEBUG_KERNEL
> -       default y
> -       help
> -         Mark the kernel read-only data as write-protected in the pagetables,
> -         in order to catch accidental (and incorrect) writes to such const
> -         data. This option may have a slight performance impact because a
> -         portion of the kernel code won't be covered by a TLB anymore.
> -         If in doubt, say "N".
> -
>  endmenu
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index c6722112..53bb0e3 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -62,9 +62,6 @@ config PCI_QUIRKS
>  config ARCH_SUPPORTS_UPROBES
>  	def_bool y
>  
> -config DEBUG_RODATA
> -	def_bool y
> -
>  config S390
>  	def_bool y
>  	select ARCH_HAS_DEVMEM_IS_ALLOWED
> @@ -73,6 +70,8 @@ config S390
>  	select ARCH_HAS_GIGANTIC_PAGE
>  	select ARCH_HAS_KCOV
>  	select ARCH_HAS_SG_CHAIN
> +	select ARCH_HAS_STRICT_KERNEL_RWX
> +	select ARCH_HAS_STRICT_MODULE_RWX
>  	select ARCH_HAS_UBSAN_SANITIZE_ALL
>  	select ARCH_HAVE_NMI_SAFE_CMPXCHG
>  	select ARCH_INLINE_READ_LOCK
> diff --git a/arch/s390/Kconfig.debug b/arch/s390/Kconfig.debug
> index 26c5d5be..57f8ea9 100644
> --- a/arch/s390/Kconfig.debug
> +++ b/arch/s390/Kconfig.debug
> @@ -17,7 +17,4 @@ config S390_PTDUMP
>  	  kernel.
>  	  If in doubt, say "N"
>  
> -config DEBUG_SET_MODULE_RONX
> -	def_bool y
> -	depends on MODULES
>  endmenu
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index e487493..13e1bf4 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -54,6 +54,8 @@ config X86
>  	select ARCH_HAS_MMIO_FLUSH
>  	select ARCH_HAS_PMEM_API		if X86_64
>  	select ARCH_HAS_SG_CHAIN
> +	select ARCH_HAS_STRICT_KERNEL_RWX
> +	select ARCH_HAS_STRICT_MODULE_RWX
>  	select ARCH_HAS_UBSAN_SANITIZE_ALL
>  	select ARCH_HAVE_NMI_SAFE_CMPXCHG
>  	select ARCH_MIGHT_HAVE_ACPI_PDC		if ACPI
> @@ -309,9 +311,6 @@ config ARCH_SUPPORTS_UPROBES
>  config FIX_EARLYCON_MEM
>  	def_bool y
>  
> -config DEBUG_RODATA
> -	def_bool y
> -
>  config PGTABLE_LEVELS
>  	int
>  	default 4 if X86_64
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 67eec55..69cdd0b 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -109,17 +109,6 @@ config DEBUG_WX
>  
>  	  If in doubt, say "Y".
>  
> -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".
> -
>  config DEBUG_NX_TEST
>  	tristate "Testcase for the NX non-executable stack feature"
>  	depends on DEBUG_KERNEL && m
> -- 
> 2.7.4
>
Kees Cook Feb. 3, 2017, 7:45 p.m. UTC | #2
On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott <labbott@redhat.com> wrote:
> There are multiple architectures that support CONFIG_DEBUG_RODATA and
> CONFIG_SET_MODULE_RONX. These options also now have the ability to be
> turned off at runtime. Move these to an architecture independent
> location and make these options def_bool y for almost all of those
> arches.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v2: This patch is now doing just the refactor of the existing config options.
> ---
>  arch/Kconfig              | 28 ++++++++++++++++++++++++++++
>  arch/arm/Kconfig          |  3 +++
>  arch/arm/Kconfig.debug    | 11 -----------
>  arch/arm/mm/Kconfig       | 12 ------------
>  arch/arm64/Kconfig        |  5 ++---
>  arch/arm64/Kconfig.debug  | 11 -----------
>  arch/parisc/Kconfig       |  1 +
>  arch/parisc/Kconfig.debug | 11 -----------
>  arch/s390/Kconfig         |  5 ++---
>  arch/s390/Kconfig.debug   |  3 ---
>  arch/x86/Kconfig          |  5 ++---
>  arch/x86/Kconfig.debug    | 11 -----------
>  12 files changed, 38 insertions(+), 68 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 99839c2..22ee01e 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -781,4 +781,32 @@ config VMAP_STACK
>           the stack to map directly to the KASAN shadow map using a formula
>           that is incorrect if the stack is in vmalloc space.
>
> +config ARCH_NO_STRICT_RWX_DEFAULTS
> +       def_bool n
> +
> +config ARCH_HAS_STRICT_KERNEL_RWX
> +       def_bool n
> +
> +config DEBUG_RODATA
> +       def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
> +       prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS

Ah! Yes, perfect. I totally forgot about using conditional "prompt" lines. Nice!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees
Russell King (Oracle) Feb. 3, 2017, 8:29 p.m. UTC | #3
On Fri, Feb 03, 2017 at 11:45:56AM -0800, Kees Cook wrote:
> On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott <labbott@redhat.com> wrote:
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index 99839c2..22ee01e 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -781,4 +781,32 @@ config VMAP_STACK
> >           the stack to map directly to the KASAN shadow map using a formula
> >           that is incorrect if the stack is in vmalloc space.
> >
> > +config ARCH_NO_STRICT_RWX_DEFAULTS
> > +       def_bool n
> > +
> > +config ARCH_HAS_STRICT_KERNEL_RWX
> > +       def_bool n
> > +
> > +config DEBUG_RODATA
> > +       def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
> > +       prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
> 
> Ah! Yes, perfect. I totally forgot about using conditional "prompt"
> lines. Nice!

It's no different from the more usual:

	bool "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
	default y if !ARCH_NO_STRICT_RWX_DEFAULTS
	depends on ARCH_HAS_STRICT_KERNEL_RWX

But... I really don't like this - way too many negations and negatives
which make it difficult to figure out what's going on here.

The situation we have today is:

-config DEBUG_RODATA
-       bool "Make kernel text and rodata read-only"
-       depends on MMU && !XIP_KERNEL
-       default y if CPU_V7

which is "allow the user to select DEBUG_RODATA if building a MMU non-XIP
kernel", suggesting that the user turns it on for ARMv7 CPUs.

That changes with this and the above:

+       select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
+       select ARCH_HAS_STRICT_MODULE_RWX if MMU
+       select ARCH_NO_STRICT_RWX_DEFAULTS if !CPU_V7

This means that ARCH_HAS_STRICT_KERNEL_RWX is set for a MMU non-XIP
kernel, which carries the same pre-condition for DEBUG_RODATA - no
problem there.

However, ARCH_NO_STRICT_RWX_DEFAULTS is set for non-ARMv7 CPUs, which
means the "Make kernel text and rodata read-only" prompt _is_ provided
for those.  However, for all ARMv7 systems, we go from "suggesting that
the user enables the option" to "you don't have a choice, you get this
whether you want it or not."

I'd prefer to keep it off for my development systems, where I don't
care about kernel security.  If we don't wish to do that as a general
rule, can we make it dependent on EMBEDDED?

Given that on ARM it can add up to 4MB to the kernel image - there
_will_ be about 1MB before the .text section, the padding on between
__modver and __ex_table which for me is around 626k, the padding
between .notes and the init sections start with .vectors (the space
between __ex_table and end of .notes is only 4124, which gets padded
up to 1MB) and lastly the padding between the .init section and the
data section (for me around 593k).  This all adds up to an increase
in kernel image size of 3.2MB on 14.2MB - an increase of 22%.

So no, I'm really not happy with that.
Kees Cook Feb. 3, 2017, 9:08 p.m. UTC | #4
On Fri, Feb 3, 2017 at 12:29 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Feb 03, 2017 at 11:45:56AM -0800, Kees Cook wrote:
>> On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott <labbott@redhat.com> wrote:
>> > diff --git a/arch/Kconfig b/arch/Kconfig
>> > index 99839c2..22ee01e 100644
>> > --- a/arch/Kconfig
>> > +++ b/arch/Kconfig
>> > @@ -781,4 +781,32 @@ config VMAP_STACK
>> >           the stack to map directly to the KASAN shadow map using a formula
>> >           that is incorrect if the stack is in vmalloc space.
>> >
>> > +config ARCH_NO_STRICT_RWX_DEFAULTS
>> > +       def_bool n
>> > +
>> > +config ARCH_HAS_STRICT_KERNEL_RWX
>> > +       def_bool n
>> > +
>> > +config DEBUG_RODATA
>> > +       def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
>> > +       prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
>>
>> Ah! Yes, perfect. I totally forgot about using conditional "prompt"
>> lines. Nice!
>
> It's no different from the more usual:
>
>         bool "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
>         default y if !ARCH_NO_STRICT_RWX_DEFAULTS
>         depends on ARCH_HAS_STRICT_KERNEL_RWX
>
> But... I really don't like this - way too many negations and negatives
> which make it difficult to figure out what's going on here.
>
> The situation we have today is:
>
> -config DEBUG_RODATA
> -       bool "Make kernel text and rodata read-only"
> -       depends on MMU && !XIP_KERNEL
> -       default y if CPU_V7
>
> which is "allow the user to select DEBUG_RODATA if building a MMU non-XIP
> kernel", suggesting that the user turns it on for ARMv7 CPUs.
>
> That changes with this and the above:
>
> +       select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> +       select ARCH_HAS_STRICT_MODULE_RWX if MMU
> +       select ARCH_NO_STRICT_RWX_DEFAULTS if !CPU_V7
>
> This means that ARCH_HAS_STRICT_KERNEL_RWX is set for a MMU non-XIP
> kernel, which carries the same pre-condition for DEBUG_RODATA - no
> problem there.
>
> However, ARCH_NO_STRICT_RWX_DEFAULTS is set for non-ARMv7 CPUs, which
> means the "Make kernel text and rodata read-only" prompt _is_ provided
> for those.  However, for all ARMv7 systems, we go from "suggesting that
> the user enables the option" to "you don't have a choice, you get this
> whether you want it or not."
>
> I'd prefer to keep it off for my development systems, where I don't
> care about kernel security.  If we don't wish to do that as a general
> rule, can we make it dependent on EMBEDDED?
>
> Given that on ARM it can add up to 4MB to the kernel image - there
> _will_ be about 1MB before the .text section, the padding on between
> __modver and __ex_table which for me is around 626k, the padding
> between .notes and the init sections start with .vectors (the space
> between __ex_table and end of .notes is only 4124, which gets padded
> up to 1MB) and lastly the padding between the .init section and the
> data section (for me around 593k).  This all adds up to an increase
> in kernel image size of 3.2MB on 14.2MB - an increase of 22%.
>
> So no, I'm really not happy with that.

Ah yeah, good point. We have three cases: unsupported, mandatory,
optional, but we have the case of setting the default for the optional
case. Maybe something like this?

config STRICT_KERNEL_RWX
  bool "Make kernel text and rodata read-only" if ARCH_OPTIONAL_KERNEL_RWX
  depends on ARCH_HAS_STRICT_KERNEL_RWX
  default ARCH_OPTIONAL_KERNEL_RWX_DEFAULT

unsupported:
!ARCH_HAS_STRICT_KERNEL_RWX

mandatory:
ARCH_HAS_STRICT_KERNEL_RWX
!ARCH_OPTIONAL_KERNEL_RWX

optional:
ARCH_HAS_STRICT_KERNEL_RWX
ARCH_OPTIONAL_KERNEL_RWX
with default controlled by ARCH_OPTIONAL_KERNEL_RWX_DEFAULT

Then arm is:
  select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
  select ARCH_HAS_STRICT_MODULE_RWX if MMU
  select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
  select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7

x86 and arm64 are:
  select ARCH_HAS_STRICT_KERNEL_RWX
  select ARCH_HAS_STRICT_MODULE_RWX

?

-Kees
Russell King (Oracle) Feb. 3, 2017, 10:28 p.m. UTC | #5
On Fri, Feb 03, 2017 at 01:08:40PM -0800, Kees Cook wrote:
> On Fri, Feb 3, 2017 at 12:29 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Fri, Feb 03, 2017 at 11:45:56AM -0800, Kees Cook wrote:
> >> On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott <labbott@redhat.com> wrote:
> >> > diff --git a/arch/Kconfig b/arch/Kconfig
> >> > index 99839c2..22ee01e 100644
> >> > --- a/arch/Kconfig
> >> > +++ b/arch/Kconfig
> >> > @@ -781,4 +781,32 @@ config VMAP_STACK
> >> >           the stack to map directly to the KASAN shadow map using a formula
> >> >           that is incorrect if the stack is in vmalloc space.
> >> >
> >> > +config ARCH_NO_STRICT_RWX_DEFAULTS
> >> > +       def_bool n
> >> > +
> >> > +config ARCH_HAS_STRICT_KERNEL_RWX
> >> > +       def_bool n
> >> > +
> >> > +config DEBUG_RODATA
> >> > +       def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
> >> > +       prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
> >>
> >> Ah! Yes, perfect. I totally forgot about using conditional "prompt"
> >> lines. Nice!
> >
> > It's no different from the more usual:
> >
> >         bool "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
> >         default y if !ARCH_NO_STRICT_RWX_DEFAULTS
> >         depends on ARCH_HAS_STRICT_KERNEL_RWX
> >
> > But... I really don't like this - way too many negations and negatives
> > which make it difficult to figure out what's going on here.
> >
> > The situation we have today is:
> >
> > -config DEBUG_RODATA
> > -       bool "Make kernel text and rodata read-only"
> > -       depends on MMU && !XIP_KERNEL
> > -       default y if CPU_V7
> >
> > which is "allow the user to select DEBUG_RODATA if building a MMU non-XIP
> > kernel", suggesting that the user turns it on for ARMv7 CPUs.
> >
> > That changes with this and the above:
> >
> > +       select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> > +       select ARCH_HAS_STRICT_MODULE_RWX if MMU
> > +       select ARCH_NO_STRICT_RWX_DEFAULTS if !CPU_V7
> >
> > This means that ARCH_HAS_STRICT_KERNEL_RWX is set for a MMU non-XIP
> > kernel, which carries the same pre-condition for DEBUG_RODATA - no
> > problem there.
> >
> > However, ARCH_NO_STRICT_RWX_DEFAULTS is set for non-ARMv7 CPUs, which
> > means the "Make kernel text and rodata read-only" prompt _is_ provided
> > for those.  However, for all ARMv7 systems, we go from "suggesting that
> > the user enables the option" to "you don't have a choice, you get this
> > whether you want it or not."
> >
> > I'd prefer to keep it off for my development systems, where I don't
> > care about kernel security.  If we don't wish to do that as a general
> > rule, can we make it dependent on EMBEDDED?
> >
> > Given that on ARM it can add up to 4MB to the kernel image - there
> > _will_ be about 1MB before the .text section, the padding on between
> > __modver and __ex_table which for me is around 626k, the padding
> > between .notes and the init sections start with .vectors (the space
> > between __ex_table and end of .notes is only 4124, which gets padded
> > up to 1MB) and lastly the padding between the .init section and the
> > data section (for me around 593k).  This all adds up to an increase
> > in kernel image size of 3.2MB on 14.2MB - an increase of 22%.
> >
> > So no, I'm really not happy with that.
> 
> Ah yeah, good point. We have three cases: unsupported, mandatory,
> optional, but we have the case of setting the default for the optional
> case. Maybe something like this?
> 
> config STRICT_KERNEL_RWX
>   bool "Make kernel text and rodata read-only" if ARCH_OPTIONAL_KERNEL_RWX
>   depends on ARCH_HAS_STRICT_KERNEL_RWX
>   default ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
> 
> unsupported:
> !ARCH_HAS_STRICT_KERNEL_RWX
> 
> mandatory:
> ARCH_HAS_STRICT_KERNEL_RWX
> !ARCH_OPTIONAL_KERNEL_RWX
> 
> optional:
> ARCH_HAS_STRICT_KERNEL_RWX
> ARCH_OPTIONAL_KERNEL_RWX
> with default controlled by ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
> 
> Then arm is:
>   select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
>   select ARCH_HAS_STRICT_MODULE_RWX if MMU
>   select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
>   select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7
> 
> x86 and arm64 are:
>   select ARCH_HAS_STRICT_KERNEL_RWX
>   select ARCH_HAS_STRICT_MODULE_RWX

Looks to me like it will do the job.

In passing, I noticed that, on ARM:

  3 .rodata       002212b4  c0703000  c0703000  00703000  2**6
                  CONTENTS, ALLOC, LOAD, DATA

a lack of READONLY there - which suggests something in this lot isn't
actually read-only data:

        .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {           \
                VMLINUX_SYMBOL(__start_rodata) = .;                     \
                *(.rodata) *(.rodata.*)                                 \
                RO_AFTER_INIT_DATA      /* Read only after init */      \
                KEEP(*(__vermagic))     /* Kernel version magic */      \
                . = ALIGN(8);                                           \
                VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;         \
                KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \
                VMLINUX_SYMBOL(__stop___tracepoints_ptrs) = .;          \
                *(__tracepoints_strings)/* Tracepoints: strings */      \
        }                                                               \

I suspect it's the RO_AFTER_INIT_DATA section - maybe this should be
placed into its own separate output section?  I don't think it's doing
any harm, it's just odd that the .rodata section is marked as a normal
data section.
Kees Cook Feb. 3, 2017, 11:07 p.m. UTC | #6
On Fri, Feb 3, 2017 at 2:28 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Feb 03, 2017 at 01:08:40PM -0800, Kees Cook wrote:
>> On Fri, Feb 3, 2017 at 12:29 PM, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > On Fri, Feb 03, 2017 at 11:45:56AM -0800, Kees Cook wrote:
>> >> On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott <labbott@redhat.com> wrote:
>> >> > diff --git a/arch/Kconfig b/arch/Kconfig
>> >> > index 99839c2..22ee01e 100644
>> >> > --- a/arch/Kconfig
>> >> > +++ b/arch/Kconfig
>> >> > @@ -781,4 +781,32 @@ config VMAP_STACK
>> >> >           the stack to map directly to the KASAN shadow map using a formula
>> >> >           that is incorrect if the stack is in vmalloc space.
>> >> >
>> >> > +config ARCH_NO_STRICT_RWX_DEFAULTS
>> >> > +       def_bool n
>> >> > +
>> >> > +config ARCH_HAS_STRICT_KERNEL_RWX
>> >> > +       def_bool n
>> >> > +
>> >> > +config DEBUG_RODATA
>> >> > +       def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
>> >> > +       prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
>> >>
>> >> Ah! Yes, perfect. I totally forgot about using conditional "prompt"
>> >> lines. Nice!
>> >
>> > It's no different from the more usual:
>> >
>> >         bool "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
>> >         default y if !ARCH_NO_STRICT_RWX_DEFAULTS
>> >         depends on ARCH_HAS_STRICT_KERNEL_RWX
>> >
>> > But... I really don't like this - way too many negations and negatives
>> > which make it difficult to figure out what's going on here.
>> >
>> > The situation we have today is:
>> >
>> > -config DEBUG_RODATA
>> > -       bool "Make kernel text and rodata read-only"
>> > -       depends on MMU && !XIP_KERNEL
>> > -       default y if CPU_V7
>> >
>> > which is "allow the user to select DEBUG_RODATA if building a MMU non-XIP
>> > kernel", suggesting that the user turns it on for ARMv7 CPUs.
>> >
>> > That changes with this and the above:
>> >
>> > +       select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
>> > +       select ARCH_HAS_STRICT_MODULE_RWX if MMU
>> > +       select ARCH_NO_STRICT_RWX_DEFAULTS if !CPU_V7
>> >
>> > This means that ARCH_HAS_STRICT_KERNEL_RWX is set for a MMU non-XIP
>> > kernel, which carries the same pre-condition for DEBUG_RODATA - no
>> > problem there.
>> >
>> > However, ARCH_NO_STRICT_RWX_DEFAULTS is set for non-ARMv7 CPUs, which
>> > means the "Make kernel text and rodata read-only" prompt _is_ provided
>> > for those.  However, for all ARMv7 systems, we go from "suggesting that
>> > the user enables the option" to "you don't have a choice, you get this
>> > whether you want it or not."
>> >
>> > I'd prefer to keep it off for my development systems, where I don't
>> > care about kernel security.  If we don't wish to do that as a general
>> > rule, can we make it dependent on EMBEDDED?
>> >
>> > Given that on ARM it can add up to 4MB to the kernel image - there
>> > _will_ be about 1MB before the .text section, the padding on between
>> > __modver and __ex_table which for me is around 626k, the padding
>> > between .notes and the init sections start with .vectors (the space
>> > between __ex_table and end of .notes is only 4124, which gets padded
>> > up to 1MB) and lastly the padding between the .init section and the
>> > data section (for me around 593k).  This all adds up to an increase
>> > in kernel image size of 3.2MB on 14.2MB - an increase of 22%.
>> >
>> > So no, I'm really not happy with that.
>>
>> Ah yeah, good point. We have three cases: unsupported, mandatory,
>> optional, but we have the case of setting the default for the optional
>> case. Maybe something like this?
>>
>> config STRICT_KERNEL_RWX
>>   bool "Make kernel text and rodata read-only" if ARCH_OPTIONAL_KERNEL_RWX
>>   depends on ARCH_HAS_STRICT_KERNEL_RWX
>>   default ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
>>
>> unsupported:
>> !ARCH_HAS_STRICT_KERNEL_RWX
>>
>> mandatory:
>> ARCH_HAS_STRICT_KERNEL_RWX
>> !ARCH_OPTIONAL_KERNEL_RWX
>>
>> optional:
>> ARCH_HAS_STRICT_KERNEL_RWX
>> ARCH_OPTIONAL_KERNEL_RWX
>> with default controlled by ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
>>
>> Then arm is:
>>   select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
>>   select ARCH_HAS_STRICT_MODULE_RWX if MMU
>>   select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
>>   select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7
>>
>> x86 and arm64 are:
>>   select ARCH_HAS_STRICT_KERNEL_RWX
>>   select ARCH_HAS_STRICT_MODULE_RWX
>
> Looks to me like it will do the job.
>
> In passing, I noticed that, on ARM:
>
>   3 .rodata       002212b4  c0703000  c0703000  00703000  2**6
>                   CONTENTS, ALLOC, LOAD, DATA
>
> a lack of READONLY there - which suggests something in this lot isn't
> actually read-only data:
>
>         .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {           \
>                 VMLINUX_SYMBOL(__start_rodata) = .;                     \
>                 *(.rodata) *(.rodata.*)                                 \
>                 RO_AFTER_INIT_DATA      /* Read only after init */      \
>                 KEEP(*(__vermagic))     /* Kernel version magic */      \
>                 . = ALIGN(8);                                           \
>                 VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;         \
>                 KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \
>                 VMLINUX_SYMBOL(__stop___tracepoints_ptrs) = .;          \
>                 *(__tracepoints_strings)/* Tracepoints: strings */      \
>         }                                                               \
>
> I suspect it's the RO_AFTER_INIT_DATA section - maybe this should be
> placed into its own separate output section?  I don't think it's doing
> any harm, it's just odd that the .rodata section is marked as a normal
> data section.

Yeah, I think the linker drops the RO flag when it see the
RO_AFTER_INIT_DATA flags. Since the kernel does the permissions
settings, this has been fine. If we can force the linker flags on it,
I'm all for it, but every time I've tried, it has defeated me. :)

-Kees
Laura Abbott Feb. 6, 2017, 6:47 p.m. UTC | #7
On 02/03/2017 01:08 PM, Kees Cook wrote:
> On Fri, Feb 3, 2017 at 12:29 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Fri, Feb 03, 2017 at 11:45:56AM -0800, Kees Cook wrote:
>>> On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott <labbott@redhat.com> wrote:
>>>> diff --git a/arch/Kconfig b/arch/Kconfig
>>>> index 99839c2..22ee01e 100644
>>>> --- a/arch/Kconfig
>>>> +++ b/arch/Kconfig
>>>> @@ -781,4 +781,32 @@ config VMAP_STACK
>>>>           the stack to map directly to the KASAN shadow map using a formula
>>>>           that is incorrect if the stack is in vmalloc space.
>>>>
>>>> +config ARCH_NO_STRICT_RWX_DEFAULTS
>>>> +       def_bool n
>>>> +
>>>> +config ARCH_HAS_STRICT_KERNEL_RWX
>>>> +       def_bool n
>>>> +
>>>> +config DEBUG_RODATA
>>>> +       def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
>>>> +       prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
>>>
>>> Ah! Yes, perfect. I totally forgot about using conditional "prompt"
>>> lines. Nice!
>>
>> It's no different from the more usual:
>>
>>         bool "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
>>         default y if !ARCH_NO_STRICT_RWX_DEFAULTS
>>         depends on ARCH_HAS_STRICT_KERNEL_RWX
>>
>> But... I really don't like this - way too many negations and negatives
>> which make it difficult to figure out what's going on here.
>>
>> The situation we have today is:
>>
>> -config DEBUG_RODATA
>> -       bool "Make kernel text and rodata read-only"
>> -       depends on MMU && !XIP_KERNEL
>> -       default y if CPU_V7
>>
>> which is "allow the user to select DEBUG_RODATA if building a MMU non-XIP
>> kernel", suggesting that the user turns it on for ARMv7 CPUs.
>>
>> That changes with this and the above:
>>
>> +       select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
>> +       select ARCH_HAS_STRICT_MODULE_RWX if MMU
>> +       select ARCH_NO_STRICT_RWX_DEFAULTS if !CPU_V7
>>
>> This means that ARCH_HAS_STRICT_KERNEL_RWX is set for a MMU non-XIP
>> kernel, which carries the same pre-condition for DEBUG_RODATA - no
>> problem there.
>>
>> However, ARCH_NO_STRICT_RWX_DEFAULTS is set for non-ARMv7 CPUs, which
>> means the "Make kernel text and rodata read-only" prompt _is_ provided
>> for those.  However, for all ARMv7 systems, we go from "suggesting that
>> the user enables the option" to "you don't have a choice, you get this
>> whether you want it or not."
>>
>> I'd prefer to keep it off for my development systems, where I don't
>> care about kernel security.  If we don't wish to do that as a general
>> rule, can we make it dependent on EMBEDDED?
>>
>> Given that on ARM it can add up to 4MB to the kernel image - there
>> _will_ be about 1MB before the .text section, the padding on between
>> __modver and __ex_table which for me is around 626k, the padding
>> between .notes and the init sections start with .vectors (the space
>> between __ex_table and end of .notes is only 4124, which gets padded
>> up to 1MB) and lastly the padding between the .init section and the
>> data section (for me around 593k).  This all adds up to an increase
>> in kernel image size of 3.2MB on 14.2MB - an increase of 22%.
>>
>> So no, I'm really not happy with that.
> 
> Ah yeah, good point. We have three cases: unsupported, mandatory,
> optional, but we have the case of setting the default for the optional
> case. Maybe something like this?
> 
> config STRICT_KERNEL_RWX
>   bool "Make kernel text and rodata read-only" if ARCH_OPTIONAL_KERNEL_RWX
>   depends on ARCH_HAS_STRICT_KERNEL_RWX
>   default ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
> 
> unsupported:
> !ARCH_HAS_STRICT_KERNEL_RWX
> 
> mandatory:
> ARCH_HAS_STRICT_KERNEL_RWX
> !ARCH_OPTIONAL_KERNEL_RWX
> 
> optional:
> ARCH_HAS_STRICT_KERNEL_RWX
> ARCH_OPTIONAL_KERNEL_RWX
> with default controlled by ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
> 
> Then arm is:
>   select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
>   select ARCH_HAS_STRICT_MODULE_RWX if MMU
>   select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
>   select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7
> 
> x86 and arm64 are:
>   select ARCH_HAS_STRICT_KERNEL_RWX
>   select ARCH_HAS_STRICT_MODULE_RWX
> 
> ?
> 
> -Kees
> 

Yes, that looks good. I wanted it to be mandatory to avoid the
mindset of "optional means we don't need it" but I see there
are some cases where it's better to turn it off. I'll see if
I can emphasize this properly in the help text ("Say Y here
unless you love security exploits running in production")

Thanks,
Laura
Pavel Machek Feb. 7, 2017, 7:36 a.m. UTC | #8
On Mon 2017-02-06 10:47:45, Laura Abbott wrote:
> On 02/03/2017 01:08 PM, Kees Cook wrote:
> > On Fri, Feb 3, 2017 at 12:29 PM, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> >> On Fri, Feb 03, 2017 at 11:45:56AM -0800, Kees Cook wrote:
> >>> On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott <labbott@redhat.com> wrote:
> >>>> diff --git a/arch/Kconfig b/arch/Kconfig
> >>>> index 99839c2..22ee01e 100644
> >>>> --- a/arch/Kconfig
> >>>> +++ b/arch/Kconfig
> >>>> @@ -781,4 +781,32 @@ config VMAP_STACK
> >>>>           the stack to map directly to the KASAN shadow map using a formula
> >>>>           that is incorrect if the stack is in vmalloc space.
> >>>>
> >>>> +config ARCH_NO_STRICT_RWX_DEFAULTS
> >>>> +       def_bool n
> >>>> +
> >>>> +config ARCH_HAS_STRICT_KERNEL_RWX
> >>>> +       def_bool n
> >>>> +
> >>>> +config DEBUG_RODATA
> >>>> +       def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
> >>>> +       prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
> >>>
> >>> Ah! Yes, perfect. I totally forgot about using conditional "prompt"
> >>> lines. Nice!
> >>
> >> It's no different from the more usual:
> >>
> >>         bool "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
> >>         default y if !ARCH_NO_STRICT_RWX_DEFAULTS
> >>         depends on ARCH_HAS_STRICT_KERNEL_RWX
> >>
> >> But... I really don't like this - way too many negations and negatives
> >> which make it difficult to figure out what's going on here.
> >>
> >> The situation we have today is:
> >>
> >> -config DEBUG_RODATA
> >> -       bool "Make kernel text and rodata read-only"
> >> -       depends on MMU && !XIP_KERNEL
> >> -       default y if CPU_V7
> >>
> >> which is "allow the user to select DEBUG_RODATA if building a MMU non-XIP
> >> kernel", suggesting that the user turns it on for ARMv7 CPUs.
> >>
> >> That changes with this and the above:
> >>
> >> +       select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> >> +       select ARCH_HAS_STRICT_MODULE_RWX if MMU
> >> +       select ARCH_NO_STRICT_RWX_DEFAULTS if !CPU_V7
> >>
> >> This means that ARCH_HAS_STRICT_KERNEL_RWX is set for a MMU non-XIP
> >> kernel, which carries the same pre-condition for DEBUG_RODATA - no
> >> problem there.
> >>
> >> However, ARCH_NO_STRICT_RWX_DEFAULTS is set for non-ARMv7 CPUs, which
> >> means the "Make kernel text and rodata read-only" prompt _is_ provided
> >> for those.  However, for all ARMv7 systems, we go from "suggesting that
> >> the user enables the option" to "you don't have a choice, you get this
> >> whether you want it or not."
> >>
> >> I'd prefer to keep it off for my development systems, where I don't
> >> care about kernel security.  If we don't wish to do that as a general
> >> rule, can we make it dependent on EMBEDDED?
> >>
> >> Given that on ARM it can add up to 4MB to the kernel image - there
> >> _will_ be about 1MB before the .text section, the padding on between
> >> __modver and __ex_table which for me is around 626k, the padding
> >> between .notes and the init sections start with .vectors (the space
> >> between __ex_table and end of .notes is only 4124, which gets padded
> >> up to 1MB) and lastly the padding between the .init section and the
> >> data section (for me around 593k).  This all adds up to an increase
> >> in kernel image size of 3.2MB on 14.2MB - an increase of 22%.
> >>
> >> So no, I'm really not happy with that.
> > 
> > Ah yeah, good point. We have three cases: unsupported, mandatory,
> > optional, but we have the case of setting the default for the optional
> > case. Maybe something like this?
> > 
> > config STRICT_KERNEL_RWX
> >   bool "Make kernel text and rodata read-only" if ARCH_OPTIONAL_KERNEL_RWX
> >   depends on ARCH_HAS_STRICT_KERNEL_RWX
> >   default ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
> > 
> > unsupported:
> > !ARCH_HAS_STRICT_KERNEL_RWX
> > 
> > mandatory:
> > ARCH_HAS_STRICT_KERNEL_RWX
> > !ARCH_OPTIONAL_KERNEL_RWX
> > 
> > optional:
> > ARCH_HAS_STRICT_KERNEL_RWX
> > ARCH_OPTIONAL_KERNEL_RWX
> > with default controlled by ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
> > 
> > Then arm is:
> >   select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> >   select ARCH_HAS_STRICT_MODULE_RWX if MMU
> >   select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
> >   select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT if CPU_V7
> > 
> > x86 and arm64 are:
> >   select ARCH_HAS_STRICT_KERNEL_RWX
> >   select ARCH_HAS_STRICT_MODULE_RWX
> > 
> > ?
> > 
> > -Kees
> > 
> 
> Yes, that looks good. I wanted it to be mandatory to avoid the
> mindset of "optional means we don't need it" but I see there
> are some cases where it's better to turn it off. I'll see if
> I can emphasize this properly in the help text ("Say Y here
> unless you love security exploits running in production")

What about fixing the memory wastage, instead? If you want something
almost-always-on, it should not waste megabytes of memory.

And BTW it is help text, not advertising for your favourite feature.
									Pavel
diff mbox

Patch

diff --git a/arch/Kconfig b/arch/Kconfig
index 99839c2..22ee01e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -781,4 +781,32 @@  config VMAP_STACK
 	  the stack to map directly to the KASAN shadow map using a formula
 	  that is incorrect if the stack is in vmalloc space.
 
+config ARCH_NO_STRICT_RWX_DEFAULTS
+	def_bool n
+
+config ARCH_HAS_STRICT_KERNEL_RWX
+	def_bool n
+
+config DEBUG_RODATA
+	def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
+	prompt "Make kernel text and rodata read-only" if ARCH_NO_STRICT_RWX_DEFAULTS
+	depends on ARCH_HAS_STRICT_KERNEL_RWX
+	help
+	  If this is set, kernel text and rodata memory will be made read-only,
+	  and non-text memory will be made non-executable. This provides
+	  protection against certain security exploits (e.g. executing the heap
+	  or modifying text)
+
+config ARCH_HAS_STRICT_MODULE_RWX
+	def_bool n
+
+config DEBUG_SET_MODULE_RONX
+	def_bool y if !ARCH_NO_STRICT_RWX_DEFAULTS
+	prompt "Set loadable kenrel module data as NX and text as RO" if ARCH_NO_STRICT_RWX_DEFAULTS
+	depends on ARCH_HAS_STRICT_MODULE_RWX && MODULES
+	help
+	  If this is set, module text and rodata memory will be made read-only,
+	  and non-text memory will be made non-executable. This provides
+	  protection against certain security exploits (e.g. writing to text)
+
 source "kernel/gcov/Kconfig"
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 186c4c2..aa73ca8 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -4,10 +4,13 @@  config ARM
 	select ARCH_CLOCKSOURCE_DATA
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
 	select ARCH_HAS_ELF_RANDOMIZE
+	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
+	select ARCH_HAS_STRICT_MODULE_RWX if MMU
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
 	select ARCH_HAVE_CUSTOM_GPIO_H
 	select ARCH_HAS_GCOV_PROFILE_ALL
 	select ARCH_MIGHT_HAVE_PC_PARPORT
+	select ARCH_NO_STRICT_RWX_DEFAULTS if !CPU_V7
 	select ARCH_SUPPORTS_ATOMIC_RMW
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF
diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index d83f7c3..426d271 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1738,17 +1738,6 @@  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 && MMU
-	---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".
-
 source "drivers/hwtracing/coresight/Kconfig"
 
 endmenu
diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index f68e8ec..419a035 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -1051,18 +1051,6 @@  config ARCH_SUPPORTS_BIG_ENDIAN
 	  This option specifies the architecture can support big endian
 	  operation.
 
-config DEBUG_RODATA
-	bool "Make kernel text and rodata read-only"
-	depends on MMU && !XIP_KERNEL
-	default y if CPU_V7
-	help
-	  If this is set, kernel text and rodata memory will be made
-	  read-only, and non-text kernel memory will be made non-executable.
-	  The tradeoff is that each region is padded to section-size (1MiB)
-	  boundaries (because their permissions are different and splitting
-	  the 1M pages into 4K ones causes TLB performance problems), which
-	  can waste memory.
-
 config DEBUG_ALIGN_RODATA
 	bool "Make rodata strictly non-executable"
 	depends on DEBUG_RODATA
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..e1efbcc 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -13,6 +13,8 @@  config ARM64
 	select ARCH_HAS_GIGANTIC_PAGE
 	select ARCH_HAS_KCOV
 	select ARCH_HAS_SG_CHAIN
+	select ARCH_HAS_STRICT_KERNEL_RWX
+	select ARCH_HAS_STRICT_MODULE_RWX
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select ARCH_SUPPORTS_ATOMIC_RMW
@@ -123,9 +125,6 @@  config ARCH_PHYS_ADDR_T_64BIT
 config MMU
 	def_bool y
 
-config DEBUG_RODATA
-	def_bool y
-
 config ARM64_PAGE_SHIFT
 	int
 	default 16 if ARM64_64K_PAGES
diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index d1ebd46..939815e 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -71,17 +71,6 @@  config DEBUG_WX
 
 	  If in doubt, say "Y".
 
-config DEBUG_SET_MODULE_RONX
-	bool "Set loadable kernel module data as NX and text as RO"
-	depends on MODULES
-	default y
-	help
-	  Is this is set, kernel module text and rodata will be made read-only.
-	  This is to help catch accidental or malicious attempts to change the
-	  kernel's executable code.
-
-	  If in doubt, say Y.
-
 config DEBUG_ALIGN_RODATA
 	depends on DEBUG_RODATA
 	bool "Align linker sections up to SECTION_SIZE"
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 3a71f38..ad294b3 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -8,6 +8,7 @@  config PARISC
 	select HAVE_SYSCALL_TRACEPOINTS
 	select ARCH_WANT_FRAME_POINTERS
 	select ARCH_HAS_ELF_RANDOMIZE
+	select ARCH_HAS_STRICT_KERNEL_RWX
 	select RTC_CLASS
 	select RTC_DRV_GENERIC
 	select INIT_ALL_POSSIBLE
diff --git a/arch/parisc/Kconfig.debug b/arch/parisc/Kconfig.debug
index 68b7cbd..0d856b9 100644
--- a/arch/parisc/Kconfig.debug
+++ b/arch/parisc/Kconfig.debug
@@ -5,15 +5,4 @@  source "lib/Kconfig.debug"
 config TRACE_IRQFLAGS_SUPPORT
 	def_bool y
 
-config DEBUG_RODATA
-       bool "Write protect kernel read-only data structures"
-       depends on DEBUG_KERNEL
-       default y
-       help
-         Mark the kernel read-only data as write-protected in the pagetables,
-         in order to catch accidental (and incorrect) writes to such const
-         data. This option may have a slight performance impact because a
-         portion of the kernel code won't be covered by a TLB anymore.
-         If in doubt, say "N".
-
 endmenu
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index c6722112..53bb0e3 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -62,9 +62,6 @@  config PCI_QUIRKS
 config ARCH_SUPPORTS_UPROBES
 	def_bool y
 
-config DEBUG_RODATA
-	def_bool y
-
 config S390
 	def_bool y
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
@@ -73,6 +70,8 @@  config S390
 	select ARCH_HAS_GIGANTIC_PAGE
 	select ARCH_HAS_KCOV
 	select ARCH_HAS_SG_CHAIN
+	select ARCH_HAS_STRICT_KERNEL_RWX
+	select ARCH_HAS_STRICT_MODULE_RWX
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select ARCH_INLINE_READ_LOCK
diff --git a/arch/s390/Kconfig.debug b/arch/s390/Kconfig.debug
index 26c5d5be..57f8ea9 100644
--- a/arch/s390/Kconfig.debug
+++ b/arch/s390/Kconfig.debug
@@ -17,7 +17,4 @@  config S390_PTDUMP
 	  kernel.
 	  If in doubt, say "N"
 
-config DEBUG_SET_MODULE_RONX
-	def_bool y
-	depends on MODULES
 endmenu
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index e487493..13e1bf4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -54,6 +54,8 @@  config X86
 	select ARCH_HAS_MMIO_FLUSH
 	select ARCH_HAS_PMEM_API		if X86_64
 	select ARCH_HAS_SG_CHAIN
+	select ARCH_HAS_STRICT_KERNEL_RWX
+	select ARCH_HAS_STRICT_MODULE_RWX
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select ARCH_MIGHT_HAVE_ACPI_PDC		if ACPI
@@ -309,9 +311,6 @@  config ARCH_SUPPORTS_UPROBES
 config FIX_EARLYCON_MEM
 	def_bool y
 
-config DEBUG_RODATA
-	def_bool y
-
 config PGTABLE_LEVELS
 	int
 	default 4 if X86_64
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 67eec55..69cdd0b 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -109,17 +109,6 @@  config DEBUG_WX
 
 	  If in doubt, say "Y".
 
-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".
-
 config DEBUG_NX_TEST
 	tristate "Testcase for the NX non-executable stack feature"
 	depends on DEBUG_KERNEL && m