linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] powerpc: fortify support
@ 2017-05-22  1:32 Daniel Axtens
  2017-05-22  1:32 ` [PATCH 1/2] powerpc: Don't fortify prom_init Daniel Axtens
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Daniel Axtens @ 2017-05-22  1:32 UTC (permalink / raw)
  To: kernel-hardening, linuxppc-dev; +Cc: Daniel Axtens

This series provides arch-specific support for Daniel Micay's
fortified string functions on powerpc.

Fortified string functions provied some compile and run time bounds
checking on commonly used string functions.

It's cool - currently it picks up a lot of random things so it will
require some more work across the tree, but hopefully it will
eventually hit mainline. These patches make powerpc ready for when it
does.

They've been tested on pseries.

I think patch 1 should probably go through Kees to line up with the
fortify patch itself, but patch 2 can apply to powerpc as is. But up
to you, mpe.

Regards,
Daniel

Daniel Axtens (2):
  powerpc: Don't fortify prom_init
  powerpc: Make feature-fixup tests fortify-safe

 arch/powerpc/kernel/prom_init.c   |   3 +
 arch/powerpc/lib/feature-fixups.c | 180 +++++++++++++++++++-------------------
 2 files changed, 93 insertions(+), 90 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] powerpc: Don't fortify prom_init
  2017-05-22  1:32 [PATCH 0/2] powerpc: fortify support Daniel Axtens
@ 2017-05-22  1:32 ` Daniel Axtens
  2017-05-22 10:04   ` [kernel-hardening] " Michael Ellerman
  2017-05-22  1:32 ` [PATCH 2/2] powerpc: Make feature-fixup tests fortify-safe Daniel Axtens
       [not found] ` <20170522013607.89E5CAE034@b01ledav005.gho.pok.ibm.com>
  2 siblings, 1 reply; 6+ messages in thread
From: Daniel Axtens @ 2017-05-22  1:32 UTC (permalink / raw)
  To: kernel-hardening, linuxppc-dev; +Cc: Daniel Axtens, Kees Cook, Daniel Micay

prom_init is a bit special; in theory it should be able to be
linked separately to the kernel. To keep this from getting too
complex, the symbols that prom_init.c uses are checked.

Fortification adds symbols, and it gets quite messy as it includes
things like panic(). So just don't fortify prom_init.c for now.

Cc: Kees Cook <keescook@chromium.org>
Cc: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>

---

This will need to go in before the main fortify support, but it
doesn't make any sense in the absence of fortify. I think it would
make most sense for Kees to queue this up with the main fortify patch,
with an Ack from mpe?

---
 arch/powerpc/kernel/prom_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index dd8a04f3053a..613f79f03877 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -15,6 +15,9 @@
 
 #undef DEBUG_PROM
 
+/* we cannot use FORTIFY as it brings in new symbols */
+#define __NO_FORTIFY
+
 #include <stdarg.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] powerpc: Make feature-fixup tests fortify-safe
  2017-05-22  1:32 [PATCH 0/2] powerpc: fortify support Daniel Axtens
  2017-05-22  1:32 ` [PATCH 1/2] powerpc: Don't fortify prom_init Daniel Axtens
