Test -Zstack-protector (#3636)

* Test -Zstack-protector

* Pass config as inline TOML to cargo

* Try to fix failing test

.github/workflows/hil.yml

@@ -108,6 +108,16 @@ jobs:
           target: ${{ matrix.target.rust-target }}
           toolchain: stable
           components: rust-src
+
+        # -Zstack-protector=all tests need to be compiled with nightly.
+      - if: ${{ !contains(fromJson('["esp32", "esp32s2", "esp32s3"]'), matrix.target.soc) }}
+        name: Install nightly Rust toolchain
+        uses: dtolnay/rust-toolchain@v1
+        with:
+          target: ${{ matrix.target.rust-target }}
+          toolchain: nightly
+          components: rust-src
+
       # Install the Rust toolchain for Xtensa devices:
       - if: contains(fromJson('["esp32", "esp32s2", "esp32s3"]'), matrix.target.soc)
         uses: esp-rs/xtensa-toolchain@v1.5

esp-hal/ld/esp32s3/memory.x

@@ -22,12 +22,12 @@ MEMORY
 {
   vectors_seg ( RX )     : ORIGIN = 0x40370000 + RESERVE_ICACHE, len = VECTORS_SIZE
   iram_seg ( RX )        : ORIGIN = 0x40370000 + RESERVE_ICACHE + VECTORS_SIZE, len = 328k - VECTORS_SIZE - RESERVE_ICACHE
-  dram_seg ( RW )        : ORIGIN = 0x3FC88000 , len = 345856 
+  dram_seg ( RW )        : ORIGIN = 0x3FC88000 , len = 345856
 
   /* memory available after the 2nd stage bootloader is finished */
   dram2_seg ( RW )       : ORIGIN = ORIGIN(dram_seg) + LENGTH(dram_seg), len = 0x3fced710 - (ORIGIN(dram_seg) + LENGTH(dram_seg))
 
-  /* external flash 
+  /* external flash
      The 0x20 offset is a convenience for the app binary image generation.
      Flash cache has 64KB pages. The .bin file which is flashed to the chip
      has a 0x18 byte file header, and each segment has a 0x08 byte segment

hil-test/.cargo/config.toml

@@ -6,7 +6,7 @@ rustflags = [
     "-C", "link-arg=-Tembedded-test.x",
     "-C", "link-arg=-Tdefmt.x",
     "-C", "link-arg=-Tlinkall.x",
-    "-C", "force-frame-pointers"
+    "-C", "force-frame-pointers",
 ]
 
 [target.'cfg(target_arch = "xtensa")']

hil-test/Cargo.toml

@@ -128,6 +128,10 @@ harness = false
 name    = "storage_read_app_desc"
 harness = false
 
+[[test]]
+name    = "stack_protector"
+harness = false
+
 [[test]]
 name    = "parl_io"
 harness = false

hil-test/tests/stack_protector.rs

@@ -0,0 +1,63 @@
+//! Tests that the `-Zstack-protector=all` works as expected.
+//!
+//! The stack protector is enabled by setting HIL_ENABLE_STACK_PROTECTOR. The
+//! xtask recognizes this and sets the cargo config to `.cargo/config_spp.toml`,
+//! which enables the feature.
+
+//% CARGO-CONFIG: target.'cfg(target_arch = "riscv32")'.rustflags = [ "-Z", "stack-protector=all" ]
+//% CARGO-CONFIG: target.'cfg(target_arch = "xtensa")'.rustflags = [ "-Z", "stack-protector=all" ]
+//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
+//% FEATURES: esp-alloc
+
+#![no_std]
+#![no_main]
+
+use hil_test as _;
+
+#[inline(never)]
+fn trigger_overflow() {
+    // Aim for the middle of heap: these are roughly DRAM_LEN - 16k
+    const SIZE: usize = if cfg!(esp32) {
+        160 * 1024
+    } else if cfg!(esp32c2) {
+        176 * 1024
+    } else if cfg!(esp32c3) {
+        297 * 1024
+    } else if cfg!(esp32c6) {
+        425 * 1024
+    } else if cfg!(esp32h2) {
+        235 * 1024
+    } else if cfg!(esp32s2) {
+        173 * 1024
+    } else if cfg!(esp32s3) {
+        322 * 1024
+    } else {
+        unreachable!()
+    };
+    let mut stack = core::hint::black_box([0u8; SIZE]);
+    stack[SIZE - 1] = 42;
+}
+
+#[cfg(test)]
+#[embedded_test::tests(default_timeout = 3)]
+mod tests {
+    use super::*;
+
+    #[init]
+    fn init() {
+        let _ = esp_hal::init(esp_hal::Config::default());
+        // Have some data that we can overflow into.
+        esp_alloc::heap_allocator!(size: 32 * 1024);
+    }
+
+    #[test]
+    fn should_be_ok() {
+        assert_eq!(1 + 1, 2);
+    }
+
+    #[test]
+    #[should_panic]
+    fn should_trigger_panic() {
+        trigger_overflow();
+    }
+}

hil-test/tests/uart_tx_rx_async.rs

@@ -160,11 +160,14 @@ mod tests {
         let mut read = [0u8; 128];
         let read = async {
             // This read should return as many bytes as the FIFO threshold, which is 120
-            // bytes by default.
+            // bytes by default. Allow for inequality in case processing is held up a bit.
             let read_count = ctx.rx.read_async(&mut read).await.unwrap();
-            assert_eq!(read_count, 120);
+            assert!(read_count >= 120);
 
-            ctx.rx.read_exact_async(&mut read[120..]).await.unwrap();
+            ctx.rx
+                .read_exact_async(&mut read[read_count..])
+                .await
+                .unwrap();
             assert_eq!(&read, &[1; 128]);
         };
         let write = async { ctx.tx.write_all(&[1; 128]).await.unwrap() };

xtask/README.md

@@ -115,6 +115,19 @@ One environment variable is specified in a single line. The name and value are s
 This key is additive. The unnamed list is added to named lists, and multiple lists with the
 same name are merged.
 
+### `//% CARGO-CONFIG`
+
+The value of this key will be passed as a `--config` argument to `cargo`. Any amount
+of configuration can be specfied this way.
+
+```
+//% CARGO-CONFIG: target.'cfg(target_arch = "riscv32")'.rustflags = [ "-Z", "stack-protector=all" ]
+//% CARGO-CONFIG: target.'cfg(target_arch = "xtensa")'.rustflags = [ "-Z", "stack-protector=all" ]
+```
+
+This key is additive. The unnamed list is added to named lists, and multiple lists with the
+same name are merged.
+
 ### Working with multiple metadata configurations
 
 Processing a file will create one configuration, or however many names (that is, the list of

xtask/src/firmware.rs

@@ -22,6 +22,7 @@ pub struct Metadata {
     tag: Option<String>,
     description: Option<String>,
     env_vars: HashMap<String, String>,
+    cargo_config: Vec<String>,
 }
 
 impl Metadata {
@@ -72,6 +73,11 @@ impl Metadata {
         &self.env_vars
     }
 
+    /// A list of all cargo `--config` values to use.
+    pub fn cargo_config(&self) -> &[String] {
+        &self.cargo_config
+    }
+
     /// If the specified chip is in the list of chips, then it is supported.
     pub fn supports_chip(&self, chip: Chip) -> bool {
         self.chip == chip
@@ -100,6 +106,7 @@ impl Metadata {
 pub struct Configuration {
     chips: Vec<Chip>,
     name: String,
+    cargo_config: Vec<String>,
     features: Vec<String>,
     esp_config: HashMap<String, String>,
     tag: Option<String>,
@@ -227,6 +234,11 @@ pub fn load(path: &Path) -> Result<Vec<Metadata>> {
                         .collect::<Vec<_>>();
                     relevant_metadata.apply(|meta| meta.chips = chips.clone());
                 }
+                // A list of cargo `--config` configurations.
+                "CARGO-CONFIG" => {
+                    relevant_metadata
+                        .apply(|meta| meta.cargo_config.push(meta_line.value.to_string()));
+                }
                 // Cargo features to enable for the current configuration.
                 "FEATURES" => {
                     let mut values = meta_line
@@ -278,6 +290,8 @@ pub fn load(path: &Path) -> Result<Vec<Metadata>> {
             // Other values are merged
             meta.features.extend_from_slice(&all_configuration.features);
             meta.esp_config.extend(all_configuration.esp_config.clone());
+            meta.cargo_config
+                .extend(all_configuration.cargo_config.clone());
         }
 
         // If no configurations are specified, fall back to the unnamed one. Otherwise
@@ -304,6 +318,7 @@ pub fn load(path: &Path) -> Result<Vec<Metadata>> {
                     features: configuration.features.clone(),
                     tag: configuration.tag.clone(),
                     env_vars: configuration.esp_config.clone(),
+                    cargo_config: configuration.cargo_config.clone(),
                 })
             }
         }

xtask/src/lib.rs

@@ -347,6 +347,14 @@ pub fn execute_app(
     };
     builder = builder.subcommand(subcommand);
 
+    for config in app.cargo_config() {
+        log::info!(" Cargo --config: {config}");
+        builder.add_arg("--config").add_arg(config);
+        // Some configuration requires nightly rust, so let's just assume it. May be
+        // overwritten by the esp toolchain on xtensa.
+        builder = builder.toolchain("nightly");
+    }
+
     if !debug {
         builder.add_arg("--release");
     }
@@ -354,7 +362,6 @@ pub fn execute_app(
     // If targeting an Xtensa device, we must use the '+esp' toolchain modifier:
     if target.starts_with("xtensa") {
         builder = builder.toolchain("esp");
-        builder.add_arg("-Zbuild-std=core,alloc");
     }
 
     let args = builder.build();