Re: [PATCHv2 2/2] arch: Rename CONFIG_DEBUG_RODATA and CONFIG_DEBUG_MODULE_RONX

2017-02-06 Thread Kees Cook 
On Mon, Feb 6, 2017 at 10:49 AM, Laura Abbott  wrote:
> On 02/03/2017 12:03 PM, Kees Cook wrote:
>> On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott  wrote:
>>>
>>> Both of these options are poorly named. The features they provide are
>>> necessary for system security and should not be considered debug only.
>>> Change the name to something that accurately describes what these
>>> options do.
>>
>> It may help to explicitly call out the name change from/to in the
>> commit message.
>>
>>>
>>> Signed-off-by: Laura Abbott 
>>> ---
>>> [...]
>>> diff --git a/arch/arm/configs/aspeed_g4_defconfig 
>>> b/arch/arm/configs/aspeed_g4_defconfig
>>> index ca39c04..beea2cc 100644
>>> --- a/arch/arm/configs/aspeed_g4_defconfig
>>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>>> @@ -25,7 +25,6 @@ CONFIG_MODULE_UNLOAD=y
>>>  # CONFIG_ARCH_MULTI_V7 is not set
>>>  CONFIG_ARCH_ASPEED=y
>>>  CONFIG_MACH_ASPEED_G4=y
>>> -CONFIG_DEBUG_RODATA=y
>>>  CONFIG_AEABI=y
>>>  CONFIG_UACCESS_WITH_MEMCPY=y
>>>  CONFIG_SECCOMP=y
>>
>> Are these defconfig cases correct (dropping DEBUG_RODATA without
>> adding STRICT_KERNEL_RWX)?
>>
>
> Yes, I think these need to be updated to the new config option since
> these are not CPUv7
>
>
>> Who should carry this series, btw?
>>
>
> An excellent question :)
>
> Would you be willing to carry it with Acks?

Yeah, I can push this via the KSPP tree: it's cross-architecture, so
it seems like this should go either through my tree or through akpm's
tree.

Are the arch maintainers okay with that?

-Kees

-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/2] arch: Rename CONFIG_DEBUG_RODATA and CONFIG_DEBUG_MODULE_RONX

2017-02-06 Thread Laura Abbott 
On 02/03/2017 12:03 PM, Kees Cook wrote:
> On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott  wrote:
>>
>> Both of these options are poorly named. The features they provide are
>> necessary for system security and should not be considered debug only.
>> Change the name to something that accurately describes what these
>> options do.
> 
> It may help to explicitly call out the name change from/to in the
> commit message.
> 
>>
>> Signed-off-by: Laura Abbott 
>> ---
>> [...]
>> diff --git a/arch/arm/configs/aspeed_g4_defconfig 
>> b/arch/arm/configs/aspeed_g4_defconfig
>> index ca39c04..beea2cc 100644
>> --- a/arch/arm/configs/aspeed_g4_defconfig
>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>> @@ -25,7 +25,6 @@ CONFIG_MODULE_UNLOAD=y
>>  # CONFIG_ARCH_MULTI_V7 is not set
>>  CONFIG_ARCH_ASPEED=y
>>  CONFIG_MACH_ASPEED_G4=y
>> -CONFIG_DEBUG_RODATA=y
>>  CONFIG_AEABI=y
>>  CONFIG_UACCESS_WITH_MEMCPY=y
>>  CONFIG_SECCOMP=y
> 
> Are these defconfig cases correct (dropping DEBUG_RODATA without
> adding STRICT_KERNEL_RWX)?
>

Yes, I think these need to be updated to the new config option since
these are not CPUv7


> Who should carry this series, btw?
> 

An excellent question :)

Would you be willing to carry it with Acks?

> -Kees
> 

Thanks,
Laura
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/2] arch: Rename CONFIG_DEBUG_RODATA and CONFIG_DEBUG_MODULE_RONX

2017-02-03 Thread Kees Cook 
On Fri, Feb 3, 2017 at 9:52 AM, Laura Abbott  wrote:
>
> Both of these options are poorly named. The features they provide are
> necessary for system security and should not be considered debug only.
> Change the name to something that accurately describes what these
> options do.