@ 2017-05-22  1:32 ` Daniel Axtens
  2017-05-22  4:38   ` Andrew Donnellan
       [not found] ` <20170522013607.89E5CAE034@b01ledav005.gho.pok.ibm.com>
  2 siblings, 1 reply; 6+ messages in thread
From: Daniel Axtens @ 2017-05-22  1:32 UTC (permalink / raw)
  To: kernel-hardening, linuxppc-dev; +Cc: Daniel Axtens, Kees Cook, Daniel Micay

Testing the fortified string functions[1] would cause a kernel
panic on boot in test_feature_fixups() due to a buffer overflow
in memcmp.

This boils down to things like this:

  extern unsigned int ftr_fixup_test1;
  extern unsigned int ftr_fixup_test1_orig;

  check(memcmp(&ftr_fixup_test1, &ftr_fixup_test1_orig, size) == 0);

We know that these are asm labels so it is safe to read up to
'size' bytes at those addresses.

However, because we have passed the address of a single unsigned
int to memcmp, the compiler believes the underlying object is in
fact a single unsigned int. So if size > sizeof(unsigned int),
there will be a panic at runtime.

We can fix this by changing the types: instead of calling the asm
labels unsigned ints, call them unsigned int[]s. Therefore the
size isn't incorrectly determined at compile time and we get a
regular unsafe memcmp and no panic.

[1] http://openwall.com/lists/kernel-hardening/2017/05/09/2

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: Kees Cook <keescook@chromium.org>
Cc: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 arch/powerpc/lib/feature-fixups.c | 180 +++++++++++++++++++-------------------
 1 file changed, 90 insertions(+), 90 deletions(-)

diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index f3917705c686..41cf5ae273cf 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -233,192 +233,192 @@ static long calc_offset(struct fixup_entry *entry, unsigned int *p)
 
 static void test_basic_patching(void)
 {
-	extern unsigned int ftr_fixup_test1;
-	extern unsigned int end_ftr_fixup_test1;
-	extern unsigned int ftr_fixup_test1_orig;
-	extern unsigned int ftr_fixup_test1_expected;
-	int size = &end_ftr_fixup_test1 - &ftr_fixup_test1;
+	extern unsigned int ftr_fixup_test1[];
+	extern unsigned int end_ftr_fixup_test1[];
+	extern unsigned int ftr_fixup_test1_orig[];
+	extern unsigned int ftr_fixup_test1_expected[];
+	int size = end_ftr_fixup_test1 - ftr_fixup_test1;
 
 	fixup.value = fixup.mask = 8;
-	fixup.start_off = calc_offset(&fixup, &ftr_fixup_test1 + 1);
-	fixup.end_off = calc_offset(&fixup, &ftr_fixup_test1 + 2);
+	fixup.start_off = calc_offset(&fixup, ftr_fixup_test1 + 1);
+	fixup.end_off = calc_offset(&fixup, ftr_fixup_test1 + 2);
 	fixup.alt_start_off = fixup.alt_end_off = 0;
 
 	/* Sanity check */
-	check(memcmp(&ftr_fixup_test1, &ftr_fixup_test1_orig, size) == 0);
+	check(memcmp(ftr_fixup_test1, ftr_fixup_test1_orig, size) == 0);
 
 	/* Check we don't patch if the value matches */
 	patch_feature_section(8, &fixup);
-	check(memcmp(&ftr_fixup_test1, &ftr_fixup_test1_orig, size) == 0);
+	check(memcmp(ftr_fixup_test1, ftr_fixup_test1_orig, size) == 0);
 
 	/* Check we do patch if the value doesn't match */
 	patch_feature_section(0, &fixup);
-	check(memcmp(&ftr_fixup_test1, &ftr_fixup_test1_expected, size) == 0);
+	check(memcmp(ftr_fixup_test1, ftr_fixup_test1_expected, size) == 0);
 
 	/* Check we do patch if the mask doesn't match */
-	memcpy(&ftr_fixup_test1, &ftr_fixup_test1_orig, size);
-	check(memcmp(&ftr_fixup_test1, &ftr_fixup_test1_orig, size) == 0);
+	memcpy(ftr_fixup_test1, ftr_fixup_test1_orig, size);
+	check(memcmp(ftr_fixup_test1, ftr_fixup_test1_orig, size) == 0);
 	patch_feature_section(~8, &fixup);
-	check(memcmp(&ftr_fixup_test1, &ftr_fixup_test1_expected, size) == 0);
+	check(memcmp(ftr_fixup_test1, ftr_fixup_test1_expected, size) == 0);
 }
 
 static void test_alternative_patching(void)
 {
-	extern unsigned int ftr_fixup_test2;
-	extern unsigned int end_ftr_fixup_test2;
-	extern unsigned int ftr_fixup_test2_orig;
-	extern unsigned int ftr_fixup_test2_alt;
-	extern unsigned int ftr_fixup_test2_expected;
-	int size = &end_ftr_fixup_test2 - &ftr_fixup_test2;
+	extern unsigned int ftr_fixup_test2[];
+	extern unsigned int end_ftr_fixup_test2[];
+	extern unsigned int ftr_fixup_test2_orig[];
+	extern unsigned int ftr_fixup_test2_alt[];
+	extern unsigned int ftr_fixup_test2_expected[];
+	int size = end_ftr_fixup_test2 - ftr_fixup_test2;
 
 	fixup.value = fixup.mask = 0xF;
-	fixup.start_off = calc_offset(&fixup, &ftr_fixup_test2 + 1);
-	fixup.end_off = calc_offset(&fixup, &ftr_fixup_test2 + 2);
-	fixup.alt_start_off = calc_offset(&fixup, &ftr_fixup_test2_alt);
-	fixup.alt_end_off = calc_offset(&fixup, &ftr_fixup_test2_alt + 1);
+	fixup.start_off = calc_offset(&fixup, ftr_fixup_test2 + 1);
+	fixup.end_off = calc_offset(&fixup, ftr_fixup_test2 + 2);
+	fixup.alt_start_off = calc_offset(&fixup, ftr_fixup_test2_alt);
+	fixup.alt_end_off = calc_offset(&fixup, ftr_fixup_test2_alt + 1);
 
 	/* Sanity check */
-	check(memcmp(&ftr_fixup_test2, &ftr_fixup_test2_orig, size) == 0);
+	check(memcmp(ftr_fixup_test2, ftr_fixup_test2_orig, size) == 0);
 
 	/* Check we don't patch if the value matches */
 	patch_feature_section(0xF, &fixup);
-	check(memcmp(&ftr_fixup_test2, &ftr_fixup_test2_orig, size) == 0);
+	check(memcmp(ftr_fixup_test2, ftr_fixup_test2_orig, size) == 0);
 
 	/* Check we do patch if the value doesn't match */
 	patch_feature_section(0, &fixup);
-	check(memcmp(&ftr_fixup_test2, &ftr_fixup_test2_expected, size) == 0);
+	check(memcmp(ftr_fixup_test2, ftr_fixup_test2_expected, size) == 0);
 
 	/* Check we do patch if the mask doesn't match */
-	memcpy(&ftr_fixup_test2, &ftr_fixup_test2_orig, size);
-	check(memcmp(&ftr_fixup_test2, &ftr_fixup_test2_orig, size) == 0);
+	memcpy(ftr_fixup_test2, ftr_fixup_test2_orig, size);
+	check(memcmp(ftr_fixup_test2, ftr_fixup_test2_orig, size) == 0);
 	patch_feature_section(~0xF, &fixup);
-	check(memcmp(&ftr_fixup_test2, &ftr_fixup_test2_expected, size) == 0);
+	check(memcmp(ftr_fixup_test2, ftr_fixup_test2_expected, size) == 0);
 }
 
 static void test_alternative_case_too_big(void)
 {
-	extern unsigned int ftr_fixup_test3;
-	extern unsigned int end_ftr_fixup_test3;
-	extern unsigned int ftr_fixup_test3_orig;
-	extern unsigned int ftr_fixup_test3_alt;
-	int size = &end_ftr_fixup_test3 - &ftr_fixup_test3;
+	extern unsigned int ftr_fixup_test3[];
+	extern unsigned int end_ftr_fixup_test3[];
+	extern unsigned int ftr_fixup_test3_orig[];
+	extern unsigned int ftr_fixup_test3_alt[];
+	int size = end_ftr_fixup_test3 - ftr_fixup_test3;
 
 	fixup.value = fixup.mask = 0xC;
-	fixup.start_off = calc_offset(&fixup, &ftr_fixup_test3 + 1);
-	fixup.end_off = calc_offset(&fixup, &ftr_fixup_test3 + 2);
-	fixup.alt_start_off = calc_offset(&fixup, &ftr_fixup_test3_alt);
-	fixup.alt_end_off = calc_offset(&fixup, &ftr_fixup_test3_alt + 2);
+	fixup.start_off = calc_offset(&fixup, ftr_fixup_test3 + 1);
+	fixup.end_off = calc_offset(&fixup, ftr_fixup_test3 + 2);
+	fixup.alt_start_off = calc_offset(&fixup, ftr_fixup_test3_alt);
+	fixup.alt_end_off = calc_offset(&fixup, ftr_fixup_test3_alt + 2);
 
 	/* Sanity check */
-	check(memcmp(&ftr_fixup_test3, &ftr_fixup_test3_orig, size) == 0);
+	check(memcmp(ftr_fixup_test3, ftr_fixup_test3_orig, size) == 0);
 
 	/* Expect nothing to be patched, and the error returned to us */
 	check(patch_feature_section(0xF, &fixup) == 1);
-	check(memcmp(&ftr_fixup_test3, &ftr_fixup_test3_orig, size) == 0);
+	check(memcmp(ftr_fixup_test3, ftr_fixup_test3_orig, size) == 0);
 	check(patch_feature_section(0, &fixup) == 1);
-	check(memcmp(&ftr_fixup_test3, &ftr_fixup_test3_orig, size) == 0);
+	check(memcmp(ftr_fixup_test3, ftr_fixup_test3_orig, size) == 0);
 	check(patch_feature_section(~0xF, &fixup) == 1);
-	check(memcmp(&ftr_fixup_test3, &ftr_fixup_test3_orig, size) == 0);
+	check(memcmp(ftr_fixup_test3, ftr_fixup_test3_orig, size) == 0);
 }
 
 static void test_alternative_case_too_small(void)
 {
-	extern unsigned int ftr_fixup_test4;
-	extern unsigned int end_ftr_fixup_test4;
-	extern unsigned int ftr_fixup_test4_orig;
-	extern unsigned int ftr_fixup_test4_alt;
-	extern unsigned int ftr_fixup_test4_expected;
-	int size = &end_ftr_fixup_test4 - &ftr_fixup_test4;
+	extern unsigned int ftr_fixup_test4[];
+	extern unsigned int end_ftr_fixup_test4[];
+	extern unsigned int ftr_fixup_test4_orig[];
+	extern unsigned int ftr_fixup_test4_alt[];
+	extern unsigned int ftr_fixup_test4_expected[];
+	int size = end_ftr_fixup_test4 - ftr_fixup_test4;
 	unsigned long flag;
 
 	/* Check a high-bit flag */
 	flag = 1UL << ((sizeof(unsigned long) - 1) * 8);
 	fixup.value = fixup.mask = flag;
-	fixup.start_off = calc_offset(&fixup, &ftr_fixup_test4 + 1);
-	fixup.end_off = calc_offset(&fixup, &ftr_fixup_test4 + 5);
-	fixup.alt_start_off = calc_offset(&fixup, &ftr_fixup_test4_alt);
-	fixup.alt_end_off = calc_offset(&fixup, &ftr_fixup_test4_alt + 2);
+	fixup.start_off = calc_offset(&fixup, ftr_fixup_test4 + 1);
+	fixup.end_off = calc_offset(&fixup, ftr_fixup_test4 + 5);
+	fixup.alt_start_off = calc_offset(&fixup, ftr_fixup_test4_alt);
+	fixup.alt_end_off = calc_offset(&fixup, ftr_fixup_test4_alt + 2);
 
 	/* Sanity check */
-	check(memcmp(&ftr_fixup_test4, &ftr_fixup_test4_orig, size) == 0);
+	check(memcmp(ftr_fixup_test4, ftr_fixup_test4_orig, size) == 0);
 
 	/* Check we don't patch if the value matches */
 	patch_feature_section(flag, &fixup);
-	check(memcmp(&ftr_fixup_test4, &ftr_fixup_test4_orig, size) == 0);
+	check(memcmp(ftr_fixup_test4, ftr_fixup_test4_orig, size) == 0);
 
 	/* Check we do patch if the value doesn't match */
 	patch_feature_section(0, &fixup);
-	check(memcmp(&ftr_fixup_test4, &ftr_fixup_test4_expected, size) == 0);
+	check(memcmp(ftr_fixup_test4, ftr_fixup_test4_expected, size) == 0);
 
 	/* Check we do patch if the mask doesn't match */
-	memcpy(&ftr_fixup_test4, &ftr_fixup_test4_orig, size);
-	check(memcmp(&ftr_fixup_test4, &ftr_fixup_test4_orig, size) == 0);
+	memcpy(ftr_fixup_test4, ftr_fixup_test4_orig, size);
+	check(memcmp(ftr_fixup_test4, ftr_fixup_test4_orig, size) == 0);
 	patch_feature_section(~flag, &fixup);
-	check(memcmp(&ftr_fixup_test4, &ftr_fixup_test4_expected, size) == 0);
+	check(memcmp(ftr_fixup_test4, ftr_fixup_test4_expected, size) == 0);
 }
 
 static void test_alternative_case_with_branch(void)
 {
-	extern unsigned int ftr_fixup_test5;
-	extern unsigned int end_ftr_fixup_test5;
-	extern unsigned int ftr_fixup_test5_expected;
-	int size = &end_ftr_fixup_test5 - &ftr_fixup_test5;
+	extern unsigned int ftr_fixup_test5[];
+	extern unsigned int end_ftr_fixup_test5[];
+	extern unsigned int ftr_fixup_test5_expected[];
+	int size = end_ftr_fixup_test5 - ftr_fixup_test5;
 
-	check(memcmp(&ftr_fixup_test5, &ftr_fixup_test5_expected, size) == 0);
+	check(memcmp(ftr_fixup_test5, ftr_fixup_test5_expected, size) == 0);
 }
 
 static void test_alternative_case_with_external_branch(void)
 {
-	extern unsigned int ftr_fixup_test6;
-	extern unsigned int end_ftr_fixup_test6;
-	extern unsigned int ftr_fixup_test6_expected;
-	int size = &end_ftr_fixup_test6 - &ftr_fixup_test6;
+	extern unsigned int ftr_fixup_test6[];
+	extern unsigned int end_ftr_fixup_test6[];
+	extern unsigned int ftr_fixup_test6_expected[];
+	int size = end_ftr_fixup_test6 - ftr_fixup_test6;
 
-	check(memcmp(&ftr_fixup_test6, &ftr_fixup_test6_expected, size) == 0);
+	check(memcmp(ftr_fixup_test6, ftr_fixup_test6_expected, size) == 0);
 }
 
 static void test_cpu_macros(void)
 {
-	extern u8 ftr_fixup_test_FTR_macros;
-	extern u8 ftr_fixup_test_FTR_macros_expected;
-	unsigned long size = &ftr_fixup_test_FTR_macros_expected -
-			     &ftr_fixup_test_FTR_macros;
+	extern u8 ftr_fixup_test_FTR_macros[];
+	extern u8 ftr_fixup_test_FTR_macros_expected[];
+	unsigned long size = ftr_fixup_test_FTR_macros_expected -
+			     ftr_fixup_test_FTR_macros;
 
 	/* The fixups have already been done for us during boot */
-	check(memcmp(&ftr_fixup_test_FTR_macros,
-		     &ftr_fixup_test_FTR_macros_expected, size) == 0);
+	check(memcmp(ftr_fixup_test_FTR_macros,
+		     ftr_fixup_test_FTR_macros_expected, size) == 0);
 }
 
 static void test_fw_macros(void)
 {
 #ifdef CONFIG_PPC64
-	extern u8 ftr_fixup_test_FW_FTR_macros;
-	extern u8 ftr_fixup_test_FW_FTR_macros_expected;
-	unsigned long size = &ftr_fixup_test_FW_FTR_macros_expected -
-			     &ftr_fixup_test_FW_FTR_macros;
+	extern u8 ftr_fixup_test_FW_FTR_macros[];
+	extern u8 ftr_fixup_test_FW_FTR_macros_expected[];
+	unsigned long size = ftr_fixup_test_FW_FTR_macros_expected -
+			     ftr_fixup_test_FW_FTR_macros;
 
 	/* The fixups have already been done for us during boot */
-	check(memcmp(&ftr_fixup_test_FW_FTR_macros,
-		     &ftr_fixup_test_FW_FTR_macros_expected, size) == 0);
+	check(memcmp(ftr_fixup_test_FW_FTR_macros,
+		     ftr_fixup_test_FW_FTR_macros_expected, size) == 0);
 #endif
 }
 
 static void test_lwsync_macros(void)
 {
-	extern u8 lwsync_fixup_test;
-	extern u8 end_lwsync_fixup_test;
-	extern u8 lwsync_fixup_test_expected_LWSYNC;
-	extern u8 lwsync_fixup_test_expected_SYNC;
-	unsigned long size = &end_lwsync_fixup_test -
-			     &lwsync_fixup_test;
+	extern u8 lwsync_fixup_test[];
+	extern u8 end_lwsync_fixup_test[];
+	extern u8 lwsync_fixup_test_expected_LWSYNC[];
+	extern u8 lwsync_fixup_test_expected_SYNC[];
+	unsigned long size = end_lwsync_fixup_test -
+			     lwsync_fixup_test;
 
 	/* The fixups have already been done for us during boot */
 	if (cur_cpu_spec->cpu_features & CPU_FTR_LWSYNC) {
-		check(memcmp(&lwsync_fixup_test,
-			     &lwsync_fixup_test_expected_LWSYNC, size) == 0);
+		check(memcmp(lwsync_fixup_test,
+			     lwsync_fixup_test_expected_LWSYNC, size) == 0);
 	} else {
-		check(memcmp(&lwsync_fixup_test,
-			     &lwsync_fixup_test_expected_SYNC, size) == 0);
+		check(memcmp(lwsync_fixup_test,
+			     lwsync_fixup_test_expected_SYNC, size) == 0);
 	}
 }
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] powerpc: Don't fortify prom_init
       [not found] ` <20170522013607.89E5CAE034@b01ledav005.gho.pok.ibm.com>
