From a2eafbf8fefa3742a8b5a21fe83d75d1e1684ad6 Mon Sep 17 00:00:00 2001 From: bjoernQ Date: Tue, 11 Jun 2024 12:59:39 +0200 Subject: [PATCH 1/4] Check DMA descriptors and buffers addresses --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/dma/mod.rs | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 04e01b242d5..f6359d50a81 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add Flex / AnyFlex GPIO pin driver (#1659) +- Add new `DmaError::UnsupportedMemoryRegion` - used memory regions are checked when preparing a transfer now ### Fixed diff --git a/esp-hal/src/dma/mod.rs b/esp-hal/src/dma/mod.rs index 882fb10df30..1500a3dc0bb 100644 --- a/esp-hal/src/dma/mod.rs +++ b/esp-hal/src/dma/mod.rs @@ -323,6 +323,8 @@ pub enum DmaError { Exhausted, /// The given buffer is too small BufferTooSmall, + /// Descriptors or buffers are not located in a supported memory region + UnsupportedMemoryRegion, } /// DMA Priorities @@ -538,6 +540,16 @@ where data: *mut u8, len: usize, ) -> Result<(), DmaError> { + if !crate::soc::is_valid_ram_address(descriptors.as_ptr() as u32) + || !crate::soc::is_valid_ram_address(core::ptr::addr_of!( + descriptors[descriptors.len() - 1] + ) as u32) + || !crate::soc::is_valid_ram_address(data as u32) + || !crate::soc::is_valid_ram_address(data.add(len) as u32) + { + return Err(DmaError::UnsupportedMemoryRegion); + } + descriptors.fill(DmaDescriptor::EMPTY); compiler_fence(core::sync::atomic::Ordering::SeqCst); @@ -966,6 +978,16 @@ where data: *const u8, len: usize, ) -> Result<(), DmaError> { + if !crate::soc::is_valid_ram_address(descriptors.as_ptr() as u32) + || !crate::soc::is_valid_ram_address(core::ptr::addr_of!( + descriptors[descriptors.len() - 1] + ) as u32) + || !crate::soc::is_valid_ram_address(data as u32) + || !crate::soc::is_valid_ram_address(unsafe { data.add(len) } as u32) + { + return Err(DmaError::UnsupportedMemoryRegion); + } + descriptors.fill(DmaDescriptor::EMPTY); compiler_fence(core::sync::atomic::Ordering::SeqCst); From dd5e0e8943e54f44b3117376e7077c4ff7f3cd29 Mon Sep 17 00:00:00 2001 From: bjoernQ Date: Tue, 11 Jun 2024 13:52:22 +0200 Subject: [PATCH 2/4] Add PR id --- esp-hal/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index f6359d50a81..1ee4b9747b6 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add Flex / AnyFlex GPIO pin driver (#1659) -- Add new `DmaError::UnsupportedMemoryRegion` - used memory regions are checked when preparing a transfer now +- Add new `DmaError::UnsupportedMemoryRegion` - used memory regions are checked when preparing a transfer now (#1670) ### Fixed From 00536d3d8a84f68bb19675d9662556c2377d65de Mon Sep 17 00:00:00 2001 From: bjoernQ Date: Tue, 11 Jun 2024 13:52:48 +0200 Subject: [PATCH 3/4] Add test for the memory region check --- hil-test/tests/spi_full_duplex_dma.rs | 97 +++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/hil-test/tests/spi_full_duplex_dma.rs b/hil-test/tests/spi_full_duplex_dma.rs index 826c36be651..e9568aae372 100644 --- a/hil-test/tests/spi_full_duplex_dma.rs +++ b/hil-test/tests/spi_full_duplex_dma.rs @@ -170,4 +170,101 @@ mod tests { transfer.wait().unwrap(); assert_eq!(send, receive); } + + #[test] + #[timeout(3)] + fn test_try_using_non_dma_memory_tx_buffer() { + const DMA_BUFFER_SIZE: usize = 4096; + + let peripherals = Peripherals::take(); + let system = SystemControl::new(peripherals.SYSTEM); + let clocks = ClockControl::boot_defaults(system.clock_control).freeze(); + + let io = Io::new(peripherals.GPIO, peripherals.IO_MUX); + let sclk = io.pins.gpio0; + let miso = io.pins.gpio2; + let mosi = io.pins.gpio4; + let cs = io.pins.gpio5; + + let dma = Dma::new(peripherals.DMA); + + #[cfg(any(feature = "esp32", feature = "esp32s2"))] + let dma_channel = dma.spi2channel; + #[cfg(not(any(feature = "esp32", feature = "esp32s2")))] + let dma_channel = dma.channel0; + + let (_, mut tx_descriptors, rx_buffer, mut rx_descriptors) = dma_buffers!(DMA_BUFFER_SIZE); + + let tx_buffer = { + // using `static`, not `static mut`, places the array in .rodata + static TX_BUFFER: [u8; DMA_BUFFER_SIZE] = [42u8; DMA_BUFFER_SIZE]; + unsafe { &mut *(core::ptr::addr_of!(TX_BUFFER) as *mut u8) } + }; + + let mut spi = Spi::new(peripherals.SPI2, 100.kHz(), SpiMode::Mode0, &clocks) + .with_pins(Some(sclk), Some(mosi), Some(miso), Some(cs)) + .with_dma(dma_channel.configure( + false, + &mut tx_descriptors, + &mut rx_descriptors, + DmaPriority::Priority0, + )); + + let mut receive = rx_buffer; + + assert!(matches!( + spi.dma_transfer(&tx_buffer, &mut receive), + Err(esp_hal::spi::Error::DmaError( + esp_hal::dma::DmaError::UnsupportedMemoryRegion + )) + )); + } + + #[test] + #[timeout(3)] + fn test_try_using_non_dma_memory_rx_buffer() { + const DMA_BUFFER_SIZE: usize = 4096; + + let peripherals = Peripherals::take(); + let system = SystemControl::new(peripherals.SYSTEM); + let clocks = ClockControl::boot_defaults(system.clock_control).freeze(); + + let io = Io::new(peripherals.GPIO, peripherals.IO_MUX); + let sclk = io.pins.gpio0; + let miso = io.pins.gpio2; + let mosi = io.pins.gpio4; + let cs = io.pins.gpio5; + + let dma = Dma::new(peripherals.DMA); + + #[cfg(any(feature = "esp32", feature = "esp32s2"))] + let dma_channel = dma.spi2channel; + #[cfg(not(any(feature = "esp32", feature = "esp32s2")))] + let dma_channel = dma.channel0; + + let (tx_buffer, mut tx_descriptors, _, mut rx_descriptors) = dma_buffers!(DMA_BUFFER_SIZE); + + let rx_buffer = { + // using `static`, not `static mut`, places the array in .rodata + static RX_BUFFER: [u8; DMA_BUFFER_SIZE] = [42u8; DMA_BUFFER_SIZE]; + unsafe { &mut *(core::ptr::addr_of!(RX_BUFFER) as *mut u8) } + }; + + let mut spi = Spi::new(peripherals.SPI2, 100.kHz(), SpiMode::Mode0, &clocks) + .with_pins(Some(sclk), Some(mosi), Some(miso), Some(cs)) + .with_dma(dma_channel.configure( + false, + &mut tx_descriptors, + &mut rx_descriptors, + DmaPriority::Priority0, + )); + + let mut receive = rx_buffer; + assert!(matches!( + spi.dma_transfer(&tx_buffer, &mut receive), + Err(esp_hal::spi::Error::DmaError( + esp_hal::dma::DmaError::UnsupportedMemoryRegion + )) + )); + } } From 21fb247da78f88bfc0551776defea553113bf639 Mon Sep 17 00:00:00 2001 From: bjoernQ Date: Tue, 11 Jun 2024 13:53:42 +0200 Subject: [PATCH 4/4] Clippy --- esp-hal/src/aes/mod.rs | 19 ++++++++-------- esp-hal/src/dma/mod.rs | 8 +++---- esp-hal/src/i2s.rs | 37 +++++++++++++++++++++---------- esp-hal/src/lcd_cam/lcd/i8080.rs | 14 +++++++----- esp-hal/src/parl_io.rs | 8 ++++--- esp-hal/src/spi/master.rs | 38 ++++++++++++++++++++------------ esp-hal/src/spi/slave.rs | 10 +++++---- 7 files changed, 82 insertions(+), 52 deletions(-) diff --git a/esp-hal/src/aes/mod.rs b/esp-hal/src/aes/mod.rs index d5d9b9538a7..20e56b9311a 100644 --- a/esp-hal/src/aes/mod.rs +++ b/esp-hal/src/aes/mod.rs @@ -412,16 +412,17 @@ pub mod dma { self.channel.tx.is_done(); self.channel.rx.is_done(); - self.channel - .tx - .prepare_transfer_without_start( - self.dma_peripheral(), - false, - write_buffer_ptr, - write_buffer_len, - ) - .and_then(|_| self.channel.tx.start_transfer())?; unsafe { + self.channel + .tx + .prepare_transfer_without_start( + self.dma_peripheral(), + false, + write_buffer_ptr, + write_buffer_len, + ) + .and_then(|_| self.channel.tx.start_transfer())?; + self.channel .rx .prepare_transfer_without_start( diff --git a/esp-hal/src/dma/mod.rs b/esp-hal/src/dma/mod.rs index 1500a3dc0bb..e9891a38aa5 100644 --- a/esp-hal/src/dma/mod.rs +++ b/esp-hal/src/dma/mod.rs @@ -912,7 +912,7 @@ pub trait TxPrivate: crate::private::Sealed { fn init_channel(&mut self); - fn prepare_transfer_without_start( + unsafe fn prepare_transfer_without_start( &mut self, peri: DmaPeripheral, circular: bool, @@ -970,7 +970,7 @@ where R::set_out_priority(priority); } - fn prepare_transfer_without_start( + unsafe fn prepare_transfer_without_start( &mut self, descriptors: &mut [DmaDescriptor], circular: bool, @@ -983,7 +983,7 @@ where descriptors[descriptors.len() - 1] ) as u32) || !crate::soc::is_valid_ram_address(data as u32) - || !crate::soc::is_valid_ram_address(unsafe { data.add(len) } as u32) + || !crate::soc::is_valid_ram_address(data.add(len) as u32) { return Err(DmaError::UnsupportedMemoryRegion); } @@ -1166,7 +1166,7 @@ where R::init_channel(); } - fn prepare_transfer_without_start( + unsafe fn prepare_transfer_without_start( &mut self, peri: DmaPeripheral, circular: bool, diff --git a/esp-hal/src/i2s.rs b/esp-hal/src/i2s.rs index a81d65b3f66..7f782e6bb09 100644 --- a/esp-hal/src/i2s.rs +++ b/esp-hal/src/i2s.rs @@ -500,9 +500,11 @@ where // Enable corresponding interrupts if needed // configure DMA outlink - self.tx_channel - .prepare_transfer_without_start(T::get_dma_peripheral(), false, ptr, data.len()) - .and_then(|_| self.tx_channel.start_transfer())?; + unsafe { + self.tx_channel + .prepare_transfer_without_start(T::get_dma_peripheral(), false, ptr, data.len()) + .and_then(|_| self.tx_channel.start_transfer())?; + } // set I2S_TX_STOP_EN if needed @@ -532,9 +534,11 @@ where // Enable corresponding interrupts if needed // configure DMA outlink - self.tx_channel - .prepare_transfer_without_start(T::get_dma_peripheral(), circular, ptr, len) - .and_then(|_| self.tx_channel.start_transfer())?; + unsafe { + self.tx_channel + .prepare_transfer_without_start(T::get_dma_peripheral(), circular, ptr, len) + .and_then(|_| self.tx_channel.start_transfer())?; + } // set I2S_TX_STOP_EN if needed @@ -2113,9 +2117,16 @@ pub mod asynch { T::reset_tx(); let future = DmaTxFuture::new(&mut self.tx_channel); - future - .tx - .prepare_transfer_without_start(T::get_dma_peripheral(), false, ptr, len)?; + + unsafe { + future.tx.prepare_transfer_without_start( + T::get_dma_peripheral(), + false, + ptr, + len, + )?; + } + future.tx.start_transfer()?; T::tx_start(); future.await?; @@ -2138,9 +2149,11 @@ pub mod asynch { // Enable corresponding interrupts if needed // configure DMA outlink - self.tx_channel - .prepare_transfer_without_start(T::get_dma_peripheral(), true, ptr, len) - .and_then(|_| self.tx_channel.start_transfer())?; + unsafe { + self.tx_channel + .prepare_transfer_without_start(T::get_dma_peripheral(), true, ptr, len) + .and_then(|_| self.tx_channel.start_transfer())?; + } // set I2S_TX_STOP_EN if needed diff --git a/esp-hal/src/lcd_cam/lcd/i8080.rs b/esp-hal/src/lcd_cam/lcd/i8080.rs index c6263bc968d..8150bb21723 100644 --- a/esp-hal/src/lcd_cam/lcd/i8080.rs +++ b/esp-hal/src/lcd_cam/lcd/i8080.rs @@ -461,12 +461,14 @@ impl<'d, TX: Tx, P> I8080<'d, TX, P> { .set_bit() }); - self.tx_channel.prepare_transfer_without_start( - DmaPeripheral::LcdCam, - false, - ptr, - len, - )?; + unsafe { + self.tx_channel.prepare_transfer_without_start( + DmaPeripheral::LcdCam, + false, + ptr, + len, + )?; + } self.tx_channel.start_transfer()?; } Ok(()) diff --git a/esp-hal/src/parl_io.rs b/esp-hal/src/parl_io.rs index 698f9dd349d..25a433c69de 100644 --- a/esp-hal/src/parl_io.rs +++ b/esp-hal/src/parl_io.rs @@ -1474,9 +1474,11 @@ where self.tx_channel.is_done(); - self.tx_channel - .prepare_transfer_without_start(DmaPeripheral::ParlIo, false, ptr, len) - .and_then(|_| self.tx_channel.start_transfer())?; + unsafe { + self.tx_channel + .prepare_transfer_without_start(DmaPeripheral::ParlIo, false, ptr, len) + .and_then(|_| self.tx_channel.start_transfer())?; + } loop { if Instance::is_tx_ready() { diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index b9eb455b454..0eb6a7310c4 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1077,8 +1077,10 @@ pub mod dma { return Err(super::Error::MaxDmaTransferSizeExceeded); } - self.spi - .start_write_bytes_dma(ptr, len, &mut self.channel.tx, false)?; + unsafe { + self.spi + .start_write_bytes_dma(ptr, len, &mut self.channel.tx, false)?; + } Ok(DmaTransferTx::new(self)) } @@ -1293,8 +1295,10 @@ pub mod dma { .modify(|_, w| unsafe { w.usr_dummy_cyclelen().bits(dummy - 1) }); } - self.spi - .start_write_bytes_dma(ptr, len, &mut self.channel.tx, false)?; + unsafe { + self.spi + .start_write_bytes_dma(ptr, len, &mut self.channel.tx, false)?; + } Ok(DmaTransferTx::new(self)) } } @@ -1419,12 +1423,14 @@ pub mod dma { async fn write(&mut self, words: &[u8]) -> Result<(), Self::Error> { for chunk in words.chunks(MAX_DMA_SIZE) { let mut future = crate::dma::asynch::DmaTxFuture::new(&mut self.channel.tx); - self.spi.start_write_bytes_dma( - chunk.as_ptr(), - chunk.len(), - future.tx(), - true, - )?; + unsafe { + self.spi.start_write_bytes_dma( + chunk.as_ptr(), + chunk.len(), + future.tx(), + true, + )?; + } future.await?; self.spi.flush()?; @@ -1870,7 +1876,9 @@ where fn write_bytes_dma<'w>(&mut self, words: &'w [u8], tx: &mut TX) -> Result<&'w [u8], Error> { for chunk in words.chunks(MAX_DMA_SIZE) { - self.start_write_bytes_dma(chunk.as_ptr(), chunk.len(), tx, false)?; + unsafe { + self.start_write_bytes_dma(chunk.as_ptr(), chunk.len(), tx, false)?; + } while !tx.is_done() {} self.flush().unwrap(); // seems "is_done" doesn't work as intended? @@ -1880,7 +1888,7 @@ where } #[cfg_attr(feature = "place-spi-driver-in-ram", ram)] - fn start_write_bytes_dma( + unsafe fn start_write_bytes_dma( &mut self, ptr: *const u8, len: usize, @@ -1896,8 +1904,10 @@ where self.update(); reset_dma_before_load_dma_dscr(reg_block); - tx.prepare_transfer_without_start(self.dma_peripheral(), false, ptr, len) - .and_then(|_| tx.start_transfer())?; + unsafe { + tx.prepare_transfer_without_start(self.dma_peripheral(), false, ptr, len) + .and_then(|_| tx.start_transfer())?; + } self.clear_dma_interrupts(); reset_dma_before_usr_cmd(reg_block); diff --git a/esp-hal/src/spi/slave.rs b/esp-hal/src/spi/slave.rs index fc3d28b88ad..c24c772fd2a 100644 --- a/esp-hal/src/spi/slave.rs +++ b/esp-hal/src/spi/slave.rs @@ -325,9 +325,11 @@ pub mod dma { return Err(Error::MaxDmaTransferSizeExceeded); } - self.spi - .start_write_bytes_dma(ptr, len, &mut self.channel.tx) - .map(move |_| DmaTransferTx::new(self)) + unsafe { + self.spi + .start_write_bytes_dma(ptr, len, &mut self.channel.tx) + .map(move |_| DmaTransferTx::new(self)) + } } /// Register a buffer for a DMA read. @@ -449,7 +451,7 @@ where Ok(()) } - fn start_write_bytes_dma( + unsafe fn start_write_bytes_dma( &mut self, ptr: *const u8, len: usize,