Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312413 > unrolled thread

[PATCH v4 3/8] param: convert some "on"/"off" users to strtobool

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 3/8] param: convert some "on"/"off" users to strtobool Kees Cook <keescook@chromium.org> - 2016-01-19 19:10 +0100
    Re: [kernel-hardening] [PATCH v4 3/8] param: convert some "on"/"off"  users to strtobool Kees Cook <keescook@chromium.org> - 2016-01-27 22:20 +0100
      [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional David Brown <david.brown@linaro.org> - 2016-01-28 01:10 +0100
        Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional Kees Cook <keescook@chromium.org> - 2016-01-28 01:20 +0100
          Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-01-28 09:30 +0100
        Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional Mark Rutland <mark.rutland@arm.com> - 2016-01-28 12:10 +0100
          Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional Kees Cook <keescook@chromium.org> - 2016-01-28 15:10 +0100
            Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional Mark Rutland <mark.rutland@arm.com> - 2016-01-28 16:00 +0100
              Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional Kees Cook <keescook@chromium.org> - 2016-01-28 16:20 +0100
    Re: [kernel-hardening] [PATCH v4 3/8] param: convert some "on"/"off"  users to strtobool David Brown <david.brown@linaro.org> - 2016-01-27 22:20 +0100

#1312413 — [PATCH v4 3/8] param: convert some "on"/"off" users to strtobool

This changes several users of manual "on"/"off" parsing to use strtobool.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/powerpc/kernel/rtasd.c                  | 10 +++-------
 arch/powerpc/platforms/pseries/hotplug-cpu.c | 11 +++--------
 arch/s390/kernel/time.c                      |  8 ++------
 arch/s390/kernel/topology.c                  |  8 +++-----
 arch/x86/kernel/aperture_64.c                | 13 +++----------
 kernel/time/hrtimer.c                        | 11 +++--------
 kernel/time/tick-sched.c                     | 11 +++--------
 7 files changed, 20 insertions(+), 52 deletions(-)

diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
index 5a2c049c1c61..984e67e91ba3 100644
--- a/arch/powerpc/kernel/rtasd.c
+++ b/arch/powerpc/kernel/rtasd.c
@@ -21,6 +21,7 @@
 #include <linux/cpu.h>
 #include <linux/workqueue.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -49,7 +50,7 @@ static unsigned int rtas_error_log_buffer_max;
 static unsigned int event_scan;
 static unsigned int rtas_event_scan_rate;
 
-static int full_rtas_msgs = 0;
+static bool full_rtas_msgs;
 
 /* Stop logging to nvram after first fatal error */
 static int logging_enabled; /* Until we initialize everything,
@@ -592,11 +593,6 @@ __setup("surveillance=", surveillance_setup);
 
 static int __init rtasmsgs_setup(char *str)
 {
-	if (strcmp(str, "on") == 0)
-		full_rtas_msgs = 1;
-	else if (strcmp(str, "off") == 0)
-		full_rtas_msgs = 0;
-
-	return 1;
+	return strtobool(str, &full_rtas_msgs);
 }
 __setup("rtasmsgs=", rtasmsgs_setup);
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 32274f72fe3f..bb333e9fd77a 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -27,6 +27,7 @@
 #include <linux/cpu.h>
 #include <linux/of.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <asm/prom.h>
 #include <asm/rtas.h>
 #include <asm/firmware.h>
@@ -47,20 +48,14 @@ static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
 
 static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
 
-static int cede_offline_enabled __read_mostly = 1;
+static bool cede_offline_enabled __read_mostly = true;
 
 /*
  * Enable/disable cede_offline when available.
  */
 static int __init setup_cede_offline(char *str)
 {
-	if (!strcmp(str, "off"))
-		cede_offline_enabled = 0;
-	else if (!strcmp(str, "on"))
-		cede_offline_enabled = 1;
-	else
-		return 0;
-	return 1;
+	return strtobool(str, &cede_offline_enabled);
 }
 
 __setup("cede_offline=", setup_cede_offline);
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 99f84ac31307..afc7fc9684ba 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -1433,7 +1433,7 @@ device_initcall(etr_init_sysfs);
 /*
  * Server Time Protocol (STP) code.
  */
-static int stp_online;
+static bool stp_online;
 static struct stp_sstpi stp_info;
 static void *stp_page;
 
@@ -1444,11 +1444,7 @@ static struct timer_list stp_timer;
 
 static int __init early_parse_stp(char *p)
 {
-	if (strncmp(p, "off", 3) == 0)
-		stp_online = 0;
-	else if (strncmp(p, "on", 2) == 0)
-		stp_online = 1;
-	return 0;
+	return strtobool(p, &stp_online);
 }
 early_param("stp", early_parse_stp);
 
diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index 40b8102fdadb..10e388216307 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/cpu.h>
 #include <linux/smp.h>
 #include <linux/mm.h>
@@ -37,7 +38,7 @@ static void set_topology_timer(void);
 static void topology_work_fn(struct work_struct *work);
 static struct sysinfo_15_1_x *tl_info;
 
-static int topology_enabled = 1;
+static bool topology_enabled = true;
 static DECLARE_WORK(topology_work, topology_work_fn);
 
 /*
@@ -444,10 +445,7 @@ static const struct cpumask *cpu_book_mask(int cpu)
 
 static int __init early_parse_topology(char *p)
 {
-	if (strncmp(p, "off", 3))
-		return 0;
-	topology_enabled = 0;
-	return 0;
+	return strtobool(p, &topology_enabled);
 }
 early_param("topology", early_parse_topology);
 
diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 6e85f713641d..6608b00a516a 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -20,6 +20,7 @@
 #include <linux/pci_ids.h>
 #include <linux/pci.h>
 #include <linux/bitops.h>
+#include <linux/string.h>
 #include <linux/suspend.h>
 #include <asm/e820.h>
 #include <asm/io.h>
@@ -227,19 +228,11 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
 	return 0;
 }
 
-static int gart_fix_e820 __initdata = 1;
+static bool gart_fix_e820 __initdata = true;
 
 static int __init parse_gart_mem(char *p)
 {
-	if (!p)
-		return -EINVAL;
-
-	if (!strncmp(p, "off", 3))
-		gart_fix_e820 = 0;
-	else if (!strncmp(p, "on", 2))
-		gart_fix_e820 = 1;
-
-	return 0;
+	return strtobool(p, &gart_fix_e820);
 }
 early_param("gart_fix_e820", parse_gart_mem);
 
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 435b8850dd80..40d82fe4d2a5 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -39,6 +39,7 @@
 #include <linux/syscalls.h>
 #include <linux/kallsyms.h>
 #include <linux/interrupt.h>
+#include <linux/string.h>
 #include <linux/tick.h>
 #include <linux/seq_file.h>
 #include <linux/err.h>
@@ -515,7 +516,7 @@ static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
 /*
  * High resolution timer enabled ?
  */
-static int hrtimer_hres_enabled __read_mostly  = 1;
+static bool hrtimer_hres_enabled __read_mostly  = true;
 unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
 EXPORT_SYMBOL_GPL(hrtimer_resolution);
 
@@ -524,13 +525,7 @@ EXPORT_SYMBOL_GPL(hrtimer_resolution);
  */
 static int __init setup_hrtimer_hres(char *str)
 {
-	if (!strcmp(str, "off"))
-		hrtimer_hres_enabled = 0;
-	else if (!strcmp(str, "on"))
-		hrtimer_hres_enabled = 1;
-	else
-		return 0;
-	return 1;
+	return strtobool(str, &hrtimer_hres_enabled);
 }
 
 __setup("highres=", setup_hrtimer_hres);
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 9cc20af58c76..f5ea98490ffa 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -19,6 +19,7 @@
 #include <linux/percpu.h>
 #include <linux/profile.h>
 #include <linux/sched.h>
+#include <linux/string.h>
 #include <linux/module.h>
 #include <linux/irq_work.h>
 #include <linux/posix-timers.h>
@@ -387,20 +388,14 @@ void __init tick_nohz_init(void)
 /*
  * NO HZ enabled ?
  */
-static int tick_nohz_enabled __read_mostly  = 1;
+static bool tick_nohz_enabled __read_mostly  = true;
 unsigned long tick_nohz_active  __read_mostly;
 /*
  * Enable / Disable tickless mode
  */
 static int __init setup_tick_nohz(char *str)
 {
-	if (!strcmp(str, "off"))
-		tick_nohz_enabled = 0;
-	else if (!strcmp(str, "on"))
-		tick_nohz_enabled = 1;
-	else
-		return 0;
-	return 1;
+	return strtobool(str, &tick_nohz_enabled);
 }
 
 __setup("nohz=", setup_tick_nohz);
-- 
2.6.3

[toc] | [next] | [standalone]

#1319849 — Re: [kernel-hardening] [PATCH v4 3/8] param: convert some "on"/"off" users to strtobool

On Wed, Jan 27, 2016 at 1:11 PM, David Brown <david.brown@linaro.org> wrote:
> On Tue, Jan 19, 2016 at 10:08:37AM -0800, Kees Cook wrote:
>
>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>> index 9cc20af58c76..f5ea98490ffa 100644
>> --- a/kernel/time/tick-sched.c
>> +++ b/kernel/time/tick-sched.c
>> @@ -387,20 +388,14 @@ void __init tick_nohz_init(void)
>> /*
>>  * NO HZ enabled ?
>>  */
>> -static int tick_nohz_enabled __read_mostly  = 1;
>> +static bool tick_nohz_enabled __read_mostly  = true;
>> unsigned long tick_nohz_active  __read_mostly;
>> /*
>>  * Enable / Disable tickless mode
>>  */
>
>
> Just discovered this conflicts with a recent patch with
> CONFIG_NO_HZ_COMMON:
>
>         commit 46373a15f65fe862f31c19a484acdf551f2b442f
>         Author: Jean Delvare <jdelvare@suse.de>
>         Date:   Mon Jan 11 17:40:31 2016 +0100
>
>             time: nohz: Expose tick_nohz_enabled
>
> kernel/time/tick-sched.c:390:6: error: conflicting types for
> ‘tick_nohz_enabled’
>         bool tick_nohz_enabled __read_mostly = true;
>              ^
> In file included from kernel/time/tick-internal.h:5:0,
>                 from kernel/time/tick-sched.c:30:
> include/linux/tick.h:101:12: note: previous declaration of
> ‘tick_nohz_enabled’ was here
>         extern int tick_nohz_enabled;
>                    ^

Thanks! Yeah, I noticed this too when rebasing recently.

> Fixing the compilation error, it compiles and boots on arm64, however
> it isn't detecting the write (with the lkdtm test).  I'll continue
> looking into what's preventing this.

I'm going to hope it's something easy like CONFIG_DEBUG_RODATA not being set. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1320159 — [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional

From 2efef8aa0f8f7f6277ffebe4ea6744fc93d54644 Mon Sep 17 00:00:00 2001
From: David Brown <david.brown@linaro.org>
Date: Wed, 27 Jan 2016 13:58:44 -0800

This removes the CONFIG_DEBUG_RODATA option and makes it always
enabled.

Signed-off-by: David Brown <david.brown@linaro.org>
---
v1: This is in the same spirit as the x86 patch, removing allowing
this option to be config selected.  The associated patch series adds a
runtime option for the same thing.  However, it does affect the way
some things are mapped, and could possibly result in either increased
memory usage, or a performance hit (due to TLB misses from 4K pages).

I've tested this on a Hikey 96board (hi6220-hikey.dtb), both with and
without 'rodata=off' on the command line.

 arch/arm64/Kconfig              |  3 +++
 arch/arm64/Kconfig.debug        | 10 ----------
 arch/arm64/kernel/insn.c        |  2 +-
 arch/arm64/kernel/vmlinux.lds.S |  5 +----
 arch/arm64/mm/mmu.c             | 12 ------------
 5 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 8cc6228..ffa617a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -201,6 +201,9 @@ config KERNEL_MODE_NEON
 config FIX_EARLYCON_MEM
 	def_bool y
 
+config DEBUG_RODATA
+	def_bool y
+
 config PGTABLE_LEVELS
 	int
 	default 2 if ARM64_16K_PAGES && ARM64_VA_BITS_36
diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index e13c4bf..db994ec 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -48,16 +48,6 @@ config DEBUG_SET_MODULE_RONX
           against certain classes of kernel exploits.
           If in doubt, say "N".
 
-config DEBUG_RODATA
-	bool "Make kernel text and rodata read-only"
-	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.
-
-          If in doubt, say Y
-
 config DEBUG_ALIGN_RODATA
 	depends on DEBUG_RODATA && ARM64_4K_PAGES
 	bool "Align linker sections up to SECTION_SIZE"
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 7371455..a04bdef 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -95,7 +95,7 @@ static void __kprobes *patch_map(void *addr, int fixmap)
 
 	if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX))
 		page = vmalloc_to_page(addr);
