diff mbox

add the option of fortified string.h functions

Message ID 87pofjqlj3.fsf@possimpible.ozlabs.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Axtens May 8, 2017, 5:57 p.m. UTC
Hi Daniel and ppc people,

(ppc people: this does some compile and run time bounds checking on
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.)

I've tested this on ppc with pseries_le_defconfig.

I needed a couple of the fixes from github
(https://github.com/thestinger/linux-hardened/commits/4.11) in order to
build, specifically
https://github.com/thestinger/linux-hardened/commit/c65d6a6f309b06703584a23ac2b2bda4bb363143
https://github.com/thestinger/linux-hardened/commit/adcec4756574a8c7f7cb5b6fa51ebeaeeae71aae

Once those were added, I needed to disable fortification in prom_init.c,
as we apparently can't have new symbols there. (I don't understand that
file so I haven't dug into it.)

We also have problems with the feature fixup tests leading to a panic on
boot. It relates to getting what I think are asm labels(?) and how we
address them. I have just disabled fortify here for now; I think the
code could be rewritten to take the labels as unsigned char *, but I
haven't dug into it.

With the following fixups, I can boot a LE buildroot initrd (per
https://github.com/linuxppc/linux/wiki/Booting-with-Qemu). Sadly I don't
have access to real hardware any more, so I can't say anything more than
that. (ajd - perhaps relevant to your interests?)

Regards,
Daniel

From 33db928b21e6bcb78f93b7883b423282d65af609 Mon Sep 17 00:00:00 2001
From: Daniel Axtens <dja@axtens.net>
Date: Tue, 9 May 2017 03:15:05 +1000
Subject: [PATCH] powerpc fixes for fortify

Signed-off-by: Daniel Axtens <daniel.axtens@canonical.com>
---
 arch/powerpc/kernel/prom_init.c   | 3 +++
 arch/powerpc/lib/feature-fixups.c | 6 ++++++
 2 files changed, 9 insertions(+)

Comments

Daniel Micay May 8, 2017, 8:50 p.m. UTC | #1
On Tue, 2017-05-09 at 03:57 +1000, Daniel Axtens wrote:
> Hi Daniel and ppc people,
> 
> (ppc people: this does some compile and run time bounds checking on
> 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.)
> 
> I've tested this on ppc with pseries_le_defconfig.
> 
> I needed a couple of the fixes from github
> (https://github.com/thestinger/linux-hardened/commits/4.11) in order
> to
> build, specifically
> https://github.com/thestinger/linux-hardened/commit/c65d6a6f309b067035
> 84a23ac2b2bda4bb363143
> https://github.com/thestinger/linux-
> hardened/commit/adcec4756574a8c7f7cb5b6fa51ebeaeeae71aae

By the way, Kees is working on landing these upstream as proper fixes
rather than the workarounds I did there. In most cases, it's a matter of
migrating from memcpy past the end of a constant string to strncpy (to
make sure that the destination is still fully filled but with zeroes
instead of arbitrary junk from rodata vs strcpy/strlcpy which won't
cover that). A few of them are a bit weirder though. I haven't seen any
false positives yet though, due to sticking to __builtin_object_size(p,
0) for now.

> Once those were added, I needed to disable fortification in
> prom_init.c,
> as we apparently can't have new symbols there. (I don't understand
> that
> file so I haven't dug into it.)
> 
> We also have problems with the feature fixup tests leading to a panic
> on
> boot. It relates to getting what I think are asm labels(?) and how we
> address them. I have just disabled fortify here for now; I think the
> code could be rewritten to take the labels as unsigned char *, but I
> haven't dug into it.

They probably need to be changed to use 'unsigned char foo[]' instead of
 'char foo' with &foo taken as an address and used as a larger buffer.
That's undefined and the compiler can assume it's limited to the size of
the type used to define it which then gets enforced by these fortified
wrappers rather than just used for optimization (in practice, it won't
break much without these, but it could).
Kees Cook May 8, 2017, 9:53 p.m. UTC | #2
On Mon, May 8, 2017 at 1:50 PM, Daniel Micay <danielmicay@gmail.com> wrote:
> On Tue, 2017-05-09 at 03:57 +1000, Daniel Axtens wrote:
>> Hi Daniel and ppc people,
>>
>> (ppc people: this does some compile and run time bounds checking on
>> 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.)
>>
>> I've tested this on ppc with pseries_le_defconfig.

Yay! Thanks very much!

>>
>> I needed a couple of the fixes from github
>> (https://github.com/thestinger/linux-hardened/commits/4.11) in order
>> to
>> build, specifically
>> https://github.com/thestinger/linux-hardened/commit/c65d6a6f309b067035
>> 84a23ac2b2bda4bb363143
>> https://github.com/thestinger/linux-
>> hardened/commit/adcec4756574a8c7f7cb5b6fa51ebeaeeae71aae
>
> By the way, Kees is working on landing these upstream as proper fixes
> rather than the workarounds I did there. In most cases, it's a matter of
> migrating from memcpy past the end of a constant string to strncpy (to
> make sure that the destination is still fully filled but with zeroes
> instead of arbitrary junk from rodata vs strcpy/strlcpy which won't
> cover that). A few of them are a bit weirder though. I haven't seen any
> false positives yet though, due to sticking to __builtin_object_size(p,
> 0) for now.

FWIW, all the net-dev ones have been applied for -next now.

>> Once those were added, I needed to disable fortification in
>> prom_init.c,
>> as we apparently can't have new symbols there. (I don't understand
>> that
>> file so I haven't dug into it.)
>>
>> We also have problems with the feature fixup tests leading to a panic
>> on
>> boot. It relates to getting what I think are asm labels(?) and how we
>> address them. I have just disabled fortify here for now; I think the
>> code could be rewritten to take the labels as unsigned char *, but I
>> haven't dug into it.
>
> They probably need to be changed to use 'unsigned char foo[]' instead of
>  'char foo' with &foo taken as an address and used as a larger buffer.
> That's undefined and the compiler can assume it's limited to the size of
> the type used to define it which then gets enforced by these fortified
> wrappers rather than just used for optimization (in practice, it won't
> break much without these, but it could).

We'd need something to actually extract the sizes of the asm
functions. Right now, that kind of thing is done in the linker
scripts, but that may be too late.

-Kees
Michael Ellerman May 10, 2017, noon UTC | #3
Daniel Axtens <dja@axtens.net> writes:

> Hi Daniel and ppc people,
>
> (ppc people: this does some compile and run time bounds checking on
> 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.)

Cool! 

> Once those were added, I needed to disable fortification in prom_init.c,
> as we apparently can't have new symbols there. (I don't understand that
> file so I haven't dug into it.)

We can refer to new symbols from there, we just have a script to check
we don't refer to anything new inadvertently.

prom_init() is sort of a shim that runs before the kernel, except it's
linked with the kernel, but ideally wouldn't be, but we never bothered
actually making it separate. O_o

> We also have problems with the feature fixup tests leading to a panic on
> boot. It relates to getting what I think are asm labels(?) and how we
> address them. I have just disabled fortify here for now; I think the
> code could be rewritten to take the labels as unsigned char *, but I
> haven't dug into it.

OK, yeah it's using asm labels to compare the patched code vs the
expected result. Can probably be fixed by making them char[] like we do
for other things like __start_interrupts etc.

cheers
diff mbox

Patch

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>
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index f3917705c686..2eee8558df61 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -12,6 +12,12 @@ 
  *  2 of the License, or (at your option) any later version.
  */
 
+/*
+ * feature fixup tests do memcmp with raw addresses rather than
+ * objects, which panics on boot with fortify on. TODO FIXME
+ */
+#define __NO_FORTIFY
+
 #include <linux/types.h>
 #include <linux/jump_label.h>
 #include <linux/kernel.h>