From 0a1da873ddb29bca926bad8301f8d7ab8aa97c52 Mon Sep 17 00:00:00 2001 From: Kyle Schaffrick Date: Tue, 30 Mar 2021 15:55:39 -0500 Subject: [PATCH] Fix unsoundness in ImmediateIO and TransactionalIO, aka RUSTSEC-2020-0152 aka edarc/max7301#1. Unsafe impls of Sync for both of these structs are removed, and Send bounds are introduced where necessary to allow Sync to be properly deduced instead. The unsafe impls were introduced due to an incorrect analysis of why Sync was not being auto-deduced as expected for these structs. The removed comment explains the original flawed analysis, which attributed it to the presence of a struct member of type PhantomData, under the assumption that the only "real" value of type EI in the struct is ultimately inside an IOMutex that should bless it with Sync-ness regardless if EI is itself Sync. In fact, Sync was not being deduced due to the absence of a *Send* bound on the type EI, which is required for Expander to be Send, which is required for IOMutex> to be *Sync*. The presence of the PhantomData member turns out to be entirely irrelevant. The bug could theoretically result in a data race if either type is used with an EI that is not safe to send between threads. This is a breaking change due to the addition of a trait bound in the public API. --- src/config.rs | 4 ++-- src/expander/immediate.rs | 17 +++----------- src/expander/mod.rs | 2 +- src/expander/transactional.rs | 17 +++----------- src/interface.rs | 44 ++++++++++++++++++----------------- 5 files changed, 32 insertions(+), 52 deletions(-) diff --git a/src/config.rs b/src/config.rs index dee5363..1120822 100644 --- a/src/config.rs +++ b/src/config.rs @@ -130,13 +130,13 @@ impl From for u8 { /// .unwrap(); /// ``` #[must_use = "Configuration changes are not applied unless committed"] -pub struct Configurator<'e, EI: ExpanderInterface> { +pub struct Configurator<'e, EI: ExpanderInterface + Send> { expander: &'e mut Expander, expander_config_dirty: bool, banks: [BankConfig; 7], } -impl<'e, EI: ExpanderInterface> Configurator<'e, EI> { +impl<'e, EI: ExpanderInterface + Send> Configurator<'e, EI> { pub(crate) fn new(expander: &'e mut Expander) -> Self { Self { expander, diff --git a/src/expander/immediate.rs b/src/expander/immediate.rs index f4fd86b..fd617d7 100644 --- a/src/expander/immediate.rs +++ b/src/expander/immediate.rs @@ -14,23 +14,12 @@ use registers::valid_port; pub struct ImmediateIO(M, PhantomData) where M: IOMutex>, - EI: ExpanderInterface; - -// Unsafety: This is only needed because the presence of PhantomData causes the struct to no -// longer be Sync, because EI is often not Sync since it owns a global resource (e.g. SPI device). -// However, the EI is actually owned by the Expander which is in the mutex which normally -// re-instates Sync-ness. PhantomData is there to shut up the unused type parameter error. -unsafe impl Sync for ImmediateIO -where - M: IOMutex>, - EI: ExpanderInterface, -{ -} + EI: ExpanderInterface + Send; impl ImmediateIO where M: IOMutex>, - EI: ExpanderInterface, + EI: ExpanderInterface + Send, { pub(crate) fn new(expander: Expander) -> Self { ImmediateIO(M::new(expander), PhantomData) @@ -54,7 +43,7 @@ where impl ExpanderIO for ImmediateIO where M: IOMutex>, - EI: ExpanderInterface, + EI: ExpanderInterface + Send, { fn write_port(&self, port: u8, bit: bool) { self.0.lock(|ex| ex.write_port(port, bit).unwrap()) diff --git a/src/expander/mod.rs b/src/expander/mod.rs index 59dbcde..27a309a 100644 --- a/src/expander/mod.rs +++ b/src/expander/mod.rs @@ -18,7 +18,7 @@ pub struct Expander { pub(crate) config: ExpanderConfig, } -impl Expander { +impl Expander { /// Create a new `Expander`. /// /// Takes ownership of the `ExpanderInterface` which it should use to communicate with the diff --git a/src/expander/transactional.rs b/src/expander/transactional.rs index d99a00c..eb6729f 100644 --- a/src/expander/transactional.rs +++ b/src/expander/transactional.rs @@ -48,7 +48,7 @@ pub enum Strategy { pub struct TransactionalIO where M: IOMutex>, - EI: ExpanderInterface, + EI: ExpanderInterface + Send, { expander: M, issued: AtomicUsize, @@ -58,21 +58,10 @@ where _ei: PhantomData, } -// NOTE(unsafe): This is only needed because the presence of PhantomData causes the struct to -// no longer be Sync, because EI is often not Sync since it owns a global resource (e.g. SPI -// device). However, the EI is actually owned by the Expander which is in the mutex which normally -// re-instates Sync-ness. PhantomData is there to shut up the unused type parameter error. -unsafe impl Sync for TransactionalIO -where - M: IOMutex>, - EI: ExpanderInterface, -{ -} - impl TransactionalIO where M: IOMutex>, - EI: ExpanderInterface, + EI: ExpanderInterface + Send, { pub(crate) fn new(expander: Expander) -> Self { TransactionalIO { @@ -160,7 +149,7 @@ where impl ExpanderIO for TransactionalIO where M: IOMutex>, - EI: ExpanderInterface, + EI: ExpanderInterface + Send, { fn write_port(&self, port: u8, bit: bool) { let or_bit = 1 << port; diff --git a/src/interface.rs b/src/interface.rs index baca2ff..d8f1e4f 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -121,9 +121,8 @@ pub(crate) mod test_spy { use super::ExpanderInterface; use registers::RegisterAddress; - use std::cell::RefCell; use std::fmt; - use std::rc::Rc; + use std::sync::{Arc, Mutex}; #[derive(Clone, Copy, Debug, PartialEq)] pub enum TestRegister { @@ -134,15 +133,15 @@ pub(crate) mod test_spy { } pub struct TestSpyInterface { - registers: Rc>>, - reads: Rc>>, + registers: Arc>>, + reads: Arc>>, } impl TestSpyInterface { pub fn new() -> Self { let mut new = Self { - registers: Rc::new(RefCell::new(Vec::new())), - reads: Rc::new(RefCell::new(Vec::new())), + registers: Arc::new(Mutex::new(Vec::new())), + reads: Arc::new(Mutex::new(Vec::new())), }; new.reset(); new @@ -151,8 +150,8 @@ pub(crate) mod test_spy { pub fn reset(&mut self) { use self::TestRegister::*; - self.reads.borrow_mut().clear(); - let mut regs = self.registers.borrow_mut(); + self.reads.lock().unwrap().clear(); + let mut regs = self.registers.lock().unwrap(); regs.clear(); regs.resize(0x60, Forbidden); @@ -179,21 +178,21 @@ pub(crate) mod test_spy { } pub fn get(&self, addr: u8) -> TestRegister { - self.registers.borrow()[addr as usize] + self.registers.lock().unwrap()[addr as usize] } pub fn set(&mut self, addr: u8, val: TestRegister) { - self.registers.borrow_mut()[addr as usize] = val; + self.registers.lock().unwrap()[addr as usize] = val; } pub fn reads(&self) -> Vec { - self.reads.borrow().clone() + self.reads.lock().unwrap().clone() } } impl ExpanderInterface for TestSpyInterface { fn write_register(&mut self, addr: RegisterAddress, value: u8) -> Result<(), ()> { - let mut regs = self.registers.borrow_mut(); + let mut regs = self.registers.lock().unwrap(); let enc_addr = u8::from(addr); assert!(enc_addr <= 0x5F); match regs[enc_addr as usize] { @@ -204,8 +203,8 @@ pub(crate) mod test_spy { Ok(()) } fn read_register(&mut self, addr: RegisterAddress) -> Result { - self.reads.borrow_mut().push(addr.into()); - let regs = self.registers.borrow(); + self.reads.lock().unwrap().push(addr.into()); + let regs = self.registers.lock().unwrap(); let enc_addr = u8::from(addr); assert!(enc_addr <= 0x5F); match regs[enc_addr as usize] { @@ -240,14 +239,14 @@ pub(crate) mod test_spy { } pub struct SemanticTestSpyInterface { - ports: Rc>>, + ports: Arc>>, } impl SemanticTestSpyInterface { pub fn new(init: Vec) -> Self { assert!(init.len() == 32 - 4); Self { - ports: Rc::new(RefCell::new( + ports: Arc::new(Mutex::new( init.into_iter() .map(|b| TestPort::Reset(b)) .collect::>(), @@ -264,7 +263,8 @@ pub(crate) mod test_spy { pub fn peek_all(&self) -> Vec { use self::TestPort::*; self.ports - .borrow() + .lock() + .unwrap() .iter() .cloned() .map(|v| match v { @@ -277,7 +277,8 @@ pub(crate) mod test_spy { pub fn peek_bits(&self) -> Vec { use self::TestPort::*; self.ports - .borrow() + .lock() + .unwrap() .iter() .cloned() .map(|v| match v { @@ -289,7 +290,7 @@ pub(crate) mod test_spy { fn write_port(&self, port: u8, bit: bool) { use self::TestPort::*; let idx = port as usize - 4; - let slot_ref = &mut self.ports.borrow_mut()[idx]; + let slot_ref = &mut self.ports.lock().unwrap()[idx]; *slot_ref = match slot_ref { Reset(_) | BlindWrite(_) => BlindWrite(bit), Read(_) | ReadWrite(_) => ReadWrite(bit), @@ -299,7 +300,7 @@ pub(crate) mod test_spy { fn read_port(&self, port: u8) -> bool { use self::TestPort::*; let idx = port as usize - 4; - let slot_ref = &mut self.ports.borrow_mut()[idx]; + let slot_ref = &mut self.ports.lock().unwrap()[idx]; let (upd, ret) = match slot_ref { Reset(b) | Read(b) => (Read(*b), *b), ReadWrite(b) => (ReadWrite(*b), *b), @@ -316,7 +317,8 @@ pub(crate) mod test_spy { f, "[ {} ]", self.ports - .borrow() + .lock() + .unwrap() .iter() .map(|&v| format!("{:?}", v)) .collect::>()