Feat/riscv pmp stackguard

Makefile.dep

@@ -33,6 +33,10 @@ ifneq (,$(filter mpu_noexec_ram,$(USEMODULE)))
   FEATURES_REQUIRED += cortexm_mpu
 endif
 
+ifneq (,$(filter pmp_stack_guard,$(USEMODULE)))
+  FEATURES_REQUIRED += periph_pmp
+endif
+
 ifneq (,$(filter pmp_noexec_ram,$(USEMODULE)))
   FEATURES_REQUIRED += periph_pmp
 endif

core/include/thread.h

@@ -193,7 +193,8 @@ struct _thread {
                                          to this thread's message queue */
 #endif
 #if defined(DEVELHELP) || IS_ACTIVE(SCHED_TEST_STACK) \
-    || defined(MODULE_MPU_STACK_GUARD) || defined(DOXYGEN)
+    || defined(MODULE_MPU_STACK_GUARD) || defined(MODULE_PMP_STACK_GUARD) \
+    || defined(DOXYGEN)
     char *stack_start;              /**< thread's stack start address   */
 #endif
 #if defined(CONFIG_THREAD_NAMES) || defined(DOXYGEN)
@@ -555,7 +556,7 @@ const char *thread_state_to_string(thread_status_t state);
 static inline void *thread_get_stackstart(const thread_t *thread)
 {
 #if defined(DEVELHELP) || IS_ACTIVE(SCHED_TEST_STACK) \
-    || defined(MODULE_MPU_STACK_GUARD)
+    || defined(MODULE_MPU_STACK_GUARD) || defined(MODULE_PMP_STACK_GUARD)
     return thread->stack_start;
 #else
     (void)thread;

core/sched.c

@@ -36,6 +36,10 @@
 #include "mpu.h"
 #endif
 
+#ifdef MODULE_PMP_STACK_GUARD
+#include "pmp.h"
+#endif
+
 #define ENABLE_DEBUG 0
 #include "debug.h"
 
@@ -218,6 +222,10 @@ thread_t *__attribute__((used)) sched_run(void)
             MPU_ATTR(1, AP_RO_RO, 0, 1, 0, 1, MPU_SIZE_32B) /* Attributes and Size */
             );
 #endif
+
+#ifdef MODULE_PMP_STACK_GUARD
+        write_pmpaddr(PMP_REGION_STACK_GUARD, (uintptr_t)next_thread->stack_start);
+#endif
         DEBUG("sched_run: done, changed sched_active_thread.\n");
     }
 

core/thread.c