-	else if (!module && IS_ENABLED(CONFIG_DEBUG_RODATA))
+	else if (!module)
 		page = virt_to_page(addr);
 	else
 		return addr;
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index e3928f5..f80903c 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -65,12 +65,9 @@ PECOFF_FILE_ALIGNMENT = 0x200;
 #if defined(CONFIG_DEBUG_ALIGN_RODATA)
 #define ALIGN_DEBUG_RO			. = ALIGN(1<<SECTION_SHIFT);
 #define ALIGN_DEBUG_RO_MIN(min)		ALIGN_DEBUG_RO
-#elif defined(CONFIG_DEBUG_RODATA)
+#else
 #define ALIGN_DEBUG_RO			. = ALIGN(1<<PAGE_SHIFT);
 #define ALIGN_DEBUG_RO_MIN(min)		ALIGN_DEBUG_RO
-#else
-#define ALIGN_DEBUG_RO
-#define ALIGN_DEBUG_RO_MIN(min)		. = ALIGN(min);
 #endif
 
 SECTIONS
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 58faeaa..3b411b7 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -313,7 +313,6 @@ static void create_mapping_late(phys_addr_t phys, unsigned long virt,
 				phys, virt, size, prot, late_alloc);
 }
 
-#ifdef CONFIG_DEBUG_RODATA
 static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
 {
 	/*
@@ -347,13 +346,6 @@ static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
 	}
 
 }
-#else
-static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
-{
-	create_mapping(start, __phys_to_virt(start), end - start,
-			PAGE_KERNEL_EXEC);
-}
-#endif
 
 static void __init map_mem(void)
 {
@@ -410,7 +402,6 @@ static void __init map_mem(void)
 
 static void __init fixup_executable(void)
 {
-#ifdef CONFIG_DEBUG_RODATA
 	/* now that we are actually fully mapped, make the start/end more fine grained */
 	if (!IS_ALIGNED((unsigned long)_stext, SWAPPER_BLOCK_SIZE)) {
 		unsigned long aligned_start = round_down(__pa(_stext),
@@ -428,10 +419,8 @@ static void __init fixup_executable(void)
 				aligned_end - __pa(__init_end),
 				PAGE_KERNEL);
 	}
-#endif
 }
 