It may help to explicitly call out the name change from/to in the
commit message.

>
> Signed-off-by: Laura Abbott 
> ---
> [...]
> diff --git a/arch/arm/configs/aspeed_g4_defconfig 
> b/arch/arm/configs/aspeed_g4_defconfig
> index ca39c04..beea2cc 100644
> --- a/arch/arm/configs/aspeed_g4_defconfig
> +++ b/arch/arm/configs/aspeed_g4_defconfig
> @@ -25,7 +25,6 @@ CONFIG_MODULE_UNLOAD=y
>  # CONFIG_ARCH_MULTI_V7 is not set
>  CONFIG_ARCH_ASPEED=y
>  CONFIG_MACH_ASPEED_G4=y
> -CONFIG_DEBUG_RODATA=y
>  CONFIG_AEABI=y
>  CONFIG_UACCESS_WITH_MEMCPY=y
>  CONFIG_SECCOMP=y

Are these defconfig cases correct (dropping DEBUG_RODATA without
adding STRICT_KERNEL_RWX)?

Who should carry this series, btw?

-Kees

-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/2] arch: Rename CONFIG_DEBUG_RODATA and CONFIG_DEBUG_MODULE_RONX

2017-02-03 Thread Mark Rutland 
On Fri, Feb 03, 2017 at 09:52:22AM -0800, Laura Abbott wrote:
> 
> Both of these options are poorly named. The features they provide are
> necessary for system security and should not be considered debug only.
> Change the name to something that accurately describes what these
> options do.
> 
> Signed-off-by: Laura Abbott 

As with patch 1, this looks good to me. FWIW:

Acked-by: Mark Rutland 

> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index 939815e..560a8d8 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -72,7 +72,7 @@ config DEBUG_WX
> If in doubt, say "Y".
>  
>  config DEBUG_ALIGN_RODATA
> - depends on DEBUG_RODATA
> + depends on STRICT_KERNEL_RWX
>   bool "Align linker sections up to SECTION_SIZE"
>   help
> If this option is enabled, sections that may potentially be marked as
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 94b62c1..67f9cb9 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -93,7 +93,7 @@ static void __kprobes *patch_map(void *addr, int fixmap)
>   bool module = !core_kernel_text(uintaddr);
>   struct page *page;
>  
> - if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX))
> + if (module && IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
>   page = vmalloc_to_page(addr);
>   else if (!module)
>   page = pfn_to_page(PHYS_PFN(__pa(addr)));

Since CONFIG_STRICT_MODULE_RWX is mandatory for arm64, we should be able
to simplify this, but that can wait for a later patch.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 2/2] arch: Rename CONFIG_DEBUG_RODATA and CONFIG_DEBUG_MODULE_RONX

2017-02-03 Thread Laura Abbott 

Both of these options are poorly named. The features they provide are
necessary for system security and should not be considered debug only.
Change the name to something that accurately describes what these
options do.

Signed-off-by: Laura Abbott 
---
v2: This patch is now doing the renaming of CONFIG_DEBUG_RODATA and
CONFIG_DEBUG_MODULE_RONX
---
 Documentation/DocBook/kgdb.tmpl| 8 
 Documentation/security/self-protection.txt | 4 ++--
 arch/Kconfig   | 4 ++--
 arch/arm/configs/aspeed_g4_defconfig   | 3 +--
 arch/arm/configs/aspeed_g5_defconfig   | 3 +--
 arch/arm/include/asm/cacheflush.h  | 2 +-
 arch/arm/kernel/patch.c| 4 ++--
 arch/arm/kernel/vmlinux.lds.S  | 8 
 arch/arm/mm/Kconfig| 2 +-
 arch/arm/mm/init.c | 4 ++--
 arch/arm64/Kconfig.debug   | 2 +-
 arch/arm64/kernel/insn.c   | 2 +-
 arch/parisc/configs/712_defconfig  | 1 -
 arch/parisc/configs/c3000_defconfig| 1 -
 arch/parisc/mm/init.c  | 2 +-
 include/linux/filter.h | 4 ++--
 include/linux/init.h   | 4 ++--
 include/linux/module.h | 2 +-
 init/main.c| 4 ++--
 kernel/configs/android-recommended.config  | 2 +-
 kernel/module.c| 6 +++---
 kernel/power/hibernate.c   | 2 +-
 kernel/power/power.h   | 4 ++--
 kernel/power/snapshot.c| 4 ++--
 24 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/Documentation/DocBook/kgdb.tmpl b/Documentation/DocBook/kgdb.tmpl