@ 2017-05-22  4:29   ` Andrew Donnellan
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Donnellan @ 2017-05-22  4:29 UTC (permalink / raw)
  To: Daniel Axtens, kernel-hardening, linuxppc-dev; +Cc: Daniel Micay, Kees Cook

On 22/05/17 11:32, Daniel Axtens wrote:
> prom_init is a bit special; in theory it should be able to be
> linked separately to the kernel. To keep this from getting too
> complex, the symbols that prom_init.c uses are checked.
>
> Fortification adds symbols, and it gets quite messy as it includes
> things like panic(). So just don't fortify prom_init.c for now.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Daniel Micay <danielmicay@gmail.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] powerpc: Make feature-fixup tests fortify-safe
  2017-05-22  1:32 ` [PATCH 2/2] powerpc: Make feature-fixup tests fortify-safe Daniel Axtens
@ 2017-05-22  4:38   ` Andrew Donnellan
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Donnellan @ 2017-05-22  4:38 UTC (permalink / raw)
  To: Daniel Axtens, kernel-hardening, linuxppc-dev; +Cc: Daniel Micay, Kees Cook

On 22/05/17 11:32, Daniel Axtens wrote:
> Testing the fortified string functions[1] would cause a kernel
> panic on boot in test_feature_fixups() due to a buffer overflow
> in memcmp.
>
> This boils down to things like this:
>
>   extern unsigned int ftr_fixup_test1;
>   extern unsigned int ftr_fixup_test1_orig;
>
>   check(memcmp(&ftr_fixup_test1, &ftr_fixup_test1_orig, size) == 0);
>
> We know that these are asm labels so it is safe to read up to
> 'size' bytes at those addresses.
>
> However, because we have passed the address of a single unsigned
> int to memcmp, the compiler believes the underlying object is in
> fact a single unsigned int. So if size > sizeof(unsigned int),
> there will be a panic at runtime.
>
> We can fix this by changing the types: instead of calling the asm
> labels unsigned ints, call them unsigned int[]s. Therefore the
> size isn't incorrectly determined at compile time and we get a
> regular unsafe memcmp and no panic.
>
> [1] http://openwall.com/lists/kernel-hardening/2017/05/09/2
>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Daniel Micay <danielmicay@gmail.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>

With this patch on top of Kees' fortify branch, my Tuleta boots 
powernv_defconfig baremetal with no obvious regressions.

Tested-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

Patch looks sane enough too.

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>


-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [kernel-hardening] [PATCH 1/2] powerpc: Don't fortify prom_init
  2017-05-22  1:32 ` [PATCH 1/2] powerpc: Don't fortify prom_init Daniel Axtens