-#ifdef CONFIG_DEBUG_RODATA
 void mark_rodata_ro(void)
 {
 	create_mapping_late(__pa(_stext), (unsigned long)_stext,
@@ -439,7 +428,6 @@ void mark_rodata_ro(void)
 				PAGE_KERNEL_ROX);
 
 }
-#endif
 
 void fixup_init(void)
 {
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1320162 — Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional

On Wed, Jan 27, 2016 at 4:09 PM, David Brown <david.brown@linaro.org> wrote:
> From 2efef8aa0f8f7f6277ffebe4ea6744fc93d54644 Mon Sep 17 00:00:00 2001
> From: David Brown <david.brown@linaro.org>
> Date: Wed, 27 Jan 2016 13:58:44 -0800
>
> This removes the CONFIG_DEBUG_RODATA option and makes it always
> enabled.
>
> Signed-off-by: David Brown <david.brown@linaro.org>

I'm all for this!

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

-Kees

> ---
> v1: This is in the same spirit as the x86 patch, removing allowing
> this option to be config selected.  The associated patch series adds a
> runtime option for the same thing.  However, it does affect the way
> some things are mapped, and could possibly result in either increased
> memory usage, or a performance hit (due to TLB misses from 4K pages).
>
> I've tested this on a Hikey 96board (hi6220-hikey.dtb), both with and
> without 'rodata=off' on the command line.
>
> arch/arm64/Kconfig              |  3 +++
> arch/arm64/Kconfig.debug        | 10 ----------
> arch/arm64/kernel/insn.c        |  2 +-
> arch/arm64/kernel/vmlinux.lds.S |  5 +----
> arch/arm64/mm/mmu.c             | 12 ------------
> 5 files changed, 5 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 8cc6228..ffa617a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -201,6 +201,9 @@ config KERNEL_MODE_NEON
> config FIX_EARLYCON_MEM
>         def_bool y
>
> +config DEBUG_RODATA
> +       def_bool y
> +
> config PGTABLE_LEVELS
>         int
>         default 2 if ARM64_16K_PAGES && ARM64_VA_BITS_36
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index e13c4bf..db994ec 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -48,16 +48,6 @@ config DEBUG_SET_MODULE_RONX
>           against certain classes of kernel exploits.
>           If in doubt, say "N".
>
> -config DEBUG_RODATA
> -       bool "Make kernel text and rodata read-only"
> -       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.
> -
> -          If in doubt, say Y
> -
> config DEBUG_ALIGN_RODATA
>         depends on DEBUG_RODATA && ARM64_4K_PAGES
>         bool "Align linker sections up to SECTION_SIZE"
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 7371455..a04bdef 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -95,7 +95,7 @@ static void __kprobes *patch_map(void *addr, int fixmap)
>
>         if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX))
>                 page = vmalloc_to_page(addr);
> -       else if (!module && IS_ENABLED(CONFIG_DEBUG_RODATA))
> +       else if (!module)
>                 page = virt_to_page(addr);
>         else
>                 return addr;
> diff --git a/arch/arm64/kernel/vmlinux.lds.S
> b/arch/arm64/kernel/vmlinux.lds.S
> index e3928f5..f80903c 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -65,12 +65,9 @@ PECOFF_FILE_ALIGNMENT = 0x200;
> #if defined(CONFIG_DEBUG_ALIGN_RODATA)
> #define ALIGN_DEBUG_RO                  . = ALIGN(1<<SECTION_SHIFT);
> #define ALIGN_DEBUG_RO_MIN(min)         ALIGN_DEBUG_RO
> -#elif defined(CONFIG_DEBUG_RODATA)
> +#else
> #define ALIGN_DEBUG_RO                  . = ALIGN(1<<PAGE_SHIFT);
> #define ALIGN_DEBUG_RO_MIN(min)         ALIGN_DEBUG_RO
> -#else
> -#define ALIGN_DEBUG_RO
> -#define ALIGN_DEBUG_RO_MIN(min)                . = ALIGN(min);
> #endif
>
> SECTIONS
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 58faeaa..3b411b7 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -313,7 +313,6 @@ static void create_mapping_late(phys_addr_t phys,
> unsigned long virt,
>                                 phys, virt, size, prot, late_alloc);
> }
>
> -#ifdef CONFIG_DEBUG_RODATA
> static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
> {
>         /*
> @@ -347,13 +346,6 @@ static void __init __map_memblock(phys_addr_t start,
> phys_addr_t end)
>         }
>
> }
> -#else
> -static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
> -{
> -       create_mapping(start, __phys_to_virt(start), end - start,
> -                       PAGE_KERNEL_EXEC);
> -}
> -#endif
>
> static void __init map_mem(void)
> {
> @@ -410,7 +402,6 @@ static void __init map_mem(void)
>
> static void __init fixup_executable(void)
> {
> -#ifdef CONFIG_DEBUG_RODATA
>         /* now that we are actually fully mapped, make the start/end more
> fine grained */
>         if (!IS_ALIGNED((unsigned long)_stext, SWAPPER_BLOCK_SIZE)) {
>                 unsigned long aligned_start = round_down(__pa(_stext),
> @@ -428,10 +419,8 @@ static void __init fixup_executable(void)
>                                 aligned_end - __pa(__init_end),
>                                 PAGE_KERNEL);
>         }
> -#endif
> }
>
> -#ifdef CONFIG_DEBUG_RODATA
> void mark_rodata_ro(void)
> {
>         create_mapping_late(__pa(_stext), (unsigned long)_stext,
> @@ -439,7 +428,6 @@ void mark_rodata_ro(void)
>                                 PAGE_KERNEL_ROX);
>
> }
> -#endif
>
> void fixup_init(void)
> {
> --
> 2.7.0
>



-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1320391 — Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional

On 28 January 2016 at 01:14, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jan 27, 2016 at 4:09 PM, David Brown <david.brown@linaro.org> wrote:
>> From 2efef8aa0f8f7f6277ffebe4ea6744fc93d54644 Mon Sep 17 00:00:00 2001
>> From: David Brown <david.brown@linaro.org>
>> Date: Wed, 27 Jan 2016 13:58:44 -0800
>>
>> This removes the CONFIG_DEBUG_RODATA option and makes it always
>> enabled.
>>
>> Signed-off-by: David Brown <david.brown@linaro.org>
>
> I'm all for this!
>

I agree that this is probably a good idea, but please note that Mark
Rutland's pagetable rework patches targeted for v4.6 make significant
changes in this area, so you're probably better off building on top of
those.

-- 
Ard.


> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
>> ---
>> v1: This is in the same spirit as the x86 patch, removing allowing
>> this option to be config selected.  The associated patch series adds a
>> runtime option for the same thing.  However, it does affect the way
>> some things are mapped, and could possibly result in either increased
>> memory usage, or a performance hit (due to TLB misses from 4K pages).
>>
>> I've tested this on a Hikey 96board (hi6220-hikey.dtb), both with and
>> without 'rodata=off' on the command line.
>>
>> arch/arm64/Kconfig              |  3 +++
>> arch/arm64/Kconfig.debug        | 10 ----------
>> arch/arm64/kernel/insn.c        |  2 +-
>> arch/arm64/kernel/vmlinux.lds.S |  5 +----
>> arch/arm64/mm/mmu.c             | 12 ------------
>> 5 files changed, 5 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 8cc6228..ffa617a 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -201,6 +201,9 @@ config KERNEL_MODE_NEON
>> config FIX_EARLYCON_MEM
>>         def_bool y
>>
>> +config DEBUG_RODATA
>> +       def_bool y
>> +
>> config PGTABLE_LEVELS
>>         int
>>         default 2 if ARM64_16K_PAGES && ARM64_VA_BITS_36
>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>> index e13c4bf..db994ec 100644
>> --- a/arch/arm64/Kconfig.debug
>> +++ b/arch/arm64/Kconfig.debug
>> @@ -48,16 +48,6 @@ config DEBUG_SET_MODULE_RONX
>>           against certain classes of kernel exploits.
>>           If in doubt, say "N".
>>
>> -config DEBUG_RODATA
>> -       bool "Make kernel text and rodata read-only"
>> -       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.
>> -
>> -          If in doubt, say Y
>> -
>> config DEBUG_ALIGN_RODATA
>>         depends on DEBUG_RODATA && ARM64_4K_PAGES
>>         bool "Align linker sections up to SECTION_SIZE"
>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>> index 7371455..a04bdef 100644
>> --- a/arch/arm64/kernel/insn.c
>> +++ b/arch/arm64/kernel/insn.c
>> @@ -95,7 +95,7 @@ static void __kprobes *patch_map(void *addr, int fixmap)
>>
>>         if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX))
>>                 page = vmalloc_to_page(addr);
>> -       else if (!module && IS_ENABLED(CONFIG_DEBUG_RODATA))
>> +       else if (!module)
>>                 page = virt_to_page(addr);
>>         else
>>                 return addr;
>> diff --git a/arch/arm64/kernel/vmlinux.lds.S
>> b/arch/arm64/kernel/vmlinux.lds.S
>> index e3928f5..f80903c 100644
>> --- a/arch/arm64/kernel/vmlinux.lds.S
>> +++ b/arch/arm64/kernel/vmlinux.lds.S
>> @@ -65,12 +65,9 @@ PECOFF_FILE_ALIGNMENT = 0x200;
>> #if defined(CONFIG_DEBUG_ALIGN_RODATA)
>> #define ALIGN_DEBUG_RO                  . = ALIGN(1<<SECTION_SHIFT);
>> #define ALIGN_DEBUG_RO_MIN(min)         ALIGN_DEBUG_RO
>> -#elif defined(CONFIG_DEBUG_RODATA)
>> +#else
>> #define ALIGN_DEBUG_RO                  . = ALIGN(1<<PAGE_SHIFT);
>> #define ALIGN_DEBUG_RO_MIN(min)         ALIGN_DEBUG_RO
>> -#else
>> -#define ALIGN_DEBUG_RO
>> -#define ALIGN_DEBUG_RO_MIN(min)                . = ALIGN(min);
>> #endif
>>
>> SECTIONS
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 58faeaa..3b411b7 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -313,7 +313,6 @@ static void create_mapping_late(phys_addr_t phys,
>> unsigned long virt,
>>                                 phys, virt, size, prot, late_alloc);
>> }
>>
>> -#ifdef CONFIG_DEBUG_RODATA
>> static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
>> {
>>         /*
>> @@ -347,13 +346,6 @@ static void __init __map_memblock(phys_addr_t start,
>> phys_addr_t end)
>>         }
>>
>> }
>> -#else
>> -static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
>> -{
>> -       create_mapping(start, __phys_to_virt(start), end - start,
>> -                       PAGE_KERNEL_EXEC);
>> -}
>> -#endif
>>
>> static void __init map_mem(void)
>> {
>> @@ -410,7 +402,6 @@ static void __init map_mem(void)
>>
>> static void __init fixup_executable(void)
>> {
>> -#ifdef CONFIG_DEBUG_RODATA
>>         /* now that we are actually fully mapped, make the start/end more
>> fine grained */
>>         if (!IS_ALIGNED((unsigned long)_stext, SWAPPER_BLOCK_SIZE)) {
>>                 unsigned long aligned_start = round_down(__pa(_stext),
>> @@ -428,10 +419,8 @@ static void __init fixup_executable(void)
>>                                 aligned_end - __pa(__init_end),
>>                                 PAGE_KERNEL);
>>         }
>> -#endif
>> }
>>
>> -#ifdef CONFIG_DEBUG_RODATA
>> void mark_rodata_ro(void)
>> {
>>         create_mapping_late(__pa(_stext), (unsigned long)_stext,
>> @@ -439,7 +428,6 @@ void mark_rodata_ro(void)
>>                                 PAGE_KERNEL_ROX);
>>
>> }
>> -#endif
>>
>> void fixup_init(void)
>> {
>> --
>> 2.7.0
>>
>
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1320559 — Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional

Hi,

On Wed, Jan 27, 2016 at 05:09:06PM -0700, David Brown wrote:
> From 2efef8aa0f8f7f6277ffebe4ea6744fc93d54644 Mon Sep 17 00:00:00 2001
> From: David Brown <david.brown@linaro.org>
> Date: Wed, 27 Jan 2016 13:58:44 -0800
> 
> This removes the CONFIG_DEBUG_RODATA option and makes it always
> enabled.
> 
> Signed-off-by: David Brown <david.brown@linaro.org>

As Ard notes, my pagetable rework series [1] changes this code quite
significantly, and this will need to be rebased atop of that (and
possibly Ard's kASLR changes [2]).

I certainly want to always have the kernel text RO. I was waiting for
those two series to settle first.

> ---
> v1: This is in the same spirit as the x86 patch, removing allowing
> this option to be config selected.  The associated patch series adds a
> runtime option for the same thing.  However, it does affect the way
> some things are mapped, and could possibly result in either increased
> memory usage, or a performance hit (due to TLB misses from 4K pages).

With my series [1] the text/rodata "chunk" of the kernel is always
mapped with page-granular boundaries (using sections internally).
Previously we always carved out the init area, so the kernel mapping was
always split.

Atop of my series this change should not increase memory usage or TLB
pressure given that it should only change the permissions.

One thing I would like to do is to avoid the need for fixup_executable
entirely, by mapping the kernel text RO from the outset. However, that
requires rework of the alternatives patching (to use a temporary RW
alias), and I haven't had the time to look into that yet.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/397095.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/402066.html

> I've tested this on a Hikey 96board (hi6220-hikey.dtb), both with and
> without 'rodata=off' on the command line.
> 
> arch/arm64/Kconfig              |  3 +++
> arch/arm64/Kconfig.debug        | 10 ----------
> arch/arm64/kernel/insn.c        |  2 +-
> arch/arm64/kernel/vmlinux.lds.S |  5 +----
> arch/arm64/mm/mmu.c             | 12 ------------
> 5 files changed, 5 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 8cc6228..ffa617a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -201,6 +201,9 @@ config KERNEL_MODE_NEON
> config FIX_EARLYCON_MEM
> 	def_bool y
> 
> +config DEBUG_RODATA
> +	def_bool y
> +
> config PGTABLE_LEVELS
> 	int
> 	default 2 if ARM64_16K_PAGES && ARM64_VA_BITS_36
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index e13c4bf..db994ec 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -48,16 +48,6 @@ config DEBUG_SET_MODULE_RONX
>           against certain classes of kernel exploits.
>           If in doubt, say "N".
> 
> -config DEBUG_RODATA
> -	bool "Make kernel text and rodata read-only"
> -	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.
> -
> -          If in doubt, say Y
> -
> config DEBUG_ALIGN_RODATA
> 	depends on DEBUG_RODATA && ARM64_4K_PAGES
> 	bool "Align linker sections up to SECTION_SIZE"
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 7371455..a04bdef 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -95,7 +95,7 @@ static void __kprobes *patch_map(void *addr, int fixmap)
> 
> 	if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX))
> 		page = vmalloc_to_page(addr);
> -	else if (!module && IS_ENABLED(CONFIG_DEBUG_RODATA))
> +	else if (!module)
> 		page = virt_to_page(addr);
> 	else
> 		return addr;
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index e3928f5..f80903c 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -65,12 +65,9 @@ PECOFF_FILE_ALIGNMENT = 0x200;
> #if defined(CONFIG_DEBUG_ALIGN_RODATA)
> #define ALIGN_DEBUG_RO			. = ALIGN(1<<SECTION_SHIFT);
> #define ALIGN_DEBUG_RO_MIN(min)		ALIGN_DEBUG_RO
> -#elif defined(CONFIG_DEBUG_RODATA)
> +#else
> #define ALIGN_DEBUG_RO			. = ALIGN(1<<PAGE_SHIFT);
> #define ALIGN_DEBUG_RO_MIN(min)		ALIGN_DEBUG_RO
> -#else
> -#define ALIGN_DEBUG_RO
> -#define ALIGN_DEBUG_RO_MIN(min)		. = ALIGN(min);
> #endif
> 
> SECTIONS
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 58faeaa..3b411b7 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -313,7 +313,6 @@ static void create_mapping_late(phys_addr_t phys, unsigned long virt,
> 				phys, virt, size, prot, late_alloc);
> }
> 
> -#ifdef CONFIG_DEBUG_RODATA
> static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
> {
> 	/*
> @@ -347,13 +346,6 @@ static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
> 	}
> 
> }
> -#else
> -static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
> -{
> -	create_mapping(start, __phys_to_virt(start), end - start,
> -			PAGE_KERNEL_EXEC);
> -}
> -#endif
> 
> static void __init map_mem(void)
> {
> @@ -410,7 +402,6 @@ static void __init map_mem(void)
> 
> static void __init fixup_executable(void)
> {
> -#ifdef CONFIG_DEBUG_RODATA
> 	/* now that we are actually fully mapped, make the start/end more fine grained */
> 	if (!IS_ALIGNED((unsigned long)_stext, SWAPPER_BLOCK_SIZE)) {
> 		unsigned long aligned_start = round_down(__pa(_stext),
> @@ -428,10 +419,8 @@ static void __init fixup_executable(void)
> 				aligned_end - __pa(__init_end),
> 				PAGE_KERNEL);
> 	}
> -#endif
> }
> 
> -#ifdef CONFIG_DEBUG_RODATA
> void mark_rodata_ro(void)
> {
> 	create_mapping_late(__pa(_stext), (unsigned long)_stext,
> @@ -439,7 +428,6 @@ void mark_rodata_ro(void)
> 				PAGE_KERNEL_ROX);
> 
> }
> -#endif
> 
> void fixup_init(void)
> {
> -- 
> 2.7.0
> 

[toc] | [prev] | [next] | [standalone]

#1320705 — Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional

On Thu, Jan 28, 2016 at 3:06 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> One thing I would like to do is to avoid the need for fixup_executable
> entirely, by mapping the kernel text RO from the outset. However, that
> requires rework of the alternatives patching (to use a temporary RW
> alias), and I haven't had the time to look into that yet.

This makes perfect sense for the rodata section, but the (future)
postinit_rodata section we'll still want to mark RO after init
finishes. x86 and ARM cheat by marking both RO after init, and they
don't have to pad sections. parisc will need to solve this too.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1320749 — Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional

On Thu, Jan 28, 2016 at 06:06:53AM -0800, Kees Cook wrote:
> On Thu, Jan 28, 2016 at 3:06 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > One thing I would like to do is to avoid the need for fixup_executable
> > entirely, by mapping the kernel text RO from the outset. However, that
> > requires rework of the alternatives patching (to use a temporary RW
> > alias), and I haven't had the time to look into that yet.
> 
> This makes perfect sense for the rodata section, but the (future)
> postinit_rodata section we'll still want to mark RO after init
> finishes. x86 and ARM cheat by marking both RO after init, and they
> don't have to pad sections. parisc will need to solve this too.

Depending on how many postinit_rodata variables there are, we might be
able to drop those in .rodata, have them RO always, and initialise them
via a temporary RW alias (e.g. something in the vmalloc area).

The only requirement for that is that we use a helper to initialise any
__postinit_ro variables via a temporary RW alias, e.g.

#define SET_POST_INIT_RO(ptr, v) ({ 		\\
	typeof(ptr) __ptr_rw = (ptr)		\\
	BUG_ON(initcalls_done);			\\
	__ptr_rw = create_rw_alias(__ptr);	\\
	__ptr_rw = v;				\\
	destroy_rw_alias(__ptr_rw);		\\
})

