Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1280406 > unrolled thread
| Started by | Kees Cook <keescook@chromium.org> |
|---|---|
| First post | 2015-12-01 00:50 +0100 |
| Last post | 2015-12-01 02:10 +0100 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] ARM: mm: flip priority of CONFIG_DEBUG_RODATA Kees Cook <keescook@chromium.org> - 2015-12-01 00:50 +0100
Re: [PATCH] ARM: mm: flip priority of CONFIG_DEBUG_RODATA Kees Cook <keescook@chromium.org> - 2015-12-01 02:10 +0100
Re: [PATCH] ARM: mm: flip priority of CONFIG_DEBUG_RODATA Laura Abbott <labbott@redhat.com> - 2015-12-01 02:30 +0100
Re: [PATCH] ARM: mm: flip priority of CONFIG_DEBUG_RODATA Nicolas Pitre <nicolas.pitre@linaro.org> - 2015-12-01 04:30 +0100
Re: [PATCH] ARM: mm: flip priority of CONFIG_DEBUG_RODATA Kees Cook <keescook@chromium.org> - 2015-12-01 20:20 +0100
Re: [PATCH] ARM: mm: flip priority of CONFIG_DEBUG_RODATA Laura Abbott <labbott@redhat.com> - 2015-12-01 02:10 +0100
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2015-12-01 00:50 +0100 |
| Subject | [PATCH] ARM: mm: flip priority of CONFIG_DEBUG_RODATA |
| Message-ID | <qAGmK-1Ri-7@gated-at.bofh.it> |
Given the choice between making things NX or making things RO, we want
RO first. As such, redefine CONFIG_DEBUG_RODATA to actually do the bulk
of the ROing (and NXing). In the place of the old CONFIG_DEBUG_RODATA,
introduce CONFIG_DEBUG_RODATA_ALIGN (after arm64's config that does the
same thing) to add the additional section alignment for making rodata
explicitly NX. Also adds human readable names to the sections so I
could more easily debug my typos, and makes CONFIG_DEBUG_RODATA default
"y" for CPU_V7.
Results in /sys/kernel/debug/kernel_page_tables for each config state:
# CONFIG_DEBUG_RODATA is not set
# CONFIG_DEBUG_ALIGN_RODATA is not set
---[ Kernel Mapping ]---
0x80000000-0x80900000 9M RW x SHD
0x80900000-0xa0000000 503M RW NX SHD
CONFIG_DEBUG_RODATA=y
CONFIG_DEBUG_ALIGN_RODATA=y
---[ Kernel Mapping ]---
0x80000000-0x80100000 1M RW NX SHD
0x80100000-0x80700000 6M ro x SHD
0x80700000-0x80a00000 3M ro NX SHD
0x80a00000-0xa0000000 502M RW NX SHD
CONFIG_DEBUG_RODATA=y
# CONFIG_DEBUG_ALIGN_RODATA is not set
---[ Kernel Mapping ]---
0x80000000-0x80100000 1M RW NX SHD
0x80100000-0x80a00000 9M ro x SHD
0x80a00000-0xa0000000 502M RW NX SHD
Signed-off-by: Kees Cook <keescook@chromium.org>
---
arch/arm/kernel/vmlinux.lds.S | 10 +++++-----
arch/arm/mm/Kconfig | 34 ++++++++++++++++++----------------
arch/arm/mm/init.c | 18 ++++++++++--------
3 files changed, 33 insertions(+), 29 deletions(-)
diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index 8b60fde5ce48..a6e395c53a48 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -8,7 +8,7 @@
#include <asm/thread_info.h>
#include <asm/memory.h>
#include <asm/page.h>
-#ifdef CONFIG_ARM_KERNMEM_PERMS
+#ifdef CONFIG_DEBUG_RODATA
#include <asm/pgtable.h>
#endif
@@ -94,7 +94,7 @@ SECTIONS
HEAD_TEXT
}
-#ifdef CONFIG_ARM_KERNMEM_PERMS
+#ifdef CONFIG_DEBUG_RODATA
. = ALIGN(1<<SECTION_SHIFT);
#endif
@@ -117,7 +117,7 @@ SECTIONS
ARM_CPU_KEEP(PROC_INFO)
}
-#ifdef CONFIG_DEBUG_RODATA
+#ifdef CONFIG_DEBUG_ALIGN_RODATA
. = ALIGN(1<<SECTION_SHIFT);
#endif
RO_DATA(PAGE_SIZE)
@@ -153,7 +153,7 @@ SECTIONS
_etext = .; /* End of text and rodata section */
#ifndef CONFIG_XIP_KERNEL
-# ifdef CONFIG_ARM_KERNMEM_PERMS
+# ifdef CONFIG_DEBUG_RODATA
. = ALIGN(1<<SECTION_SHIFT);
# else
. = ALIGN(PAGE_SIZE);
@@ -231,7 +231,7 @@ SECTIONS
__data_loc = ALIGN(4); /* location in binary */
. = PAGE_OFFSET + TEXT_OFFSET;
#else
-#ifdef CONFIG_ARM_KERNMEM_PERMS
+#ifdef CONFIG_DEBUG_RODATA
. = ALIGN(1<<SECTION_SHIFT);
#else
. = ALIGN(THREAD_SIZE);
diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index 41218867a9a6..b617084e9520 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -1039,24 +1039,26 @@ config ARCH_SUPPORTS_BIG_ENDIAN
This option specifies the architecture can support big endian
operation.
-config ARM_KERNMEM_PERMS
- bool "Restrict kernel memory permissions"
+config DEBUG_RODATA
+ bool "Make kernel text and rodata read-only"
depends on MMU
+ default y if CPU_V7
help
- If this is set, kernel memory other than kernel text (and rodata)
- 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), wasting memory.
+ If this is set, kernel memory (text, rodata, etc) 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_RODATA
- bool "Make kernel text and rodata read-only"
- depends on ARM_KERNMEM_PERMS
+config DEBUG_ALIGN_RODATA
+ bool "Make rodata strictly non-executable"
+ depends on DEBUG_RODATA
default y
help
- If this is set, kernel text and rodata will be made read-only. This
- is to help catch accidental or malicious attempts to change the
- kernel's executable code. Additionally splits rodata from kernel
- text so it can be made explicitly non-executable. This creates
- another section-size padded region, so it can waste more memory
- space while gaining the read-only protections.
+ If this is set, rodata will be made explicitly non-executable. This
+ provides protection on the rare chance that attackers might find and
+ use ROP gadgets that exist in the rodata section. This adds an
+ additional section-aligned split of rodata from kernel text so it
+ can be made explicitly non-executable. This padding may waste memory
+ space to gain this additional protection.
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 8a63b4cdc0f2..e99f65fbcf2b 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -568,8 +568,9 @@ void __init mem_init(void)
}
}
-#ifdef CONFIG_ARM_KERNMEM_PERMS
+#ifdef CONFIG_DEBUG_RODATA
struct section_perm {
+ const char *name;
unsigned long start;
unsigned long end;
pmdval_t mask;
@@ -580,6 +581,7 @@ struct section_perm {
static struct section_perm nx_perms[] = {
/* Make pages tables, etc before _stext RW (set NX). */
{
+ .name = "pre-text NX",
.start = PAGE_OFFSET,
.end = (unsigned long)_stext,
.mask = ~PMD_SECT_XN,
@@ -587,14 +589,16 @@ static struct section_perm nx_perms[] = {
},
/* Make init RW (set NX). */
{
+ .name = "init NX",
.start = (unsigned long)__init_begin,
.end = (unsigned long)_sdata,
.mask = ~PMD_SECT_XN,
.prot = PMD_SECT_XN,
},
-#ifdef CONFIG_DEBUG_RODATA
+#ifdef CONFIG_DEBUG_ALIGN_RODATA
/* Make rodata NX (set RO in ro_perms below). */
{
+ .name = "rodata NX",
.start = (unsigned long)__start_rodata,
.end = (unsigned long)__init_begin,
.mask = ~PMD_SECT_XN,
@@ -603,10 +607,10 @@ static struct section_perm nx_perms[] = {
#endif
};
-#ifdef CONFIG_DEBUG_RODATA
static struct section_perm ro_perms[] = {
/* Make kernel code and rodata RX (set RO). */
{
+ .name = "text/rodata RO",
.start = (unsigned long)_stext,
.end = (unsigned long)__init_begin,
#ifdef CONFIG_ARM_LPAE
@@ -619,7 +623,6 @@ static struct section_perm ro_perms[] = {
#endif
},
};
-#endif
/*
* Updates section permissions only for the current mm (sections are
@@ -666,7 +669,8 @@ static inline bool arch_has_strict_perms(void)
for (i = 0; i < ARRAY_SIZE(perms); i++) { \
if (!IS_ALIGNED(perms[i].start, SECTION_SIZE) || \
!IS_ALIGNED(perms[i].end, SECTION_SIZE)) { \
- pr_err("BUG: section %lx-%lx not aligned to %lx\n", \
+ pr_err("BUG: %s section %lx-%lx not aligned to %lx\n", \
+ perms[i].name, \
perms[i].start, perms[i].end, \
SECTION_SIZE); \
continue; \
@@ -685,7 +689,6 @@ static inline void fix_kernmem_perms(void)
set_section_perms(nx_perms, prot);
}
-#ifdef CONFIG_DEBUG_RODATA
void mark_rodata_ro(void)
{
set_section_perms(ro_perms, prot);
@@ -700,11 +703,10 @@ void set_kernel_text_ro(void)
{
set_section_perms(ro_perms, prot);
}
-#endif /* CONFIG_DEBUG_RODATA */
#else
static inline void fix_kernmem_perms(void) { }
-#endif /* CONFIG_ARM_KERNMEM_PERMS */
+#endif /* CONFIG_DEBUG_RODATA */
void free_tcmmem(void)
{
--
1.9.1
--
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2015-12-01 02:10 +0100 |
| Message-ID | <qAHC9-2Om-11@gated-at.bofh.it> |
| In reply to | #1280406 |
On Mon, Nov 30, 2015 at 5:03 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 11/30/2015 03:38 PM, Kees Cook wrote:
>>
>> Given the choice between making things NX or making things RO, we want
>> RO first. As such, redefine CONFIG_DEBUG_RODATA to actually do the bulk
>
>
> Can you give a citation for why? The thread that inspired it might be
> a good link.
This was inspired by my examining the existing architecture's
implementations of CONFIG_DEBUG_RODATA after Ingo suggested it be made
a common feature not a build-time config (or at least renamed):
http://www.openwall.com/lists/kernel-hardening/2015/11/30/13
>> of the ROing (and NXing). In the place of the old CONFIG_DEBUG_RODATA,
>> introduce CONFIG_DEBUG_RODATA_ALIGN (after arm64's config that does the
>> same thing) to add the additional section alignment for making rodata
>> explicitly NX. Also adds human readable names to the sections so I
>> could more easily debug my typos, and makes CONFIG_DEBUG_RODATA default
>> "y" for CPU_V7.
>>
>> Results in /sys/kernel/debug/kernel_page_tables for each config state:
>>
>> # CONFIG_DEBUG_RODATA is not set
>> # CONFIG_DEBUG_ALIGN_RODATA is not set
>>
>> ---[ Kernel Mapping ]---
>> 0x80000000-0x80900000 9M RW x SHD
>> 0x80900000-0xa0000000 503M RW NX SHD
>>
>> CONFIG_DEBUG_RODATA=y
>> CONFIG_DEBUG_ALIGN_RODATA=y
>>
>> ---[ Kernel Mapping ]---
>> 0x80000000-0x80100000 1M RW NX SHD
>> 0x80100000-0x80700000 6M ro x SHD
>> 0x80700000-0x80a00000 3M ro NX SHD
>> 0x80a00000-0xa0000000 502M RW NX SHD
>>
>> CONFIG_DEBUG_RODATA=y
>> # CONFIG_DEBUG_ALIGN_RODATA is not set
>>
>> ---[ Kernel Mapping ]---
>> 0x80000000-0x80100000 1M RW NX SHD
>> 0x80100000-0x80a00000 9M ro x SHD
>> 0x80a00000-0xa0000000 502M RW NX SHD
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> arch/arm/kernel/vmlinux.lds.S | 10 +++++-----
>> arch/arm/mm/Kconfig | 34 ++++++++++++++++++----------------
>> arch/arm/mm/init.c | 18 ++++++++++--------
>> 3 files changed, 33 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
>> index 8b60fde5ce48..a6e395c53a48 100644
>> --- a/arch/arm/kernel/vmlinux.lds.S
>> +++ b/arch/arm/kernel/vmlinux.lds.S
>> @@ -8,7 +8,7 @@
>> #include <asm/thread_info.h>
>> #include <asm/memory.h>
>> #include <asm/page.h>
>> -#ifdef CONFIG_ARM_KERNMEM_PERMS
>> +#ifdef CONFIG_DEBUG_RODATA
>> #include <asm/pgtable.h>
>> #endif
>>
>> @@ -94,7 +94,7 @@ SECTIONS
>> HEAD_TEXT
>> }
>>
>> -#ifdef CONFIG_ARM_KERNMEM_PERMS
>> +#ifdef CONFIG_DEBUG_RODATA
>> . = ALIGN(1<<SECTION_SHIFT);
>> #endif
>>
>> @@ -117,7 +117,7 @@ SECTIONS
>> ARM_CPU_KEEP(PROC_INFO)
>> }
>>
>> -#ifdef CONFIG_DEBUG_RODATA
>> +#ifdef CONFIG_DEBUG_ALIGN_RODATA
>> . = ALIGN(1<<SECTION_SHIFT);
>> #endif
>> RO_DATA(PAGE_SIZE)
>> @@ -153,7 +153,7 @@ SECTIONS
>> _etext = .; /* End of text and rodata section
>> */
>>
>> #ifndef CONFIG_XIP_KERNEL
>> -# ifdef CONFIG_ARM_KERNMEM_PERMS
>> +# ifdef CONFIG_DEBUG_RODATA
>> . = ALIGN(1<<SECTION_SHIFT);
>> # else
>> . = ALIGN(PAGE_SIZE);
>> @@ -231,7 +231,7 @@ SECTIONS
>> __data_loc = ALIGN(4); /* location in binary */
>> . = PAGE_OFFSET + TEXT_OFFSET;
>> #else
>> -#ifdef CONFIG_ARM_KERNMEM_PERMS
>> +#ifdef CONFIG_DEBUG_RODATA
>> . = ALIGN(1<<SECTION_SHIFT);
>> #else
>> . = ALIGN(THREAD_SIZE);
>> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
>> index 41218867a9a6..b617084e9520 100644
>> --- a/arch/arm/mm/Kconfig
>> +++ b/arch/arm/mm/Kconfig
>> @@ -1039,24 +1039,26 @@ config ARCH_SUPPORTS_BIG_ENDIAN
>> This option specifies the architecture can support big endian
>> operation.
>>
>> -config ARM_KERNMEM_PERMS
>> - bool "Restrict kernel memory permissions"
>> +config DEBUG_RODATA
>> + bool "Make kernel text and rodata read-only"
>> depends on MMU
>> + default y if CPU_V7
>> help
>> - If this is set, kernel memory other than kernel text (and
>> rodata)
>> - 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), wasting memory.
>> + If this is set, kernel memory (text, rodata, etc) 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_RODATA
>> - bool "Make kernel text and rodata read-only"
>> - depends on ARM_KERNMEM_PERMS
>> +config DEBUG_ALIGN_RODATA
>> + bool "Make rodata strictly non-executable"
>> + depends on DEBUG_RODATA
>> default y
>> help
>> - If this is set, kernel text and rodata will be made read-only.
>> This
>> - is to help catch accidental or malicious attempts to change the
>> - kernel's executable code. Additionally splits rodata from kernel
>> - text so it can be made explicitly non-executable. This creates
>> - another section-size padded region, so it can waste more memory
>> - space while gaining the read-only protections.
>> + If this is set, rodata will be made explicitly non-executable.
>> This
>> + provides protection on the rare chance that attackers might find
>> and
>> + use ROP gadgets that exist in the rodata section. This adds an
>> + additional section-aligned split of rodata from kernel text so
>> it
>> + can be made explicitly non-executable. This padding may waste
>> memory
>> + space to gain this additional protection.
>
>
> I get that you want to make this match arm64 but it's really not intuitive that
> something with ALIGN_RODATA in the name is actually for setting NX. The purpose
> of ALIGN_RODATA was also slightly different on arm64 since the RO/NX will still
> be there, the difference is if the sections are present versus broken down into
> pages.
Well, it seems to have the same effect: without the alignment, a
portion of rodata may remain executable on arm64. Unless I
misunderstand?
-Kees
>
>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> index 8a63b4cdc0f2..e99f65fbcf2b 100644
>> --- a/arch/arm/mm/init.c
>> +++ b/arch/arm/mm/init.c
>> @@ -568,8 +568,9 @@ void __init mem_init(void)
>> }
>> }
>>
>> -#ifdef CONFIG_ARM_KERNMEM_PERMS
>> +#ifdef CONFIG_DEBUG_RODATA
>> struct section_perm {
>> + const char *name;
>> unsigned long start;
>> unsigned long end;
>> pmdval_t mask;
>> @@ -580,6 +581,7 @@ struct section_perm {
>> static struct section_perm nx_perms[] = {
>> /* Make pages tables, etc before _stext RW (set NX). */
>> {
>> + .name = "pre-text NX",
>> .start = PAGE_OFFSET,
>> .end = (unsigned long)_stext,
>> .mask = ~PMD_SECT_XN,
>> @@ -587,14 +589,16 @@ static struct section_perm nx_perms[] = {
>> },
>> /* Make init RW (set NX). */
>> {
>> + .name = "init NX",
>> .start = (unsigned long)__init_begin,
>> .end = (unsigned long)_sdata,
>> .mask = ~PMD_SECT_XN,
>> .prot = PMD_SECT_XN,
>> },
>> -#ifdef CONFIG_DEBUG_RODATA
>> +#ifdef CONFIG_DEBUG_ALIGN_RODATA
>> /* Make rodata NX (set RO in ro_perms below). */
>> {
>> + .name = "rodata NX",
>> .start = (unsigned long)__start_rodata,
>> .end = (unsigned long)__init_begin,
>> .mask = ~PMD_SECT_XN,
>> @@ -603,10 +607,10 @@ static struct section_perm nx_perms[] = {
>> #endif
>> };
>>
>> -#ifdef CONFIG_DEBUG_RODATA
>> static struct section_perm ro_perms[] = {
>> /* Make kernel code and rodata RX (set RO). */
>> {
>> + .name = "text/rodata RO",
>> .start = (unsigned long)_stext,
>> .end = (unsigned long)__init_begin,
>> #ifdef CONFIG_ARM_LPAE
>> @@ -619,7 +623,6 @@ static struct section_perm ro_perms[] = {
>> #endif
>> },
>> };
>> -#endif
>>
>> /*
>> * Updates section permissions only for the current mm (sections are
>> @@ -666,7 +669,8 @@ static inline bool arch_has_strict_perms(void)
>> for (i = 0; i < ARRAY_SIZE(perms); i++) { \
>> if (!IS_ALIGNED(perms[i].start, SECTION_SIZE) || \
>> !IS_ALIGNED(perms[i].end, SECTION_SIZE)) { \
>> - pr_err("BUG: section %lx-%lx not aligned to
>> %lx\n", \
>> + pr_err("BUG: %s section %lx-%lx not aligned to
>> %lx\n", \
>> + perms[i].name, \
>> perms[i].start, perms[i].end, \
>> SECTION_SIZE); \
>> continue; \
>> @@ -685,7 +689,6 @@ static inline void fix_kernmem_perms(void)
>> set_section_perms(nx_perms, prot);
>> }
>>
>> -#ifdef CONFIG_DEBUG_RODATA
>> void mark_rodata_ro(void)
>> {
>> set_section_perms(ro_perms, prot);
>> @@ -700,11 +703,10 @@ void set_kernel_text_ro(void)
>> {
>> set_section_perms(ro_perms, prot);
>> }
>> -#endif /* CONFIG_DEBUG_RODATA */
>>
>> #else
>> static inline void fix_kernmem_perms(void) { }
>> -#endif /* CONFIG_ARM_KERNMEM_PERMS */
>> +#endif /* CONFIG_DEBUG_RODATA */
>>
>> void free_tcmmem(void)
>> {
>>
>
> Thanks,
> Laura
--
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2015-12-01 02:30 +0100 |
| Message-ID | <qAHVw-2Vf-23@gated-at.bofh.it> |
| In reply to | #1280449 |
On 11/30/2015 05:08 PM, Kees Cook wrote: > On Mon, Nov 30, 2015 at 5:03 PM, Laura Abbott <labbott@redhat.com> wrote: >> On 11/30/2015 03:38 PM, Kees Cook wrote: >>> >>> Given the choice between making things NX or making things RO, we want >>> RO first. As such, redefine CONFIG_DEBUG_RODATA to actually do the bulk >> >> >> Can you give a citation for why? The thread that inspired it might be >> a good link. > > This was inspired by my examining the existing architecture's > implementations of CONFIG_DEBUG_RODATA after Ingo suggested it be made > a common feature not a build-time config (or at least renamed): > http://www.openwall.com/lists/kernel-hardening/2015/11/30/13 > Thanks. I read the thread and I think it would be good to put a link in the commit message to make it clearer why this is going in. >>> index 41218867a9a6..b617084e9520 100644 >>> --- a/arch/arm/mm/Kconfig >>> +++ b/arch/arm/mm/Kconfig >>> @@ -1039,24 +1039,26 @@ config ARCH_SUPPORTS_BIG_ENDIAN >>> This option specifies the architecture can support big endian >>> operation. >>> >>> -config ARM_KERNMEM_PERMS >>> - bool "Restrict kernel memory permissions" >>> +config DEBUG_RODATA >>> + bool "Make kernel text and rodata read-only" >>> depends on MMU >>> + default y if CPU_V7 >>> help >>> - If this is set, kernel memory other than kernel text (and >>> rodata) >>> - 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), wasting memory. >>> + If this is set, kernel memory (text, rodata, etc) 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_RODATA >>> - bool "Make kernel text and rodata read-only" >>> - depends on ARM_KERNMEM_PERMS >>> +config DEBUG_ALIGN_RODATA >>> + bool "Make rodata strictly non-executable" >>> + depends on DEBUG_RODATA >>> default y >>> help >>> - If this is set, kernel text and rodata will be made read-only. >>> This >>> - is to help catch accidental or malicious attempts to change the >>> - kernel's executable code. Additionally splits rodata from kernel >>> - text so it can be made explicitly non-executable. This creates >>> - another section-size padded region, so it can waste more memory >>> - space while gaining the read-only protections. >>> + If this is set, rodata will be made explicitly non-executable. >>> This >>> + provides protection on the rare chance that attackers might find >>> and >>> + use ROP gadgets that exist in the rodata section. This adds an >>> + additional section-aligned split of rodata from kernel text so >>> it >>> + can be made explicitly non-executable. This padding may waste >>> memory >>> + space to gain this additional protection. >> >> >> I get that you want to make this match arm64 but it's really not intuitive that >> something with ALIGN_RODATA in the name is actually for setting NX. The purpose >> of ALIGN_RODATA was also slightly different on arm64 since the RO/NX will still >> be there, the difference is if the sections are present versus broken down into >> pages. > > Well, it seems to have the same effect: without the alignment, a > portion of rodata may remain executable on arm64. Unless I > misunderstand? > No, on arm64 everything should always be NX, the difference is part of the NX sections may be mapped as pages instead of sections so you take the TLB hit. It's a trade off of memory vs TLB pressure instead of just security vs TLB. Thanks, Laura -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Nicolas Pitre <nicolas.pitre@linaro.org> |
|---|---|
| Date | 2015-12-01 04:30 +0100 |
| Message-ID | <qAJND-49P-9@gated-at.bofh.it> |
| In reply to | #1280465 |
On Mon, 30 Nov 2015, Laura Abbott wrote: > On 11/30/2015 05:08 PM, Kees Cook wrote: > > On Mon, Nov 30, 2015 at 5:03 PM, Laura Abbott <labbott@redhat.com> wrote: > > > On 11/30/2015 03:38 PM, Kees Cook wrote: > > > > > > > > Given the choice between making things NX or making things RO, we want > > > > RO first. As such, redefine CONFIG_DEBUG_RODATA to actually do the bulk > > > > > > > > > Can you give a citation for why? The thread that inspired it might be > > > a good link. > > > > This was inspired by my examining the existing architecture's > > implementations of CONFIG_DEBUG_RODATA after Ingo suggested it be made > > a common feature not a build-time config (or at least renamed): > > http://www.openwall.com/lists/kernel-hardening/2015/11/30/13 > > > > Thanks. I read the thread and I think it would be good to put a link > in the commit message to make it clearer why this is going in. A link is good, but a summary is even better. You may add both for best results. > > > > > index 41218867a9a6..b617084e9520 100644 > > > > --- a/arch/arm/mm/Kconfig > > > > +++ b/arch/arm/mm/Kconfig > > > > @@ -1039,24 +1039,26 @@ config ARCH_SUPPORTS_BIG_ENDIAN > > > > This option specifies the architecture can support big endian > > > > operation. > > > > > > > > -config ARM_KERNMEM_PERMS > > > > - bool "Restrict kernel memory permissions" > > > > +config DEBUG_RODATA > > > > + bool "Make kernel text and rodata read-only" > > > > depends on MMU > > > > + default y if CPU_V7 > > > > help > > > > - If this is set, kernel memory other than kernel text (and > > > > rodata) > > > > - 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), wasting memory. > > > > + If this is set, kernel memory (text, rodata, etc) 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_RODATA > > > > - bool "Make kernel text and rodata read-only" > > > > - depends on ARM_KERNMEM_PERMS > > > > +config DEBUG_ALIGN_RODATA > > > > + bool "Make rodata strictly non-executable" > > > > + depends on DEBUG_RODATA > > > > default y > > > > help > > > > - If this is set, kernel text and rodata will be made read-only. > > > > This > > > > - is to help catch accidental or malicious attempts to change > > > > the > > > > - kernel's executable code. Additionally splits rodata from > > > > kernel > > > > - text so it can be made explicitly non-executable. This creates > > > > - another section-size padded region, so it can waste more > > > > memory > > > > - space while gaining the read-only protections. > > > > + If this is set, rodata will be made explicitly non-executable. > > > > This > > > > + provides protection on the rare chance that attackers might > > > > find > > > > and > > > > + use ROP gadgets that exist in the rodata section. This adds an > > > > + additional section-aligned split of rodata from kernel text so > > > > it > > > > + can be made explicitly non-executable. This padding may waste > > > > memory > > > > + space to gain this additional protection. > > > > > > > > > I get that you want to make this match arm64 but it's really not intuitive > > > that > > > something with ALIGN_RODATA in the name is actually for setting NX. The > > > purpose > > > of ALIGN_RODATA was also slightly different on arm64 since the RO/NX will > > > still > > > be there, the difference is if the sections are present versus broken down > > > into > > > pages. > > > > Well, it seems to have the same effect: without the alignment, a > > portion of rodata may remain executable on arm64. Unless I > > misunderstand? > > > > No, on arm64 everything should always be NX, the difference is part of the NX > sections may be mapped as pages instead of sections so you take the TLB hit. > It's a trade off of memory vs TLB pressure instead of just security vs TLB. > > Thanks, > Laura > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2015-12-01 20:20 +0100 |
| Message-ID | <qAYD0-5ha-3@gated-at.bofh.it> |
| In reply to | #1280465 |
On Mon, Nov 30, 2015 at 5:20 PM, Laura Abbott <labbott@redhat.com> wrote: > On 11/30/2015 05:08 PM, Kees Cook wrote: >> >> On Mon, Nov 30, 2015 at 5:03 PM, Laura Abbott <labbott@redhat.com> wrote: >>> >>> On 11/30/2015 03:38 PM, Kees Cook wrote: >>>> >>>> >>>> Given the choice between making things NX or making things RO, we want >>>> RO first. As such, redefine CONFIG_DEBUG_RODATA to actually do the bulk >>> >>> >>> >>> Can you give a citation for why? The thread that inspired it might be >>> a good link. >> >> >> This was inspired by my examining the existing architecture's >> implementations of CONFIG_DEBUG_RODATA after Ingo suggested it be made >> a common feature not a build-time config (or at least renamed): >> http://www.openwall.com/lists/kernel-hardening/2015/11/30/13 >> > > Thanks. I read the thread and I think it would be good to put a link > in the commit message to make it clearer why this is going in. Okay, I will add this (and a summary, as Nicolas suggested). >>>> index 41218867a9a6..b617084e9520 100644 >>>> --- a/arch/arm/mm/Kconfig >>>> +++ b/arch/arm/mm/Kconfig >>>> @@ -1039,24 +1039,26 @@ config ARCH_SUPPORTS_BIG_ENDIAN >>>> This option specifies the architecture can support big endian >>>> operation. >>>> >>>> -config ARM_KERNMEM_PERMS >>>> - bool "Restrict kernel memory permissions" >>>> +config DEBUG_RODATA >>>> + bool "Make kernel text and rodata read-only" >>>> depends on MMU >>>> + default y if CPU_V7 >>>> help >>>> - If this is set, kernel memory other than kernel text (and >>>> rodata) >>>> - 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), wasting memory. >>>> + If this is set, kernel memory (text, rodata, etc) 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_RODATA >>>> - bool "Make kernel text and rodata read-only" >>>> - depends on ARM_KERNMEM_PERMS >>>> +config DEBUG_ALIGN_RODATA >>>> + bool "Make rodata strictly non-executable" >>>> + depends on DEBUG_RODATA >>>> default y >>>> help >>>> - If this is set, kernel text and rodata will be made read-only. >>>> This >>>> - is to help catch accidental or malicious attempts to change >>>> the >>>> - kernel's executable code. Additionally splits rodata from >>>> kernel >>>> - text so it can be made explicitly non-executable. This creates >>>> - another section-size padded region, so it can waste more >>>> memory >>>> - space while gaining the read-only protections. >>>> + If this is set, rodata will be made explicitly non-executable. >>>> This >>>> + provides protection on the rare chance that attackers might >>>> find >>>> and >>>> + use ROP gadgets that exist in the rodata section. This adds an >>>> + additional section-aligned split of rodata from kernel text so >>>> it >>>> + can be made explicitly non-executable. This padding may waste >>>> memory >>>> + space to gain this additional protection. >>> >>> >>> >>> I get that you want to make this match arm64 but it's really not >>> intuitive that >>> something with ALIGN_RODATA in the name is actually for setting NX. The >>> purpose >>> of ALIGN_RODATA was also slightly different on arm64 since the RO/NX will >>> still >>> be there, the difference is if the sections are present versus broken >>> down into >>> pages. >> >> >> Well, it seems to have the same effect: without the alignment, a >> portion of rodata may remain executable on arm64. Unless I >> misunderstand? >> > > No, on arm64 everything should always be NX, the difference is part of the > NX > sections may be mapped as pages instead of sections so you take the TLB hit. > It's a trade off of memory vs TLB pressure instead of just security vs TLB. Ah! I understand now. I will clarify the commit and Kconfig text on ARM. I'd still prefer to keep the name, since it's still doing section-alignment of rodata, it's just that without ARM's ALIGN_RODATA, you get an executable rodata section. -Kees -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2015-12-01 02:10 +0100 |
| Message-ID | <qAHC9-2Om-13@gated-at.bofh.it> |
| In reply to | #1280406 |
On 11/30/2015 03:38 PM, Kees Cook wrote:
> Given the choice between making things NX or making things RO, we want
> RO first. As such, redefine CONFIG_DEBUG_RODATA to actually do the bulk
Can you give a citation for why? The thread that inspired it might be
a good link.
> of the ROing (and NXing). In the place of the old CONFIG_DEBUG_RODATA,
> introduce CONFIG_DEBUG_RODATA_ALIGN (after arm64's config that does the
> same thing) to add the additional section alignment for making rodata
> explicitly NX. Also adds human readable names to the sections so I
> could more easily debug my typos, and makes CONFIG_DEBUG_RODATA default
> "y" for CPU_V7.
>
> Results in /sys/kernel/debug/kernel_page_tables for each config state:
>
> # CONFIG_DEBUG_RODATA is not set
> # CONFIG_DEBUG_ALIGN_RODATA is not set
>
> ---[ Kernel Mapping ]---
> 0x80000000-0x80900000 9M RW x SHD
> 0x80900000-0xa0000000 503M RW NX SHD
>
> CONFIG_DEBUG_RODATA=y
> CONFIG_DEBUG_ALIGN_RODATA=y
>
> ---[ Kernel Mapping ]---
> 0x80000000-0x80100000 1M RW NX SHD
> 0x80100000-0x80700000 6M ro x SHD
> 0x80700000-0x80a00000 3M ro NX SHD
> 0x80a00000-0xa0000000 502M RW NX SHD
>
> CONFIG_DEBUG_RODATA=y
> # CONFIG_DEBUG_ALIGN_RODATA is not set
>
> ---[ Kernel Mapping ]---
> 0x80000000-0x80100000 1M RW NX SHD
> 0x80100000-0x80a00000 9M ro x SHD
> 0x80a00000-0xa0000000 502M RW NX SHD
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> arch/arm/kernel/vmlinux.lds.S | 10 +++++-----
> arch/arm/mm/Kconfig | 34 ++++++++++++++++++----------------
> arch/arm/mm/init.c | 18 ++++++++++--------
> 3 files changed, 33 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
> index 8b60fde5ce48..a6e395c53a48 100644
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -8,7 +8,7 @@
> #include <asm/thread_info.h>
> #include <asm/memory.h>
> #include <asm/page.h>
> -#ifdef CONFIG_ARM_KERNMEM_PERMS
> +#ifdef CONFIG_DEBUG_RODATA
> #include <asm/pgtable.h>
> #endif
>
> @@ -94,7 +94,7 @@ SECTIONS
> HEAD_TEXT
> }
>
> -#ifdef CONFIG_ARM_KERNMEM_PERMS
> +#ifdef CONFIG_DEBUG_RODATA
> . = ALIGN(1<<SECTION_SHIFT);
> #endif
>
> @@ -117,7 +117,7 @@ SECTIONS
> ARM_CPU_KEEP(PROC_INFO)
> }
>
> -#ifdef CONFIG_DEBUG_RODATA
> +#ifdef CONFIG_DEBUG_ALIGN_RODATA
> . = ALIGN(1<<SECTION_SHIFT);
> #endif
> RO_DATA(PAGE_SIZE)
> @@ -153,7 +153,7 @@ SECTIONS
> _etext = .; /* End of text and rodata section */
>
> #ifndef CONFIG_XIP_KERNEL
> -# ifdef CONFIG_ARM_KERNMEM_PERMS
> +# ifdef CONFIG_DEBUG_RODATA
> . = ALIGN(1<<SECTION_SHIFT);
> # else
> . = ALIGN(PAGE_SIZE);
> @@ -231,7 +231,7 @@ SECTIONS
> __data_loc = ALIGN(4); /* location in binary */
> . = PAGE_OFFSET + TEXT_OFFSET;
> #else
> -#ifdef CONFIG_ARM_KERNMEM_PERMS
> +#ifdef CONFIG_DEBUG_RODATA
> . = ALIGN(1<<SECTION_SHIFT);
> #else
> . = ALIGN(THREAD_SIZE);
> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index 41218867a9a6..b617084e9520 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -1039,24 +1039,26 @@ config ARCH_SUPPORTS_BIG_ENDIAN
> This option specifies the architecture can support big endian
> operation.
>
> -config ARM_KERNMEM_PERMS
> - bool "Restrict kernel memory permissions"
> +config DEBUG_RODATA
> + bool "Make kernel text and rodata read-only"
> depends on MMU
> + default y if CPU_V7
> help
> - If this is set, kernel memory other than kernel text (and rodata)
> - 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), wasting memory.
> + If this is set, kernel memory (text, rodata, etc) 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_RODATA
> - bool "Make kernel text and rodata read-only"
> - depends on ARM_KERNMEM_PERMS
> +config DEBUG_ALIGN_RODATA
> + bool "Make rodata strictly non-executable"
> + depends on DEBUG_RODATA
> default y
> help
> - If this is set, kernel text and rodata will be made read-only. This
> - is to help catch accidental or malicious attempts to change the
> - kernel's executable code. Additionally splits rodata from kernel
> - text so it can be made explicitly non-executable. This creates
> - another section-size padded region, so it can waste more memory
> - space while gaining the read-only protections.
> + If this is set, rodata will be made explicitly non-executable. This
> + provides protection on the rare chance that attackers might find and
> + use ROP gadgets that exist in the rodata section. This adds an
> + additional section-aligned split of rodata from kernel text so it
> + can be made explicitly non-executable. This padding may waste memory
> + space to gain this additional protection.
I get that you want to make this match arm64 but it's really not intuitive that
something with ALIGN_RODATA in the name is actually for setting NX. The purpose
of ALIGN_RODATA was also slightly different on arm64 since the RO/NX will still
be there, the difference is if the sections are present versus broken down into
pages.
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 8a63b4cdc0f2..e99f65fbcf2b 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -568,8 +568,9 @@ void __init mem_init(void)
> }
> }
>
> -#ifdef CONFIG_ARM_KERNMEM_PERMS
> +#ifdef CONFIG_DEBUG_RODATA
> struct section_perm {
> + const char *name;
> unsigned long start;
> unsigned long end;
> pmdval_t mask;
> @@ -580,6 +581,7 @@ struct section_perm {
> static struct section_perm nx_perms[] = {
> /* Make pages tables, etc before _stext RW (set NX). */
> {
> + .name = "pre-text NX",
> .start = PAGE_OFFSET,
> .end = (unsigned long)_stext,
> .mask = ~PMD_SECT_XN,
> @@ -587,14 +589,16 @@ static struct section_perm nx_perms[] = {
> },
> /* Make init RW (set NX). */
> {
> + .name = "init NX",
> .start = (unsigned long)__init_begin,
> .end = (unsigned long)_sdata,
> .mask = ~PMD_SECT_XN,
> .prot = PMD_SECT_XN,
> },
> -#ifdef CONFIG_DEBUG_RODATA
> +#ifdef CONFIG_DEBUG_ALIGN_RODATA
> /* Make rodata NX (set RO in ro_perms below). */
> {
> + .name = "rodata NX",
> .start = (unsigned long)__start_rodata,
> .end = (unsigned long)__init_begin,
> .mask = ~PMD_SECT_XN,
> @@ -603,10 +607,10 @@ static struct section_perm nx_perms[] = {
> #endif
> };
>
> -#ifdef CONFIG_DEBUG_RODATA
> static struct section_perm ro_perms[] = {
> /* Make kernel code and rodata RX (set RO). */
> {
> + .name = "text/rodata RO",
> .start = (unsigned long)_stext,
> .end = (unsigned long)__init_begin,
> #ifdef CONFIG_ARM_LPAE
> @@ -619,7 +623,6 @@ static struct section_perm ro_perms[] = {
> #endif
> },
> };
> -#endif
>
> /*
> * Updates section permissions only for the current mm (sections are
> @@ -666,7 +669,8 @@ static inline bool arch_has_strict_perms(void)
> for (i = 0; i < ARRAY_SIZE(perms); i++) { \
> if (!IS_ALIGNED(perms[i].start, SECTION_SIZE) || \
> !IS_ALIGNED(perms[i].end, SECTION_SIZE)) { \
> - pr_err("BUG: section %lx-%lx not aligned to %lx\n", \
> + pr_err("BUG: %s section %lx-%lx not aligned to %lx\n", \
> + perms[i].name, \
> perms[i].start, perms[i].end, \
> SECTION_SIZE); \
> continue; \
> @@ -685,7 +689,6 @@ static inline void fix_kernmem_perms(void)
> set_section_perms(nx_perms, prot);
> }
>
> -#ifdef CONFIG_DEBUG_RODATA
> void mark_rodata_ro(void)
> {
> set_section_perms(ro_perms, prot);
> @@ -700,11 +703,10 @@ void set_kernel_text_ro(void)
> {
> set_section_perms(ro_perms, prot);
> }
> -#endif /* CONFIG_DEBUG_RODATA */
>
> #else
> static inline void fix_kernmem_perms(void) { }
> -#endif /* CONFIG_ARM_KERNMEM_PERMS */
> +#endif /* CONFIG_DEBUG_RODATA */
>
> void free_tcmmem(void)
> {
>
Thanks,
Laura
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web