index f3abca7..856ac20 100644
--- a/Documentation/DocBook/kgdb.tmpl
+++ b/Documentation/DocBook/kgdb.tmpl
@@ -115,12 +115,12 @@
 
 
 If the architecture that you are using supports the kernel option
-CONFIG_DEBUG_RODATA, you should consider turning it off.  This
+CONFIG_STRICT_KERNEL_RWX, you should consider turning it off.  This
 option will prevent the use of software breakpoints because it
 marks certain regions of the kernel's memory space as read-only.
 If kgdb supports it for the architecture you are using, you can
 use hardware breakpoints if you desire to run with the
-CONFIG_DEBUG_RODATA option turned on, else you need to turn off
+CONFIG_STRICT_KERNEL_RWX option turned on, else you need to turn off
 this option.
 
 
@@ -135,7 +135,7 @@
 Here is an example set of .config symbols to enable or
 disable for kgdb:
 
-# CONFIG_DEBUG_RODATA is not set
+# CONFIG_STRICT_KERNEL_RWX is not set
 CONFIG_FRAME_POINTER=y
 CONFIG_KGDB=y
 CONFIG_KGDB_SERIAL_CONSOLE=y
@@ -166,7 +166,7 @@
 
 Here is an example set of .config symbols to enable/disable kdb:
 
-# CONFIG_DEBUG_RODATA is not set
+# CONFIG_STRICT_KERNEL_RWX is not set
 CONFIG_FRAME_POINTER=y
 CONFIG_KGDB=y
 CONFIG_KGDB_SERIAL_CONSOLE=y
diff --git a/Documentation/security/self-protection.txt 
b/Documentation/security/self-protection.txt
index 3010576..dd2a3b1 100644
--- a/Documentation/security/self-protection.txt
+++ b/Documentation/security/self-protection.txt
@@ -51,8 +51,8 @@ kernel, they are implemented in a way where the memory is 
temporarily
 made writable during the update, and then returned to the original
 permissions.)
 
-In support of this are (the poorly named) CONFIG_DEBUG_RODATA and
-CONFIG_DEBUG_SET_MODULE_RONX, which seek to make sure that code is not
+In support of this are CONFIG_STRICT_KERNEL_RWX and
+CONFIG_STRICT_MODULE_RWX, which seek to make sure that code is not
 writable, data is not executable, and read-only data is neither writable
 nor executable.
 
diff --git a/arch/Kconfig b/arch/Kconfig
index 22ee01e..406f6cd 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -787,7 +787,7 @@ config ARCH_NO_STRICT_RWX_DEFAULTS
 config ARCH_HAS_STRICT_KERNEL_RWX
def_bool n
 
-config DEBUG_RODATA
+config STRICT_KERNEL_RWX
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
@@ -800,7 +800,7 @@ config DEBUG_RODATA
 config ARCH_HAS_STRICT_MODULE_RWX
def_bool n
 
-config DEBUG_SET_MODULE_RONX
+config STRICT_MODULE_RWX
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
diff --git a/arch/arm/configs/aspeed_g4_defconfig 
b/arch/arm/configs/aspeed_g4_defconfig
index ca39c04..beea2cc 100644
--- a/arch/arm/configs/aspeed_g4_defconfig
+++ b/arch/arm/configs/aspeed_g4_defconfig
@@ -25,7 +25,6 @@ CONFIG_MODULE_UNLOAD=y
 # CONFIG_ARCH_MULTI_V7 is not set
 CONFIG_ARCH_ASPEED=y
 CONFIG_MACH_ASPEED_G4=y