...

__postinit_ro void *thing;

void __init some_init_func(void) {
	void *__thing = some_ranodomized_allocator();
	SET_POSTINIT_RO(thing, thing);
}


Mark.

[toc] | [prev] | [next] | [standalone]

#1320775 — Re: [PATCH] arm64: make CONFIG_DEBUG_RODATA non-optional

On Thu, Jan 28, 2016 at 6:59 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jan 28, 2016 at 06:06:53AM -0800, Kees Cook wrote:
>> On Thu, Jan 28, 2016 at 3:06 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > One thing I would like to do is to avoid the need for fixup_executable
>> > entirely, by mapping the kernel text RO from the outset. However, that
>> > requires rework of the alternatives patching (to use a temporary RW
>> > alias), and I haven't had the time to look into that yet.
>>
>> This makes perfect sense for the rodata section, but the (future)
>> postinit_rodata section we'll still want to mark RO after init
>> finishes. x86 and ARM cheat y marking both RO after init, and they
>> don't have to pad sections. parisc will need to solve this too.
>
> Depending on how many postinit_rodata variables there are, we might be
> able to drop those in .rodata, have them RO always, and initialise them
> via a temporary RW alias (e.g. something in the vmalloc area).
>
> The only requirement for that is that we use a helper to initialise any
> __postinit_ro variables via a temporary RW alias, e.g.
>
> #define SET_POST_INIT_RO(ptr, v) ({             \\
>         typeof(ptr) __ptr_rw = (ptr)            \\
>         BUG_ON(initcalls_done);                 \\
>         __ptr_rw = create_rw_alias(__ptr);      \\
>         __ptr_rw = v;                           \\
>         destroy_rw_alias(__ptr_rw);             \\
> })
>
> ...
>
> __postinit_ro void *thing;
>
> void __init some_init_func(void) {
>         void *__thing = some_ranodomized_allocator();
>         SET_POSTINIT_RO(thing, thing);
> }

