Merge #289

289: Make it unsafe to define NMI handlers r=therealprof a=jonas-schievink

Reverts rust-embedded/cortex-m-rt#257

Fixes rust-embedded/cortex-m-rt#269
Fixes #196 (see that thread for details)

Co-authored-by: Jonas Schievink <[email protected]>

Author: bors[bot]

cortex-m-rt/examples/divergent-default-handler.rs

@@ -1,4 +1,3 @@
-#![deny(unsafe_code)]
 #![deny(warnings)]
 #![no_main]
 #![no_std]
@@ -14,6 +13,6 @@ fn foo() -> ! {
 }
 
 #[exception]
-fn DefaultHandler(_irqn: i16) -> ! {
+unsafe fn DefaultHandler(_irqn: i16) -> ! {
     loop {}
 }

cortex-m-rt/examples/override-exception.rs

@@ -1,6 +1,5 @@
 //! How to override the hard fault exception handler and the default exception handler
 
-#![deny(unsafe_code)]
 #![deny(warnings)]
 #![no_main]
 #![no_std]
@@ -18,12 +17,12 @@ fn main() -> ! {
 }
 
 #[exception]
-fn DefaultHandler(_irqn: i16) {
+unsafe fn DefaultHandler(_irqn: i16) {
     asm::bkpt();
 }
 
 #[exception]
-fn HardFault(_ef: &ExceptionFrame) -> ! {
+unsafe fn HardFault(_ef: &ExceptionFrame) -> ! {
     asm::bkpt();
 
     loop {}

cortex-m-rt/macros/src/lib.rs

@@ -113,6 +113,14 @@ pub fn entry(args: TokenStream, input: TokenStream) -> TokenStream {
     .into()
 }
 
+#[derive(Debug, PartialEq)]
+enum Exception {
+    DefaultHandler,
+    HardFault,
+    NonMaskableInt,
+    Other,
+}
+
 #[proc_macro_attribute]
 pub fn exception(args: TokenStream, input: TokenStream) -> TokenStream {
     let mut f = parse_macro_input!(input as ItemFn);
@@ -130,28 +138,38 @@ pub fn exception(args: TokenStream, input: TokenStream) -> TokenStream {
     let fspan = f.span();
     let ident = f.sig.ident.clone();
 
-    enum Exception {
-        DefaultHandler,
-        HardFault,
-        Other,
-    }
-
     let ident_s = ident.to_string();
     let exn = match &*ident_s {
         "DefaultHandler" => Exception::DefaultHandler,
         "HardFault" => Exception::HardFault,
+        "NonMaskableInt" => Exception::NonMaskableInt,
         // NOTE that at this point we don't check if the exception is available on the target (e.g.
         // MemoryManagement is not available on Cortex-M0)
-        "NonMaskableInt" | "MemoryManagement" | "BusFault" | "UsageFault" | "SecureFault"
-        | "SVCall" | "DebugMonitor" | "PendSV" | "SysTick" => Exception::Other,
+        "MemoryManagement" | "BusFault" | "UsageFault" | "SecureFault" | "SVCall"
+        | "DebugMonitor" | "PendSV" | "SysTick" => Exception::Other,
         _ => {
             return parse::Error::new(ident.span(), "This is not a valid exception name")
                 .to_compile_error()
                 .into();
         }
     };
 
-    // XXX should we blacklist other attributes?
+    if f.sig.unsafety.is_none() {
+        match exn {
+            Exception::DefaultHandler | Exception::HardFault | Exception::NonMaskableInt => {
+                // These are unsafe to define.
+                let name = if exn == Exception::DefaultHandler {
+                    format!("`DefaultHandler`")
+                } else {
+                    format!("`{:?}` handler", exn)
+                };
+                return parse::Error::new(ident.span(), format_args!("defining a {} is unsafe and requires an `unsafe fn` (see the cortex-m-rt docs)", name))
+                    .to_compile_error()
+                    .into();
+            }
+            Exception::Other => {}
+        }
+    }
 
     match exn {
         Exception::DefaultHandler => {
@@ -174,7 +192,7 @@ pub fn exception(args: TokenStream, input: TokenStream) -> TokenStream {
             if !valid_signature {
                 return parse::Error::new(
                     fspan,
-                    "`DefaultHandler` must have signature `[unsafe] fn(i16) [-> !]`",
+                    "`DefaultHandler` must have signature `unsafe fn(i16) [-> !]`",
                 )
                 .to_compile_error()
                 .into();
@@ -231,7 +249,7 @@ pub fn exception(args: TokenStream, input: TokenStream) -> TokenStream {
             if !valid_signature {
                 return parse::Error::new(
                     fspan,
-                    "`HardFault` handler must have signature `[unsafe] fn(&ExceptionFrame) -> !`",
+                    "`HardFault` handler must have signature `unsafe fn(&ExceptionFrame) -> !`",
                 )
                 .to_compile_error()
                 .into();
@@ -257,7 +275,7 @@ pub fn exception(args: TokenStream, input: TokenStream) -> TokenStream {
             )
             .into()
         }
-        Exception::Other => {
+        Exception::NonMaskableInt | Exception::Other => {
             let valid_signature = f.sig.constness.is_none()
                 && f.vis == Visibility::Inherited
                 && f.sig.abi.is_none()

cortex-m-rt/src/lib.rs

@@ -199,15 +199,14 @@
 //! won't find it.
 //!
 //! - `DefaultHandler`. This is the default handler. If not overridden using `#[exception] fn
-//! DefaultHandler(..` this will cause a panic with the message "DefaultHandler #`i`", where `i` is
-//! the number of the interrupt handler.
+//! DefaultHandler(..` this will be an infinite loop.
 //!
 //! - `HardFaultTrampoline`. This is the real hard fault handler. This function is simply a
 //! trampoline that jumps into the user defined hard fault handler named `HardFault`. The
 //! trampoline is required to set up the pointer to the stacked exception frame.
 //!
 //! - `HardFault`. This is the user defined hard fault handler. If not overridden using
-//! `#[exception] fn HardFault(..` it will default to a panic with message "HardFault".
+//! `#[exception] fn HardFault(..` it will default to an infinite loop.
 //!
 //! - `__STACK_START`. This is the first entry in the `.vector_table` section. This symbol contains
 //! the initial value of the stack pointer; this is where the stack will be located -- the stack
@@ -442,6 +441,7 @@ extern crate cortex_m_rt_macros as macros;
 extern crate r0;
 
 use core::fmt;
+use core::sync::atomic::{self, Ordering};
 
 /// Attribute to declare an interrupt (AKA device-specific exception) handler
 ///
@@ -612,13 +612,13 @@ pub use macros::entry;
 ///
 /// # Usage
 ///
-/// `#[exception] fn HardFault(..` sets the hard fault handler. The handler must have signature
-/// `[unsafe] fn(&ExceptionFrame) -> !`. This handler is not allowed to return as that can cause
-/// undefined behavior.
+/// `#[exception] unsafe fn HardFault(..` sets the hard fault handler. The handler must have
+/// signature `unsafe fn(&ExceptionFrame) -> !`. This handler is not allowed to return as that can
+/// cause undefined behavior.
 ///
-/// `#[exception] fn DefaultHandler(..` sets the *default* handler. All exceptions which have not
-/// been assigned a handler will be serviced by this handler. This handler must have signature
-/// `[unsafe] fn(irqn: i16) [-> !]`. `irqn` is the IRQ number (See CMSIS); `irqn` will be a negative
+/// `#[exception] unsafe fn DefaultHandler(..` sets the *default* handler. All exceptions which have
+/// not been assigned a handler will be serviced by this handler. This handler must have signature
+/// `unsafe fn(irqn: i16) [-> !]`. `irqn` is the IRQ number (See CMSIS); `irqn` will be a negative
 /// number when the handler is servicing a core exception; `irqn` will be a positive number when the
 /// handler is servicing a device specific exception (interrupt).
 ///
@@ -637,31 +637,33 @@ pub use macros::entry;
 /// the attribute will help by making a transformation to the source code: for this reason a
 /// variable like `static mut FOO: u32` will become `let FOO: &mut u32;`.
 ///
-/// # Examples
+/// # Safety
 ///
-/// - Setting the `HardFault` handler
+/// It is not generally safe to register handlers for non-maskable interrupts. On Cortex-M,
+/// `HardFault` is non-maskable (at least in general), and there is an explicitly non-maskable
+/// interrupt `NonMaskableInt`.
 ///
-/// ```
-/// # extern crate cortex_m_rt;
-/// # extern crate cortex_m_rt_macros;
-/// use cortex_m_rt::{ExceptionFrame, exception};
+/// The reason for that is that non-maskable interrupts will preempt any currently running function,
+/// even if that function executes within a critical section. Thus, if it was safe to define NMI
+/// handlers, critical sections wouldn't work safely anymore.
 ///
-/// #[exception]
-/// fn HardFault(ef: &ExceptionFrame) -> ! {
-///     // prints the exception frame as a panic message
-///     panic!("{:#?}", ef);
-/// }
+/// This also means that defining a `DefaultHandler` must be unsafe, as that will catch
+/// `NonMaskableInt` and `HardFault` if no handlers for those are defined.
 ///
-/// # fn main() {}
-/// ```
+/// The safety requirements on those handlers is as follows: The handler must not access any data
+/// that is protected via a critical section and shared with other interrupts that may be preempted
+/// by the NMI while holding the critical section. As long as this requirement is fulfilled, it is
+/// safe to handle NMIs.
+///
+/// # Examples
 ///
 /// - Setting the default handler
 ///
 /// ```
 /// use cortex_m_rt::exception;
 ///
 /// #[exception]
-/// fn DefaultHandler(irqn: i16) {
+/// unsafe fn DefaultHandler(irqn: i16) {
 ///     println!("IRQn = {}", irqn);
 /// }
 ///
@@ -990,17 +992,21 @@ pub unsafe extern "C" fn Reset() -> ! {
 #[link_section = ".HardFault.default"]
 #[no_mangle]
 pub unsafe extern "C" fn HardFault_(ef: &ExceptionFrame) -> ! {
-    panic!("HardFault");
+    loop {
+        // add some side effect to prevent this from turning into a UDF instruction
+        // see rust-lang/rust#28728 for details
+        atomic::compiler_fence(Ordering::SeqCst);
+    }
 }
 
 #[doc(hidden)]
 #[no_mangle]
 pub unsafe extern "C" fn DefaultHandler_() -> ! {
-    const SCB_ICSR: *const u32 = 0xE000_ED04 as *const u32;
-
-    let irqn = core::ptr::read(SCB_ICSR) as u8 as i16 - 16;
-
-    panic!("DefaultHandler #{}", irqn);
+    loop {
+        // add some side effect to prevent this from turning into a UDF instruction
+        // see rust-lang/rust#28728 for details
+        atomic::compiler_fence(Ordering::SeqCst);
+    }
 }
 
 #[doc(hidden)]

cortex-m-rt/tests/compile-fail/default-handler-bad-signature-1.rs

@@ -12,5 +12,5 @@ fn foo() -> ! {
 }
 
 #[exception]
-fn DefaultHandler(_irqn: i16, undef: u32) {}
-//~^ ERROR `DefaultHandler` must have signature `[unsafe] fn(i16) [-> !]`
+unsafe fn DefaultHandler(_irqn: i16, undef: u32) {}
+//~^ ERROR `DefaultHandler` must have signature `unsafe fn(i16) [-> !]`

cortex-m-rt/tests/compile-fail/default-handler-bad-signature-2.rs

@@ -12,7 +12,7 @@ fn foo() -> ! {
 }
 
 #[exception]
-fn DefaultHandler(_irqn: i16) -> u32 {
-    //~^ ERROR `DefaultHandler` must have signature `[unsafe] fn(i16) [-> !]`
+unsafe fn DefaultHandler(_irqn: i16) -> u32 {
+    //~^ ERROR `DefaultHandler` must have signature `unsafe fn(i16) [-> !]`
     0
 }

cortex-m-rt/tests/compile-fail/default-handler-hidden.rs

@@ -18,5 +18,5 @@ mod hidden {
     use cortex_m_rt::exception;
 
     #[exception]
-    fn DefaultHandler(_irqn: i16) {}
+    unsafe fn DefaultHandler(_irqn: i16) {}
 }

cortex-m-rt/tests/compile-fail/default-handler-twice.rs

@@ -12,11 +12,11 @@ fn foo() -> ! {
 }
 
 #[exception]
-fn DefaultHandler(_irqn: i16) {}
+unsafe fn DefaultHandler(_irqn: i16) {}
 
 pub mod reachable {
     use cortex_m_rt::exception;
 
     #[exception] //~ ERROR symbol `DefaultHandler` is already defined
-    fn DefaultHandler(_irqn: i16) {}
+    unsafe fn DefaultHandler(_irqn: i16) {}
 }

cortex-m-rt/tests/compile-fail/exception-nmi-unsafe.rs

@@ -0,0 +1,24 @@
+#![no_main]
+#![no_std]
+
+extern crate cortex_m_rt;
+extern crate panic_halt;
+
+use cortex_m_rt::{entry, exception};
+
+#[entry]
+fn foo() -> ! {
+    loop {}
+}
+
+#[exception]
+fn DefaultHandler(_irq: i16) {}
+//~^ ERROR defining a `DefaultHandler` is unsafe and requires an `unsafe fn`
+
+#[exception]
+fn HardFault() {}
+//~^ ERROR defining a `HardFault` handler is unsafe and requires an `unsafe fn`
+
+#[exception]
+fn NonMaskableInt() {}
+//~^ ERROR defining a `NonMaskableInt` handler is unsafe and requires an `unsafe fn`

cortex-m-rt/tests/compile-fail/hard-fault-bad-signature-1.rs

@@ -12,7 +12,7 @@ fn foo() -> ! {
 }
 
 #[exception]
-fn HardFault(_ef: &ExceptionFrame, undef: u32) -> ! {
-    //~^ ERROR `HardFault` handler must have signature `[unsafe] fn(&ExceptionFrame) -> !`
+unsafe fn HardFault(_ef: &ExceptionFrame, undef: u32) -> ! {
+    //~^ ERROR `HardFault` handler must have signature `unsafe fn(&ExceptionFrame) -> !`
     loop {}
 }

cortex-m-rt/tests/compile-fail/hard-fault-twice.rs

@@ -12,15 +12,15 @@ fn foo() -> ! {
 }
 
 #[exception]
-fn HardFault(_ef: &ExceptionFrame) -> ! {
+unsafe fn HardFault(_ef: &ExceptionFrame) -> ! {
     loop {}
 }
 
 pub mod reachable {
     use cortex_m_rt::{exception, ExceptionFrame};
 
     #[exception] //~ ERROR symbol `HardFault` is already defined
-    fn HardFault(_ef: &ExceptionFrame) -> ! {
+    unsafe fn HardFault(_ef: &ExceptionFrame) -> ! {
         loop {}
     }
 }

cortex-m-rt/tests/compile-fail/unsafe-init-static.rs

@@ -29,12 +29,12 @@ fn SVCall() {
 }
 
 #[exception]
-fn DefaultHandler(_irq: i16) {
+unsafe fn DefaultHandler(_irq: i16) {
     static mut X: u32 = init();  //~ ERROR requires unsafe
 }
 
 #[exception]
-fn HardFault(_frame: &cortex_m_rt::ExceptionFrame) -> ! {
+unsafe fn HardFault(_frame: &cortex_m_rt::ExceptionFrame) -> ! {
     static mut X: u32 = init();  //~ ERROR requires unsafe
     loop {}
 }