From 2da917c4cfa1e5bdf8892ee70c5bc33a8105e018 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Vi=C3=B6l?= Date: Tue, 21 Nov 2023 12:08:17 +0100 Subject: [PATCH 1/5] Fix undefined behavior compile error --- Cargo.toml | 1 + src/spi.rs | 16 ++++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 849ad150..55b64fd8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,6 +37,7 @@ embedded-hal = { version = "0.2.5", features = ["unproven"] } embedded-time = "0.12.0" enumset = { version = "1.0.6", optional = true } nb = "1.0.0" +num-traits = { version = "0.2.17", default-features = false} paste = "1.0.5" rtcc = { version = "0.3.0", optional = true } stm32-usbd = { version = "0.6.0", optional = true } diff --git a/src/spi.rs b/src/spi.rs index 2942e7fb..12a3219c 100644 --- a/src/spi.rs +++ b/src/spi.rs @@ -24,6 +24,7 @@ use crate::{ rcc::{self, Clocks}, time::rate, }; +use num_traits::{AsPrimitive, PrimInt}; /// SPI error #[derive(Copy, Clone, PartialEq, Eq, Debug)] @@ -337,6 +338,8 @@ where // SckPin could technically be omitted, though not advisable. Miso: MisoPin, Mosi: MosiPin, + Word: PrimInt + Into + 'static, + u32: AsPrimitive, { type Error = Error; @@ -350,10 +353,8 @@ where } else if sr.crcerr().is_no_match() { nb::Error::Other(Error::Crc) } else if sr.rxne().is_not_empty() { - let read_ptr = &self.spi.dr as *const _ as *const Word; // NOTE(unsafe) read from register owned by this Spi struct - let value = unsafe { ptr::read_volatile(read_ptr) }; - return Ok(value); + return Ok(self.spi.dr.read().bits().as_()); } else { nb::Error::WouldBlock }) @@ -369,9 +370,8 @@ where } else if sr.crcerr().is_no_match() { nb::Error::Other(Error::Crc) } else if sr.txe().is_empty() { - let write_ptr = &self.spi.dr as *const _ as *mut Word; - // NOTE(unsafe) write to register owned by this Spi struct - unsafe { ptr::write_volatile(write_ptr, word) }; + // NOTE(unsafe): this is the data register, where unsafe writes are ok. + self.spi.dr.write(|w| unsafe { w.bits(word.into()) }); return Ok(()); } else { nb::Error::WouldBlock @@ -384,6 +384,8 @@ where SPI: Instance, Miso: MisoPin, Mosi: MosiPin, + Word: PrimInt + Into + 'static, + u32: AsPrimitive, { } @@ -392,6 +394,8 @@ where SPI: Instance, Miso: MisoPin, Mosi: MosiPin, + Word: PrimInt + Into + 'static, + u32: AsPrimitive, { } From 14e0c0e2f0e7ed39d6bb8e80c076ddc0bdf9a559 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Vi=C3=B6l?= Date: Wed, 22 Nov 2023 07:29:57 +0100 Subject: [PATCH 2/5] WIP: Use casts again? --- CHANGELOG.md | 4 ++++ src/spi.rs | 10 ++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32d6df1f..d58c6c7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,10 +22,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Deprecate `Toggle` enum and use `Switch` instead for better naming purposes ([#334]) - Add `impl From for Switch` to reduce churn. +- Fix undefined behavior in SPI implementation ([#346]) + - Add `num_traits::PrimInt` bounds to `Word` ### Added - Add missing ADC channels ([#337]) +- Add many `#[must_use]` instances ([#346]) ### Fixed @@ -595,6 +598,7 @@ let clocks = rcc [defmt]: https://github.com/knurling-rs/defmt [filter]: https://defmt.ferrous-systems.com/filtering.html +[#346]: https://github.com/stm32-rs/stm32f3xx-hal/pull/346 [#340]: https://github.com/stm32-rs/stm32f3xx-hal/pull/340 [#338]: https://github.com/stm32-rs/stm32f3xx-hal/pull/338 [#337]: https://github.com/stm32-rs/stm32f3xx-hal/pull/337 diff --git a/src/spi.rs b/src/spi.rs index 12a3219c..f9673d8c 100644 --- a/src/spi.rs +++ b/src/spi.rs @@ -353,8 +353,9 @@ where } else if sr.crcerr().is_no_match() { nb::Error::Other(Error::Crc) } else if sr.rxne().is_not_empty() { - // NOTE(unsafe) read from register owned by this Spi struct - return Ok(self.spi.dr.read().bits().as_()); + let read_ptr = core::ptr::addr_of!(self.spi.dr) as *const Word; + let value = unsafe { core::ptr::read_volatile(read_ptr) }; + return Ok(value); } else { nb::Error::WouldBlock }) @@ -370,8 +371,9 @@ where } else if sr.crcerr().is_no_match() { nb::Error::Other(Error::Crc) } else if sr.txe().is_empty() { - // NOTE(unsafe): this is the data register, where unsafe writes are ok. - self.spi.dr.write(|w| unsafe { w.bits(word.into()) }); + let write_ptr = core::ptr::addr_of!(self.spi.dr) as *mut Word; + // NOTE(unsafe) write to register owned by this Spi struct + unsafe { core::ptr::write_volatile(write_ptr, word) }; return Ok(()); } else { nb::Error::WouldBlock From 1734f28ab6a4f1e0106cb58a037317441dab0749 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Vi=C3=B6l?= Date: Wed, 22 Nov 2023 09:13:35 +0100 Subject: [PATCH 3/5] Remove unrelated CHANGELOG entry --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d58c6c7b..e03f4791 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Added - Add missing ADC channels ([#337]) -- Add many `#[must_use]` instances ([#346]) ### Fixed From 121cd0c758b4a03a194cf929ad3d3400343e9811 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Vi=C3=B6l?= Date: Wed, 22 Nov 2023 09:17:13 +0100 Subject: [PATCH 4/5] fixup! Remove unrelated CHANGELOG entry --- src/spi.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spi.rs b/src/spi.rs index f9673d8c..4d988764 100644 --- a/src/spi.rs +++ b/src/spi.rs @@ -6,7 +6,7 @@ //! //! [examples/spi.rs]: https://github.com/stm32-rs/stm32f3xx-hal/blob/v0.9.1/examples/spi.rs -use core::{fmt, marker::PhantomData, ops::Deref, ptr}; +use core::{fmt, marker::PhantomData, ops::Deref}; use crate::hal::blocking::spi; use crate::hal::spi::FullDuplex; From 4d5377dff009be0418164e6fb7d2f78a07cdd04e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Vi=C3=B6l?= Date: Wed, 22 Nov 2023 22:01:00 +0100 Subject: [PATCH 5/5] Add safety note --- src/spi.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/spi.rs b/src/spi.rs index 4d988764..13932263 100644 --- a/src/spi.rs +++ b/src/spi.rs @@ -354,6 +354,7 @@ where nb::Error::Other(Error::Crc) } else if sr.rxne().is_not_empty() { let read_ptr = core::ptr::addr_of!(self.spi.dr) as *const Word; + // SAFETY: Read from register owned by this Spi struct let value = unsafe { core::ptr::read_volatile(read_ptr) }; return Ok(value); } else { @@ -372,7 +373,7 @@ where nb::Error::Other(Error::Crc) } else if sr.txe().is_empty() { let write_ptr = core::ptr::addr_of!(self.spi.dr) as *mut Word; - // NOTE(unsafe) write to register owned by this Spi struct + // SAFETY: Write to register owned by this Spi struct unsafe { core::ptr::write_volatile(write_ptr, word) }; return Ok(()); } else {