Well, that certainly would make their usage explicit, but I'd really
like to avoid that, especially in the face of trying to const-ify as
much of the kernel as possible to reduce attack surface. I don't want
to have to both mark the variable and its writes, since that would
make the constification gcc plugin a bit more complex.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1319878 — Re: [kernel-hardening] [PATCH v4 3/8] param: convert some "on"/"off" users to strtobool

On Tue, Jan 19, 2016 at 10:08:37AM -0800, Kees Cook wrote:

>diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>index 9cc20af58c76..f5ea98490ffa 100644
>--- a/kernel/time/tick-sched.c
>+++ b/kernel/time/tick-sched.c
>@@ -387,20 +388,14 @@ void __init tick_nohz_init(void)
> /*
>  * NO HZ enabled ?
>  */
>-static int tick_nohz_enabled __read_mostly  = 1;
>+static bool tick_nohz_enabled __read_mostly  = true;
> unsigned long tick_nohz_active  __read_mostly;
> /*
>  * Enable / Disable tickless mode
>  */

Just discovered this conflicts with a recent patch with
CONFIG_NO_HZ_COMMON:

	commit 46373a15f65fe862f31c19a484acdf551f2b442f
	Author: Jean Delvare <jdelvare@suse.de>
	Date:   Mon Jan 11 17:40:31 2016 +0100

	    time: nohz: Expose tick_nohz_enabled

kernel/time/tick-sched.c:390:6: error: conflicting types for ‘tick_nohz_enabled’
	bool tick_nohz_enabled __read_mostly = true;
	     ^
In file included from kernel/time/tick-internal.h:5:0,
                 from kernel/time/tick-sched.c:30:
include/linux/tick.h:101:12: note: previous declaration of ‘tick_nohz_enabled’ was here
	extern int tick_nohz_enabled;
	           ^

Fixing the compilation error, it compiles and boots on arm64, however
it isn't detecting the write (with the lkdtm test).  I'll continue
looking into what's preventing this.

David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web