Fix thumbv6 source masking (#902)

We unconditionally enabled interrupts on exit of locks, now we
only enable interrupts that were disabled by the mask.

rtic/CHANGELOG.md

@@ -7,6 +7,12 @@ For each category, *Added*, *Changed*, *Fixed* add new entries at the top!
 
 ## [Unreleased]
 
+## [v2.1.1] - 2024-03-13
+
+### Fixed
+
+- **Soundness fix:** `thumbv6` was subject to race in source mask.
+
 ## [v2.1.0] - 2024-02-27
 
 ### Added

rtic/Cargo.toml

@@ -22,7 +22,7 @@ name = "rtic"
 readme = "../README.md"
 repository = "https://github.com/rtic-rs/rtic"
 
-version = "2.1.0"
+version = "2.1.1"
 
 [package.metadata.docs.rs]
 features = ["rtic-macros/test-template"]

rtic/ci/expected/racy-source-masking.run

@@ -0,0 +1,6 @@
+foo accessing shared2 resource start
+pending bar
+bar pended
+foo accessing shared2 resource end
+bar running
+bar accesing shared2 resource

rtic/examples/racy-source-masking.rs

@@ -0,0 +1,74 @@
+//! Bug in sourcemasking test.
+
+#![no_main]
+#![no_std]
+#![deny(warnings)]
+#![deny(unsafe_code)]
+#![deny(missing_docs)]
+
+use panic_semihosting as _;
+
+#[rtic::app(device = lm3s6965, dispatchers = [GPIOA])]
+mod app {
+    use cortex_m_semihosting::{debug, hprintln};
+
+    #[shared]
+    struct Shared {
+        shared1: u32, //shared between foo and bar, masks GPIOA and GPIOB
+        shared2: u32, //same
+    }
+
+    #[local]
+    struct Local {}
+
+    #[init]
+    fn init(_: init::Context) -> (Shared, Local) {
+        foo::spawn().unwrap();
+
+        (
+            Shared {
+                shared1: 0,
+                shared2: 0,
+            },
+            Local {},
+        )
+    }
+
+    #[task(shared = [shared1, shared2])]
+    async fn foo(c: foo::Context) {
+        let mut shared1 = c.shared.shared1;
+        let mut shared2 = c.shared.shared2;
+
+        shared2.lock(|shared2| {
+            hprintln!("foo accessing shared2 resource start");
+            //GPIOA and GPIOB masked
+            *shared2 += 1; //OK access to shared 1
+            shared1.lock(|shared1| {
+                //GPIOA and GPIOB masked again
+                *shared1 += 1; //so far so ggood
+            });
+
+            hprintln!("pending bar");
+            rtic::pend(lm3s6965::Interrupt::GPIOB);
+            hprintln!("bar pended");
+
+            //GPIOA and GPIOB unmasked
+            //racy access to shared2!
+            *shared2 += 1;
+            hprintln!("foo accessing shared2 resource end");
+        });
+
+        //GPIOA and GPIOB unmasked again
+
+        debug::exit(debug::EXIT_SUCCESS); // Exit QEMU simulator
+    }
+
+    #[task(binds = GPIOB, priority = 2, shared = [shared1, shared2])]
+    fn bar(mut c: bar::Context) {
+        hprintln!("bar running");
+        c.shared.shared2.lock(|shared2| {
+            hprintln!("bar accesing shared2 resource");
+            *shared2 += 1; // this can race with access in foo!
+        });
+    }
+}

rtic/src/export/cortex_source_mask.rs

@@ -134,12 +134,13 @@ pub unsafe fn lock<T, R, const M: usize>(
     } else {
         // safe to manipulate outside critical section
         let mask = compute_mask(0, ceiling, masks);
+        let old_mask = read_mask(mask);
         clear_enable_mask(mask);
 
         // execute closure under protection of raised system ceiling
         let r = f(&mut *ptr);
 
-        set_enable_mask(mask);
+        set_enable_mask(mask, old_mask);
 
         // safe to manipulate outside critical section
         r
@@ -178,11 +179,26 @@ pub const fn compute_mask<const M: usize>(
 
 // enables interrupts
 #[inline(always)]
-unsafe fn set_enable_mask<const M: usize>(mask: Mask<M>) {
+unsafe fn read_mask<const M: usize>(mask: Mask<M>) -> Mask<M> {
+    let mut out = Mask([0; M]);
+
     for i in 0..M {
         // This check should involve compile time constants and be optimized out.
         if mask.0[i] != 0 {
-            (*NVIC::PTR).iser[i].write(mask.0[i]);
+            out.0[i] = (*NVIC::PTR).iser[i].read();
+        }
+    }
+
+    out
+}
+
+// enables interrupts
+#[inline(always)]
+unsafe fn set_enable_mask<const M: usize>(mask: Mask<M>, old_mask: Mask<M>) {
+    for i in 0..M {
+        // This check should involve compile time constants and be optimized out.
+        if mask.0[i] != 0 {
+            (*NVIC::PTR).iser[i].write(old_mask.0[i]);
         }
     }
 }