Check DMA descriptors and buffers addresses (#1670)

* Check DMA descriptors and buffers addresses

* Add PR id

* Add test for the memory region check

* Clippy

esp-hal/CHANGELOG.md

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - uart: Added `UartTx` and `UartRx` constructors (#1592)
 
 - Add Flex / AnyFlex GPIO pin driver (#1659)
+- Add new `DmaError::UnsupportedMemoryRegion` - used memory regions are checked when preparing a transfer now (#1670)
 
 ### Fixed
 

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(

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);
@@ -900,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,
@@ -958,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,
@@ -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(data.add(len) as u32)
+        {
+            return Err(DmaError::UnsupportedMemoryRegion);
+        }
+
         descriptors.fill(DmaDescriptor::EMPTY);
 
         compiler_fence(core::sync::atomic::Ordering::SeqCst);
@@ -1144,7 +1166,7 @@ where
         R::init_channel();
     }
 
-    fn prepare_transfer_without_start(
+    unsafe fn prepare_transfer_without_start(
         &mut self,
         peri: DmaPeripheral,
         circular: bool,

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
 

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(())

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() {

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);

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,

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
+            ))
+        ));
+    }
 }