Re: [PATCH 2/2] powerpc: Make feature-fixup tests fortify-safe

[Andrew Donnellan]

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(_fixup_test1, _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 
Cc: Kees Cook 
Cc: Daniel Micay 
Signed-off-by: Daniel Axtens 


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


Tested-by: Andrew Donnellan 

Patch looks sane enough too.

Reviewed-by: Andrew Donnellan 


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

[PATCH 2/2] powerpc: Make feature-fixup tests fortify-safe

[Daniel Axtens]

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(_fixup_test1, _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 
Cc: Kees Cook 
Cc: Daniel Micay 
Signed-off-by: Daniel Axtens 
---
 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 = _ftr_fixup_test1 - _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_test1 + 1);
-   fixup.end_off = calc_offset(, _fixup_test1 + 2);
+   fixup.start_off = calc_offset(, ftr_fixup_test1 + 1);
+   fixup.end_off = calc_offset(, ftr_fixup_test1 + 2);
fixup.alt_start_off = fixup.alt_end_off = 0;
 
/* Sanity check */
-   check(memcmp(_fixup_test1, _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, );
-   check(memcmp(_fixup_test1, _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, );
-   check(memcmp(_fixup_test1, _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(_fixup_test1, _fixup_test1_orig, size);
-   check(memcmp(_fixup_test1, _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, );
-   check(memcmp(_fixup_test1, _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 = _ftr_fixup_test2 - _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_test2 + 1);
-   fixup.end_off = calc_offset(, _fixup_test2 + 2);
-   fixup.alt_start_off = calc_offset(, _fixup_test2_alt);
-   fixup.alt_end_off = calc_offset(, _fixup_test2_alt + 1);
+   fixup.start_off = calc_offset(, ftr_fixup_test2 + 1);
+   fixup.end_off = calc_offset(, ftr_fixup_test2 + 2);
+   fixup.alt_start_off = calc_offset(, ftr_fixup_test2_alt);
+   fixup.alt_end_off = calc_offset(, ftr_fixup_test2_alt + 1);
 
/* Sanity check */
-   check(memcmp(_fixup_test2, _fixup_test2_orig, size) == 0);
+   check(memcmp(ftr_fixup_test2, ftr_fixup_test2_orig, size) == 0);
 
/* Check we don't patch