Stack protector for stmhal

[prusnak]

I am starting this thread rather than sending a pull request, because I'd like to get more input on the issue first.

It seems that stack protector is turned off for stmhal port (#700).

Is there a significant reason why not to enable it?

My minimal changeset to enable it is here:

diff --git a/stmhal/Makefile b/stmhal/Makefile
index 1ad2783..7183237 100644
--- a/stmhal/Makefile
+++ b/stmhal/Makefile
@@ -66,6 +66,9 @@ LIBS =
 CFLAGS += -fdata-sections -ffunction-sections
 LDFLAGS += --gc-sections

+# Enable stack protector
+CFLAGS += -fstack-protector-all
+
 # Debugging/Optimization
 ifeq ($(DEBUG), 1)
 CFLAGS += -g -DPENDSV_DEBUG
diff --git a/stmhal/main.c b/stmhal/main.c
index 30dddaf..d626ad2 100644
--- a/stmhal/main.c
+++ b/stmhal/main.c
@@ -105,6 +105,12 @@ void nlr_jump_fail(void *val) {
     __fatal_error("");
 }

+uint32_t __stack_chk_guard;
+
+void NORETURN __stack_chk_fail(void) {
+    __fatal_error("Stack smashing detected");
+}
+
 #ifndef NDEBUG
 void MP_WEAK __assert_func(const char *file, int line, const char *func, const char *expr) {
     (void)func;
@@ -343,6 +349,9 @@ STATIC uint update_reset_mode(uint reset_mode) {
 }

 int main(void) {
+
+    __stack_chk_guard = rng_get();
+
     // TODO disable JTAG

     // Stack limit should be less than real stack size, so we have a chance

Couple of issues to resolve:

• where to put __stack_chk_guard initialization? (obviously you want to have it closest to the beginning of the main function, but only after RNG is initialized)
• compiling goes without problem, but linking fails with arm-none-eabi-ld: build-PYBV10/firmware.elf section.isr_vector' will not fit in region FLASH_ISR'

Ideas? Comments?

[dhylands]

Well it results in a 20% increase in code size (a bit over 60K).

Reading up on the technique, it seems that its really a hardening technique and not a debugging technique. Its primary purpose is to prevent intentional buffer overflows that are used to inject malicious code.

Definitely not worth the 20% increase in code size in my books.

The reason for the ISR vector not fitting is that segment is fixed at 16K and the code that we stick in there increased enough that it no longer fits.

[prusnak]

I would certainly appreciate if we could make this happen with a compile-time option. I am not suggesting it as default, but having it easily accessible.