@@ -285,7 +285,7 @@ kernel_pid_t thread_create(char *stack, int stacksize, uint8_t priority,
     thread->sp = thread_stack_init(function, arg, stack, stacksize);
 
 #if defined(DEVELHELP) || IS_ACTIVE(SCHED_TEST_STACK) || \
-    defined(MODULE_MPU_STACK_GUARD)
+    defined(MODULE_MPU_STACK_GUARD) || defined(MODULE_PMP_STACK_GUARD)
     thread->stack_start = stack;
 #endif
 

cpu/fe310/include/cpu_conf.h

@@ -38,9 +38,10 @@ extern "C" {
 
 /**
  * @brief Number of available PMP regions
- * Note, the upper 8 regions are hardwired to zero!
+ * Note, this differs from the specification, which expects 16 or 64 regions.
+ * Here, the upper 8 regions are hardwired to zero!
  */
-#define NUM_PMP_ENTRIES 16
+#define PMP_REGION_COUNT (8)
 
 #ifdef __cplusplus
 }

cpu/riscv_common/include/pmp.h

@@ -13,8 +13,8 @@
  * @file
  * @brief           RISC-V PMP configuration options
  *
- * RISCV implementations using this peripheral must define the `NUM_PMP_ENTRIES`
- * `NUM_PMP_ENTRIES` must be 16 or 64.
+ * RISCV implementations using this peripheral must define the `PMP_REGION_COUNT`
+ * `PMP_REGION_COUNT` must be 16 or 64.
  *
  * @author          Bennet Blischke
  */
@@ -25,10 +25,26 @@
 #include <assert.h>
 #include <stdint.h>
 
+#include "cpu_conf.h"
+
 #ifdef __cplusplus
 extern "C" {
 #endif
 
+/**
+ * @name Region definitions per use-case, depending on the priority and the 
+ * number of available regions on a given system
+ * @{
+ */
+#if PMP_REGION_COUNT < 4
+#error "You do not have enough PMP entries to run RIOT"
+#endif
+#define PMP_REGION_STACK_GUARD (PMP_REGION_COUNT - 4) /**< Used for the PMP_STACK_GUARD module */
+/* One region left free to account for silicon bugs in the hifive1b / fe310 */
+#define PMP_REGION_NOEXEC_RAM (PMP_REGION_COUNT - 2) /**< Used for the PMP_NOEXEC_RAM module */
+#define PMP_REGION_ALLOW_ALL (PMP_REGION_COUNT - 1) /**< Used as a catch-all fall back */
+/** @} */
+
 /**
  * @name    Bit masks for the PMP configuration register
  * @{
@@ -59,6 +75,11 @@ static inline uint32_t make_napot(uint32_t addr, uint32_t size)
     return addr | ((size - 1) >> 1);
 }
 
+/**
+ * @brief A NAPOT formatted address that encodes the entire 34-Bit address space
+ */
+#define PMP_NAPOT_ENTIRE_ADDRESS_SPACE UINT32_MAX
+
 /**
  * @brief   Writes a complete pmpcfg register
  *

cpu/riscv_common/periph/pmp.c

@@ -23,10 +23,6 @@
 #include "vendor/riscv_csr.h"
 #include "pmp.h"
 
-#if NUM_PMP_ENTRIES != 16 && NUM_PMP_ENTRIES != 64
-#error "Only 16 or 64 PMP entries allowed."
-#endif
-
 #define _WRITE_PMPCFG(REG, VALUE) write_csr(REG, VALUE)
 #define _READ_PMPCFG(REG) read_csr(REG)
 #define WRITE_PMPCFG(REG, VALUE) _WRITE_PMPCFG(CSR_PMPCFG0 + REG, VALUE)
@@ -50,13 +46,13 @@
  */
 void write_pmpcfg(uint8_t reg_num, uint32_t value)
 {
-    assert(reg_num < NUM_PMP_ENTRIES / 4);
+    assert(reg_num < PMP_REGION_COUNT / 4);
     switch (reg_num) {
     case 0: WRITE_PMPCFG(0, value); break;
     case 1: WRITE_PMPCFG(1, value); break;
     case 2: WRITE_PMPCFG(2, value); break;
     case 3: WRITE_PMPCFG(3, value); break;
-#if NUM_PMP_ENTRIES == 64
+#if PMP_REGION_COUNT > 16
     case 4: WRITE_PMPCFG(4, value); break;
     case 5: WRITE_PMPCFG(5, value); break;
     case 6: WRITE_PMPCFG(6, value); break;
@@ -75,13 +71,13 @@ void write_pmpcfg(uint8_t reg_num, uint32_t value)
 
 uint32_t read_pmpcfg(uint8_t reg_num)
 {
-    assert(reg_num < NUM_PMP_ENTRIES / 4);
+    assert(reg_num < PMP_REGION_COUNT / 4);
     switch (reg_num) {
     case 0: return READ_PMPCFG(0);
     case 1: return READ_PMPCFG(1);
     case 2: return READ_PMPCFG(2);
     case 3: return READ_PMPCFG(3);
-#if NUM_PMP_ENTRIES == 64
+#if PMP_REGION_COUNT > 16
     case 4: return READ_PMPCFG(4);
     case 5: return READ_PMPCFG(5);
     case 6: return READ_PMPCFG(6);
@@ -101,7 +97,7 @@ uint32_t read_pmpcfg(uint8_t reg_num)
 
 void write_pmpaddr(uint8_t reg_num, uint32_t value)
 {
-    assert(reg_num < NUM_PMP_ENTRIES);
+    assert(reg_num < PMP_REGION_COUNT);
     switch (reg_num) {
     case 0: WRITE_PMPADDR(0, value); break;
     case 1: WRITE_PMPADDR(1, value); break;
@@ -119,7 +115,7 @@ void write_pmpaddr(uint8_t reg_num, uint32_t value)
     case 13: WRITE_PMPADDR(13, value); break;
     case 14: WRITE_PMPADDR(14, value); break;
     case 15: WRITE_PMPADDR(15, value); break;
-#if NUM_PMP_ENTRIES == 64
+#if PMP_REGION_COUNT > 16
     case 16: WRITE_PMPADDR(16, value); break;
     case 17: WRITE_PMPADDR(17, value); break;
     case 18: WRITE_PMPADDR(18, value); break;
@@ -174,7 +170,7 @@ void write_pmpaddr(uint8_t reg_num, uint32_t value)
 
 uint32_t read_pmpaddr(uint8_t reg_num)
 {
-    assert(reg_num < NUM_PMP_ENTRIES);
+    assert(reg_num < PMP_REGION_COUNT);
     switch (reg_num) {
     case 0: return READ_PMPADDR(0);
     case 1: return READ_PMPADDR(1);
@@ -192,7 +188,7 @@ uint32_t read_pmpaddr(uint8_t reg_num)
     case 13: return READ_PMPADDR(13);
     case 14: return READ_PMPADDR(14);
     case 15: return READ_PMPADDR(15);
-#if NUM_PMP_ENTRIES == 64
+#if PMP_REGION_COUNT > 16
     case 16: return READ_PMPADDR(16);
     case 17: return READ_PMPADDR(17);
     case 18: return READ_PMPADDR(18);

cpu/riscv_common/riscv_init.c

@@ -21,7 +21,7 @@
 #include "cpu_conf_common.h"
 #include "periph_cpu_common.h"
 
-#ifdef MODULE_PMP_NOEXEC_RAM
+#if defined(MODULE_PMP_NOEXEC_RAM) || defined(MODULE_PMP_STACK_GUARD)
 #include "pmp.h"
 #endif
 
@@ -49,13 +49,33 @@ void riscv_init(void)
 {
     riscv_fpu_init();
     riscv_irq_init();
-#ifdef MODULE_PMP_NOEXEC_RAM
-    /* This marks the (main) RAM region as non
-     * executable. Using PMP entry 0.
+
+#ifdef MODULE_PMP_STACK_GUARD
+    /* There is no thread running yet, pre-configure the stack guard using
+     * the zero address to prevent unintended side effects.
+     */
+    write_pmpaddr(PMP_REGION_STACK_GUARD, 0x00000000);
+    set_pmpcfg(PMP_REGION_STACK_GUARD, PMP_NA4 | PMP_R);
+
+    /* When the effective memory privilege is dropped later, all M-mode accesses 
+     * are treated as U-mode and still need to pass the PMP.
+     * This implements an allowlist for all memory accesses with the lowest
+     * priority, that is, the highest region number.
      */
-    write_pmpaddr(0, make_napot(CPU_RAM_BASE, CPU_RAM_SIZE));
+    write_pmpaddr(PMP_REGION_ALLOW_ALL, PMP_NAPOT_ENTIRE_ADDRESS_SPACE);
+    set_pmpcfg(PMP_REGION_ALLOW_ALL, PMP_NAPOT | PMP_W | PMP_R | PMP_X);
+
+    /* Drop the memory privilege to U-mode to make sure that rules (the stack guard)
+     * without the lock bit set also apply to M-mode */
+    clear_csr(mstatus, MSTATUS_MPP);
+    set_csr(mstatus, MSTATUS_MPRV);
+#endif
+
+#if defined(MODULE_PMP_NOEXEC_RAM)
+    /* This marks the (main) RAM region as non-executable. */
+    write_pmpaddr(PMP_REGION_NOEXEC_RAM, make_napot(CPU_RAM_BASE, CPU_RAM_SIZE));
 
     /* Lock & select NAPOT, only allow write and read */
-    set_pmpcfg(0, PMP_L | PMP_NAPOT | PMP_W | PMP_R);
+    set_pmpcfg(PMP_REGION_NOEXEC_RAM, PMP_L | PMP_NAPOT | PMP_W | PMP_R);
 #endif
 }

makefiles/pseudomodules.inc.mk

@@ -322,6 +322,13 @@ PSEUDOMODULES += mpu_stack_guard
 PSEUDOMODULES += mpu_noexec_ram
 ## @}
 
+## @defgroup pseudomodule_pmp_stack_guard pmp_stack_guard
+## @brief PMP based stack guard
+##
+## When this module is active,
+## the Physical Memory Protection Unit will be configured to detect stack overflows.
+PSEUDOMODULES += pmp_stack_guard
+
 ## @defgroup pseudomodule_pmp_noexec_ram pmp_noexec_ram
 ## @{
 ## @brief Mark RAM as non-executable using the PMP

tests/bench/sizeof_coretypes/main.c

@@ -101,7 +101,8 @@ int main(void)
     P(msg_queue);
     P(msg_array);
 #endif
-#if defined(DEVELHELP) || IS_ACTIVE(SCHED_TEST_STACK) || defined(MODULE_MPU_STACK_GUARD)
+#if defined(DEVELHELP) || IS_ACTIVE(SCHED_TEST_STACK) \
+|| defined(MODULE_MPU_STACK_GUARD) || defined(MODULE_PMP_STACK_GUARD)
     P(stack_start);
 #endif
 #ifdef DEVELHELP

tests/cpu/pmp_stack_guard/Makefile

@@ -0,0 +1,11 @@
+BOARD ?= hifive1b
+
+include ../Makefile.cpu_common
+
+USEMODULE += pmp_stack_guard
+
+include $(RIOTBASE)/Makefile.include
+
+ifeq (llvm,$(TOOLCHAIN))
+  CFLAGS += -Wno-infinite-recursion
+endif

tests/cpu/pmp_stack_guard/main.c

@@ -0,0 +1,98 @@
+/*
+ * Copyright (C) 2016 Loci Controls Inc.
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup tests
+ * @{
+ *
+ * @file
+ * @brief Test application for the pmp_stack_guard pseudo-module
+ *
+ * @author Ian Martin <[email protected]>
+ * @author Bennet Blischke <[email protected]>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+
+#include "cpu.h"
+#include "thread.h"
+#include "thread_arch.h"
+#include "pmp.h"
+
+#define CANARY_VALUE 0xdeadbeef
+
+static struct {
+    char overflow_buffer[128];
+    unsigned int canary;
+    char stack[THREAD_STACKSIZE_MAIN];
+} buf;
+
+
+static inline unsigned int __get_SP(void) {
+    unsigned int __tmp;
+    __asm__ volatile ("mv %0, sp" : "=r"(__tmp));
+    return __tmp;
+}
+
+/* Tell modern GCC (12.x) to not complain that this infinite recursion is
+ * bound to overflow the stack - this is exactly what this test wants to do :)
+ *
+ * Also, tell older versions of GCC that do not know about -Winfinit-recursion
+ * that it is safe to ignore `GCC diagnostics ignored "-Winfinit-recursion"`.
+ * They behave as intended in this case :)
+ */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wpragmas"
+#pragma GCC diagnostic ignored "-Winfinite-recursion"
+static int recurse(int counter)
+{
+    printf("counter =%4d, SP = 0x%08x, canary = 0x%08x\n", counter, (unsigned int)__get_SP(), buf.canary);
+
+    if (buf.canary != CANARY_VALUE) {
+        printf("canary = 0x%08x\nTest failed.\n", buf.canary);
+
+        for (;;) {
+            thread_sleep();
+        }
+    }
+
+    counter++;
+
+    /* Recursing twice here prevents the compiler from optimizing-out the recursion. */
+    return recurse(counter) + recurse(counter);
+}
+#pragma GCC diagnostic pop
+
+static void *thread(void *arg)
+{
+    (void) arg;
+
+    recurse(0);
+
+    return NULL;
+}
+
+int main(void)
+{
+    puts("\nPMP Stack Guard Test\n");
+    puts("If the test fails, the canary value will change unexpectedly");
+    puts("after ~50 iterations. If the test succeeds, the MEM MANAGE HANDLER");
+    puts("will trigger a RIOT kernel panic before the canary value changes.\n");
+#ifdef MODULE_PMP_STACK_GUARD
+    puts("The pmp_stack_guard module is present. Expect the test to succeed.\n");
+#else
+    puts("The pmp_stack_guard module is missing! Expect the test to fail.\n");
+#endif
+
+    buf.canary = CANARY_VALUE;
+    thread_create(buf.stack, sizeof(buf.stack), THREAD_PRIORITY_MAIN - 1, 0, thread, NULL, "thread");
+
+    return 0;
+}