From 744e2cbb8a0b58eccf3a9708c8c28f1ef17e4771 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Mon, 5 Jul 2021 17:42:43 +1000 Subject: extras: Fix UB in `Peripheral` `Peripheral` assumed that interrupts can't be preempted, when they can be preempted by higher priority interrupts. So I put the interrupt handler inside a critical section, and also added checks for whether the state had been dropped before the critical section was entered. I also added a `'static` bound to `PeripheralState`, since `Pin` only guarantees that the memory it directly references will not be invalidated. It doesn't guarantee that memory its pointee references also won't be invalidated. There were already some implementations of `PeripheralState` that weren't `'static`, though, so I added an unsafe `PeripheralStateUnchecked` trait and forwarded the `unsafe` to the constructors of the implementors. --- embassy-extras/src/peripheral_shared.rs | 45 ++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 12 deletions(-) (limited to 'embassy-extras/src/peripheral_shared.rs') diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index c62113396..a9fca8ca6 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -1,16 +1,27 @@ -use core::cell::UnsafeCell; use core::marker::{PhantomData, PhantomPinned}; use core::pin::Pin; +use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; -pub trait PeripheralState { +/// # Safety +/// When types implementing this trait are used with `Peripheral` or `PeripheralMutex`, +/// their lifetime must not end without first calling `Drop` on the `Peripheral` or `PeripheralMutex`. +pub unsafe trait PeripheralStateUnchecked { type Interrupt: Interrupt; fn on_interrupt(&self); } -pub struct Peripheral { - state: UnsafeCell, +// `Peripheral` is safe because `Pin` guarantees that the memory it references will not be invalidated or reused +// without calling `Drop`. However, it provides no guarantees about references contained within the state still being valid, +// so this `'static` bound is necessary. +pub trait PeripheralState: 'static { + type Interrupt: Interrupt; + fn on_interrupt(&self); +} + +pub struct Peripheral { + state: S, irq_setup_done: bool, irq: S::Interrupt, @@ -19,13 +30,13 @@ pub struct Peripheral { _pinned: PhantomPinned, } -impl Peripheral { +impl Peripheral { pub fn new(irq: S::Interrupt, state: S) -> Self { Self { irq, irq_setup_done: false, - state: UnsafeCell::new(state), + state, _not_send: PhantomData, _pinned: PhantomPinned, } @@ -39,8 +50,16 @@ impl Peripheral { this.irq.disable(); this.irq.set_handler(|p| { - let state = unsafe { &*(p as *const S) }; - state.on_interrupt(); + // We need to be in a critical section so that no one can preempt us + // and drop the state after we check whether `p.is_null()`. + critical_section::with(|_| { + if p.is_null() { + // The state was dropped, so we can't operate on it. + return; + } + let state = unsafe { &*(p as *const S) }; + state.on_interrupt(); + }); }); this.irq .set_handler_context((&this.state) as *const _ as *mut ()); @@ -49,15 +68,17 @@ impl Peripheral { this.irq_setup_done = true; } - pub fn state(self: Pin<&mut Self>) -> &S { - let this = unsafe { self.get_unchecked_mut() }; - unsafe { &*this.state.get() } + pub fn state<'a>(self: Pin<&'a mut Self>) -> &'a S { + &self.into_ref().get_ref().state } } -impl Drop for Peripheral { +impl Drop for Peripheral { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); + // Set the context to null so that the interrupt will know we're dropped + // if we pre-empted it before it entered a critical section. + self.irq.set_handler_context(ptr::null_mut()); } } -- cgit From 3d96b10b0cd0066d4269094852e458bbf80a9807 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Mon, 5 Jul 2021 17:47:55 +1000 Subject: Elide lifetimes on `Peripheral::state` --- embassy-extras/src/peripheral_shared.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'embassy-extras/src/peripheral_shared.rs') diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index a9fca8ca6..d05ae0307 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -68,7 +68,7 @@ impl Peripheral { this.irq_setup_done = true; } - pub fn state<'a>(self: Pin<&'a mut Self>) -> &'a S { + pub fn state(self: Pin<&mut Self>) -> &S { &self.into_ref().get_ref().state } } -- cgit From fc1ef4947d463495ccd3da1d7763dcbe95a2588f Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Mon, 5 Jul 2021 18:18:05 +1000 Subject: Fix stm32 ethernet --- embassy-extras/src/peripheral_shared.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'embassy-extras/src/peripheral_shared.rs') diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index d05ae0307..820622bb9 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -5,8 +5,8 @@ use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; /// # Safety -/// When types implementing this trait are used with `Peripheral` or `PeripheralMutex`, -/// their lifetime must not end without first calling `Drop` on the `Peripheral` or `PeripheralMutex`. +/// When types implementing this trait are used with `Peripheral`, +/// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `Peripheral`. pub unsafe trait PeripheralStateUnchecked { type Interrupt: Interrupt; fn on_interrupt(&self); -- cgit From 1b7ad7080e6a8c96f2622d75b99a4d3bdbc7c394 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Sat, 24 Jul 2021 12:53:57 +1000 Subject: Add `Send/Sync` bounds to `PeripheralState` --- embassy-extras/src/peripheral_shared.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) (limited to 'embassy-extras/src/peripheral_shared.rs') diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index 820622bb9..5961441e0 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -4,18 +4,25 @@ use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; +/// A version of `PeripheralState` without the `'static` bound, +/// for cases where the compiler can't statically make sure +/// that `on_interrupt` doesn't reference anything which might be invalidated. +/// /// # Safety /// When types implementing this trait are used with `Peripheral`, /// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `Peripheral`. -pub unsafe trait PeripheralStateUnchecked { +pub unsafe trait PeripheralStateUnchecked: Sync { type Interrupt: Interrupt; fn on_interrupt(&self); } -// `Peripheral` is safe because `Pin` guarantees that the memory it references will not be invalidated or reused -// without calling `Drop`. However, it provides no guarantees about references contained within the state still being valid, -// so this `'static` bound is necessary. -pub trait PeripheralState: 'static { +/// A type which can be used as state with `Peripheral`. +/// +/// It needs to be `Sync` because references are shared between the 'thread' which owns the `Peripheral` and the interrupt. +/// +/// It also requires `'static`, because although `Pin` guarantees that the memory of the state won't be invalidated, +/// it doesn't guarantee that the lifetime will last. +pub trait PeripheralState: Sync + 'static { type Interrupt: Interrupt; fn on_interrupt(&self); } -- cgit From 079526559f44a0822574205e8798136f11f207d3 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Tue, 27 Jul 2021 17:28:52 +1000 Subject: Remove critical sections from `PeripheralMutex` interrupt handler by checking the interrupt's priority on startup. Since `PeripheralMutex` is the only way to safely maintain state across interrupts, and it no longer allows setting the interrupt's priority, the priority changing isn't a concern. This also prevents other causes of UB due to the interrupt being exposed during `with`, and allowing enabling the interrupt and setting its context to a bogus pointer. --- embassy-extras/src/peripheral_shared.rs | 61 +++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 14 deletions(-) (limited to 'embassy-extras/src/peripheral_shared.rs') diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index 5961441e0..ae9ae6935 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -1,9 +1,10 @@ use core::marker::{PhantomData, PhantomPinned}; use core::pin::Pin; -use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; +use crate::peripheral::can_be_preempted; + /// A version of `PeripheralState` without the `'static` bound, /// for cases where the compiler can't statically make sure /// that `on_interrupt` doesn't reference anything which might be invalidated. @@ -39,6 +40,10 @@ pub struct Peripheral { impl Peripheral { pub fn new(irq: S::Interrupt, state: S) -> Self { + if can_be_preempted(&irq) { + panic!("`Peripheral` cannot be created in an interrupt with higher priority than the interrupt it wraps"); + } + Self { irq, irq_setup_done: false, @@ -57,16 +62,11 @@ impl Peripheral { this.irq.disable(); this.irq.set_handler(|p| { - // We need to be in a critical section so that no one can preempt us - // and drop the state after we check whether `p.is_null()`. - critical_section::with(|_| { - if p.is_null() { - // The state was dropped, so we can't operate on it. - return; - } - let state = unsafe { &*(p as *const S) }; - state.on_interrupt(); - }); + // The state can't have been dropped, otherwise the interrupt would have been disabled. + // We checked in `new` that the thread owning the `Peripheral` can't preempt the interrupt, + // so someone can't have preempted us before this point and dropped the `Peripheral`. + let state = unsafe { &*(p as *const S) }; + state.on_interrupt(); }); this.irq .set_handler_context((&this.state) as *const _ as *mut ()); @@ -78,14 +78,47 @@ impl Peripheral { pub fn state(self: Pin<&mut Self>) -> &S { &self.into_ref().get_ref().state } + + /// Enables the wrapped interrupt. + pub fn enable(&self) { + // This is fine to do before initialization, because we haven't set the handler yet. + self.irq.enable() + } + + /// Disables the wrapped interrupt. + pub fn disable(&self) { + self.irq.disable() + } + + /// Returns whether the wrapped interrupt is enabled. + pub fn is_enabled(&self) -> bool { + self.irq.is_enabled() + } + + /// Returns whether the wrapped interrupt is currently in a pending state. + pub fn is_pending(&self) -> bool { + self.irq.is_pending() + } + + /// Forces the wrapped interrupt into a pending state. + pub fn pend(&self) { + self.irq.pend() + } + + /// Forces the wrapped interrupt out of a pending state. + pub fn unpend(&self) { + self.irq.unpend() + } + + /// Gets the priority of the wrapped interrupt. + pub fn priority(&self) -> ::Priority { + self.irq.get_priority() + } } impl Drop for Peripheral { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); - // Set the context to null so that the interrupt will know we're dropped - // if we pre-empted it before it entered a critical section. - self.irq.set_handler_context(ptr::null_mut()); } } -- cgit From 4d9514cbcb97343c3a75bfa565d753c44c2a0e27 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Wed, 28 Jul 2021 21:39:31 +1000 Subject: Don't allow disabling interrupts wrapped by `PeripheralMutex` --- embassy-extras/src/peripheral_shared.rs | 16 ---------------- 1 file changed, 16 deletions(-) (limited to 'embassy-extras/src/peripheral_shared.rs') diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index ae9ae6935..788ac3f96 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -79,22 +79,6 @@ impl Peripheral { &self.into_ref().get_ref().state } - /// Enables the wrapped interrupt. - pub fn enable(&self) { - // This is fine to do before initialization, because we haven't set the handler yet. - self.irq.enable() - } - - /// Disables the wrapped interrupt. - pub fn disable(&self) { - self.irq.disable() - } - - /// Returns whether the wrapped interrupt is enabled. - pub fn is_enabled(&self) -> bool { - self.irq.is_enabled() - } - /// Returns whether the wrapped interrupt is currently in a pending state. pub fn is_pending(&self) -> bool { self.irq.is_pending() -- cgit From d5ba35424d7eef2cc0c501758d214ce3a6febfc1 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Thu, 29 Jul 2021 15:11:26 +1000 Subject: Replace `PeripheralStateUnchecked` with `register_interrupt_unchecked` --- embassy-extras/src/peripheral_shared.rs | 52 +++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 19 deletions(-) (limited to 'embassy-extras/src/peripheral_shared.rs') diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index 788ac3f96..71d746341 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -5,30 +5,19 @@ use embassy::interrupt::{Interrupt, InterruptExt}; use crate::peripheral::can_be_preempted; -/// A version of `PeripheralState` without the `'static` bound, -/// for cases where the compiler can't statically make sure -/// that `on_interrupt` doesn't reference anything which might be invalidated. -/// -/// # Safety -/// When types implementing this trait are used with `Peripheral`, -/// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `Peripheral`. -pub unsafe trait PeripheralStateUnchecked: Sync { - type Interrupt: Interrupt; - fn on_interrupt(&self); -} - /// A type which can be used as state with `Peripheral`. /// /// It needs to be `Sync` because references are shared between the 'thread' which owns the `Peripheral` and the interrupt. /// -/// It also requires `'static`, because although `Pin` guarantees that the memory of the state won't be invalidated, +/// It also requires `'static` to be used safely with `Peripheral::register_interrupt`, +/// because although `Pin` guarantees that the memory of the state won't be invalidated, /// it doesn't guarantee that the lifetime will last. -pub trait PeripheralState: Sync + 'static { +pub trait PeripheralState: Sync { type Interrupt: Interrupt; fn on_interrupt(&self); } -pub struct Peripheral { +pub struct Peripheral { state: S, irq_setup_done: bool, @@ -38,7 +27,24 @@ pub struct Peripheral { _pinned: PhantomPinned, } -impl Peripheral { +impl Peripheral { + /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it. + /// + /// This requires this `Peripheral`'s `PeripheralState` to live for `'static`, + /// because `Pin` only guarantees that it's memory won't be repurposed, + /// not that it's lifetime will last. + /// + /// To use non-`'static` `PeripheralState`, use the unsafe `register_interrupt_unchecked`. + /// + /// Note: `'static` doesn't mean it _has_ to live for the entire program, like an `&'static T`; + /// it just means it _can_ live for the entire program - for example, `u8` lives for `'static`. + pub fn register_interrupt(self: Pin<&mut Self>) { + // SAFETY: `S: 'static`, so there's no way it's lifetime can expire. + unsafe { self.register_interrupt_unchecked() } + } +} + +impl Peripheral { pub fn new(irq: S::Interrupt, state: S) -> Self { if can_be_preempted(&irq) { panic!("`Peripheral` cannot be created in an interrupt with higher priority than the interrupt it wraps"); @@ -54,8 +60,16 @@ impl Peripheral { } } - pub fn register_interrupt(self: Pin<&mut Self>) { - let this = unsafe { self.get_unchecked_mut() }; + /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it. + /// + /// # Safety + /// The lifetime of any data in `PeripheralState` that is accessed by the interrupt handler + /// must not end without `Drop` being called on this `Peripheral`. + /// + /// This can be accomplished by either not accessing any data with a lifetime in `on_interrupt`, + /// or making sure that nothing like `mem::forget` is used on the `Peripheral`. + pub unsafe fn register_interrupt_unchecked(self: Pin<&mut Self>) { + let this = self.get_unchecked_mut(); if this.irq_setup_done { return; } @@ -100,7 +114,7 @@ impl Peripheral { } } -impl Drop for Peripheral { +impl Drop for Peripheral { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); -- cgit