@ 2017-05-22 10:04   ` Michael Ellerman
  0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2017-05-22 10:04 UTC (permalink / raw)
  To: Daniel Axtens, kernel-hardening, linuxppc-dev
  Cc: Daniel Micay, Kees Cook, Daniel Axtens

Daniel Axtens <dja@axtens.net> writes:
> prom_init is a bit special; in theory it should be able to be
> linked separately to the kernel. To keep this from getting too
> complex, the symbols that prom_init.c uses are checked.
>
> Fortification adds symbols, and it gets quite messy as it includes
> things like panic(). So just don't fortify prom_init.c for now.

Calling panic() at that point is unlikely to work well.

> Cc: Kees Cook <keescook@chromium.org>
> Cc: Daniel Micay <danielmicay@gmail.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
>
> ---
>
> This will need to go in before the main fortify support, but it
> doesn't make any sense in the absence of fortify. I think it would
> make most sense for Kees to queue this up with the main fortify patch,
> with an Ack from mpe?

Yeah that's fine by me.

> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index dd8a04f3053a..613f79f03877 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -15,6 +15,9 @@
>  
>  #undef DEBUG_PROM
>  
> +/* we cannot use FORTIFY as it brings in new symbols */
> +#define __NO_FORTIFY
> +
>  #include <stdarg.h>
>  #include <linux/kernel.h>
>  #include <linux/string.h>

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-05-22 10:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-22  1:32 [PATCH 0/2] powerpc: fortify support Daniel Axtens
2017-05-22  1:32 ` [PATCH 1/2] powerpc: Don't fortify prom_init Daniel Axtens
2017-05-22 10:04   ` [kernel-hardening] " Michael Ellerman
2017-05-22  1:32 ` [PATCH 2/2] powerpc: Make feature-fixup tests fortify-safe Daniel Axtens
2017-05-22  4:38   ` Andrew Donnellan
     [not found] ` <20170522013607.89E5CAE034@b01ledav005.gho.pok.ibm.com>
2017-05-22  4:29   ` [PATCH 1/2] powerpc: Don't fortify prom_init Andrew Donnellan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).