From 9c5b5cd530d73978987e961b6da123e8800a7799 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 23 Aug 2020 21:57:08 +0200 Subject: [PATCH 1/5] Make it unsafe to define NMI handlers --- macros/src/lib.rs | 42 +++++++++++++------ .../default-handler-bad-signature-1.rs | 4 +- .../default-handler-bad-signature-2.rs | 4 +- tests/compile-fail/default-handler-hidden.rs | 2 +- tests/compile-fail/default-handler-twice.rs | 4 +- tests/compile-fail/exception-nmi-unsafe.rs | 24 +++++++++++ .../hard-fault-bad-signature-1.rs | 4 +- tests/compile-fail/hard-fault-twice.rs | 4 +- tests/compile-fail/unsafe-init-static.rs | 4 +- 9 files changed, 67 insertions(+), 25 deletions(-) create mode 100644 tests/compile-fail/exception-nmi-unsafe.rs diff --git a/macros/src/lib.rs b/macros/src/lib.rs index 3c532c0d..7e54a5cf 100644 --- a/macros/src/lib.rs +++ b/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,20 +138,15 @@ 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() @@ -151,7 +154,22 @@ pub fn exception(args: TokenStream, input: TokenStream) -> TokenStream { } }; - // 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() diff --git a/tests/compile-fail/default-handler-bad-signature-1.rs b/tests/compile-fail/default-handler-bad-signature-1.rs index 5436115b..b5908839 100644 --- a/tests/compile-fail/default-handler-bad-signature-1.rs +++ b/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) [-> !]` diff --git a/tests/compile-fail/default-handler-bad-signature-2.rs b/tests/compile-fail/default-handler-bad-signature-2.rs index 1cca10cd..0dadd6ab 100644 --- a/tests/compile-fail/default-handler-bad-signature-2.rs +++ b/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 } diff --git a/tests/compile-fail/default-handler-hidden.rs b/tests/compile-fail/default-handler-hidden.rs index e57cb31d..c658e2bf 100644 --- a/tests/compile-fail/default-handler-hidden.rs +++ b/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) {} } diff --git a/tests/compile-fail/default-handler-twice.rs b/tests/compile-fail/default-handler-twice.rs index ad1c3f9b..bbf2eddd 100644 --- a/tests/compile-fail/default-handler-twice.rs +++ b/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) {} } diff --git a/tests/compile-fail/exception-nmi-unsafe.rs b/tests/compile-fail/exception-nmi-unsafe.rs new file mode 100644 index 00000000..f5de2f8d --- /dev/null +++ b/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` diff --git a/tests/compile-fail/hard-fault-bad-signature-1.rs b/tests/compile-fail/hard-fault-bad-signature-1.rs index d3b43929..11b53dc9 100644 --- a/tests/compile-fail/hard-fault-bad-signature-1.rs +++ b/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 {} } diff --git a/tests/compile-fail/hard-fault-twice.rs b/tests/compile-fail/hard-fault-twice.rs index 030b54cc..03b79a57 100644 --- a/tests/compile-fail/hard-fault-twice.rs +++ b/tests/compile-fail/hard-fault-twice.rs @@ -12,7 +12,7 @@ fn foo() -> ! { } #[exception] -fn HardFault(_ef: &ExceptionFrame) -> ! { +unsafe fn HardFault(_ef: &ExceptionFrame) -> ! { loop {} } @@ -20,7 +20,7 @@ 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 {} } } diff --git a/tests/compile-fail/unsafe-init-static.rs b/tests/compile-fail/unsafe-init-static.rs index c0401731..23105f15 100644 --- a/tests/compile-fail/unsafe-init-static.rs +++ b/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 {} } From ed09bf9e783733199446422ce6e06cc700dc3317 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 23 Aug 2020 21:59:01 +0200 Subject: [PATCH 2/5] Revert "Changed Hardfault's and DefaultHander's default implementations to panic" This reverts commit 6f524bd7552b1410c28bac43a797ed9447e8ad26. --- src/lib.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index fe4ee20c..335a3573 100644 --- a/src/lib.rs +++ b/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 /// @@ -990,17 +990,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)] From 7e3631b0192a526bdeb0b6b420a723283cbeb27c Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 23 Aug 2020 22:11:18 +0200 Subject: [PATCH 3/5] Document NMI handler unsafety --- src/lib.rs | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 335a3573..fe564771 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -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,6 +637,24 @@ 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;`. /// +/// # Safety +/// +/// 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`. +/// +/// 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. +/// +/// This also means that defining a `DefaultHandler` must be unsafe, as that will catch +/// `NonMaskableInt` and `HardFault` if no handlers for those are defined. +/// +/// 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 `HardFault` handler From b92b7d771ca4abe0746573bb59480881c6a9a305 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 23 Aug 2020 22:13:32 +0200 Subject: [PATCH 4/5] Remove broken doctest/example This is unsound unless you control the panic handler implementation. --- src/lib.rs | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index fe564771..65dee34e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -657,29 +657,13 @@ pub use macros::entry; /// /// # Examples /// -/// - Setting the `HardFault` handler -/// -/// ``` -/// # extern crate cortex_m_rt; -/// # extern crate cortex_m_rt_macros; -/// use cortex_m_rt::{ExceptionFrame, exception}; -/// -/// #[exception] -/// fn HardFault(ef: &ExceptionFrame) -> ! { -/// // prints the exception frame as a panic message -/// panic!("{:#?}", ef); -/// } -/// -/// # fn main() {} -/// ``` -/// /// - Setting the default handler /// /// ``` /// use cortex_m_rt::exception; /// /// #[exception] -/// fn DefaultHandler(irqn: i16) { +/// unsafe fn DefaultHandler(irqn: i16) { /// println!("IRQn = {}", irqn); /// } /// From 24c7ab13f3d65cc1573b12183b123dbb488fe98e Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 23 Aug 2020 22:33:15 +0200 Subject: [PATCH 5/5] Fix examples --- examples/divergent-default-handler.rs | 3 +-- examples/override-exception.rs | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/examples/divergent-default-handler.rs b/examples/divergent-default-handler.rs index 22fa437b..32902540 100644 --- a/examples/divergent-default-handler.rs +++ b/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 {} } diff --git a/examples/override-exception.rs b/examples/override-exception.rs index 6da6c5e4..3190b77d 100644 --- a/examples/override-exception.rs +++ b/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 {}