fix image creation

1. nonce should be generated from the LSB of the commit hash.
security doesn't come from the nonce, and having the commit hash
in the nonce is helpful for version control of the swap image
(although it doesn't capture "dirty" builds, e.g. builds
made with uncommitted changes).

2. fix IniS regions to not require special alignment. Since the
data is copied (similar to IniE), we don't have to waste space
page-aligning things so that offsets map perfectly between
the image on disk and in RAM.

3. add checks that the loading position of data inside the
swap image matches our expectations. We should have 0x1000
bytes of unencrypted header, then 0x1000 bytes of XArgs + padding,
and then image data should start.

There is a "just so" coincidence in that the image data by
the MiniElf program always lines up at an address > 0x1000, making
space for the XArgs. I actually don't know why this padding is
there, but the "section" doesn't decode to anything we
can parse. Maybe it's some reserved space for OS-specific
loader shenanigans?

tools/src/bin/create-image.rs

@@ -81,7 +81,7 @@ fn csr_to_config(hv: tools::utils::CsrConfig, ram_config: &mut RamConfig) {
     ram_config.regions.add(candidate_region);
 }
 
-fn main() {
+fn main() -> Result<(), Box<dyn std::error::Error>> {
     env_logger::init();
     let matches = App::new("Xous Image Creator")
         .version(crate_version!())
@@ -211,23 +211,20 @@ fn main() {
     if let Some(val) = matches.value_of("ram") {
         let ram_parts: Vec<&str> = val.split(':').collect();
         if ram_parts.len() != 2 {
-            eprintln!("Error: --ram argument should be of the form [offset]:[size]");
-            return;
+            return Err("Error: --ram argument should be of the form [offset]:[size]".into());
         }
 
         ram_config.offset = match parse_u32(ram_parts[0]) {
             Ok(o) => o,
             Err(e) => {
-                eprintln!("Error: Unable to parse {}: {:?}", ram_parts[0], e);
-                return;
+                return Err(format!("Error: Unable to parse {}: {:?}", ram_parts[0], e).into());
             }
         };
 
         ram_config.size = match parse_u32(ram_parts[1]) {
             Ok(o) => o,
             Err(e) => {
-                eprintln!("Error: Unable to parse {}: {:?}", ram_parts[1], e);
-                return;
+                return Err(format!("Error: Unable to parse {}: {:?}", ram_parts[1], e).into());
             }
         };
 
@@ -333,23 +330,20 @@ fn main() {
     if let Some(val) = matches.value_of("swap") {
         let swap_parts: Vec<&str> = val.split(':').collect();
         if swap_parts.len() != 2 {
-            eprintln!("Error: --swap argument should be of the form [offset]:[size]");
-            return;
+            return Err("Error: --swap argument should be of the form [offset]:[size]".into());
         }
 
         let offset = match parse_u32(swap_parts[0]) {
             Ok(o) => o,
             Err(e) => {
-                eprintln!("Error: Unable to parse {}: {:?}", swap_parts[0], e);
-                return;
+                return Err(format!("Error: Unable to parse {}: {:?}", swap_parts[0], e).into());
             }
         };
 
         let size = match parse_u32(swap_parts[1]) {
             Ok(o) => o,
             Err(e) => {
-                eprintln!("Error: Unable to parse {}: {:?}", swap_parts[1], e);
-                return;
+                return Err(format!("Error: Unable to parse {}: {:?}", swap_parts[1], e).into());
             }
         };
 
@@ -444,7 +438,7 @@ fn main() {
                 );
                 pid += 1;
                 let init = read_minielf(init_path).expect("couldn't parse init file");
-                sargs.add(IniS::new(init.entry_point, init.sections, init.program, init.alignment_offset));
+                sargs.add(IniS::new(init.entry_point, init.sections, init.program));
             }
         } else {
             println!("Warning: inis regions specified, but no swap region specified. Ignoring inis regions!");
@@ -477,8 +471,11 @@ fn main() {
 
     if let Some(mut sargs) = swap_args {
         let mut swap_buffer = SwapWriter::new();
+        // Transfer our unencrypted data inside sargs to swap_buffer
         sargs.write(&mut swap_buffer).expect("Couldn't write out swap args");
+        let buf_len = swap_buffer.buffer.get_ref().len();
 
+        // Create the swap target image and encrypt swap_buffer to it
         let swap_filename = matches.value_of("swap-name").expect("swap filename not present");
         let sf = File::create(swap_filename)
             .unwrap_or_else(|_| panic!("Couldn't create output file {}", swap_filename));
@@ -489,8 +486,14 @@ fn main() {
 
         match matches.value_of("swap-debug-name") {
             Some(sf_dbg) => {
-                let sf =
+                use std::io::Write;
+                let mut sf =
                     File::create(sf_dbg).unwrap_or_else(|_| panic!("Couldn't create debug file {}", sf_dbg));
+                // serialize the unencrypted header into the debug image
+                let header = tools::swap_writer::SwapHeader::new(buf_len);
+                sf.write(&header.serialize()?)?;
+                // directly write our unencrypted data from sargs into file sf. It's an exact copy of
+                // what ended up in swap_buffer.
                 sargs.write(&sf).expect("Couldn't write debug swap image");
                 println!("Swap debug created in file {}", sf_dbg);
             }
@@ -504,4 +507,5 @@ fn main() {
         println!("Runtime will also require {} bytes to track swap", s.size / 4096);
     }
     println!("Image created in file {}", output_filename);
+    Ok(())
 }

tools/src/swap_writer.rs

@@ -1,19 +1,43 @@
 use std::convert::TryInto;
 use std::io::{Cursor, Result, Seek, SeekFrom, Write};
+use std::process::Command;
 
 use aes_gcm_siv::{
     aead::{Aead, KeyInit, Payload},
     Aes256GcmSiv, Nonce,
 };
-use rand::Rng;
 
 const SWAP_VERSION: u32 = 0x01_01_0000;
 
+pub fn git_rev() -> u64 {
+    // Execute the git command
+    let output =
+        Command::new("git").args(&["rev-parse", "HEAD"]).output().expect("Failed to execute command");
+
+    // Check if the command was successful
+    if output.status.success() {
+        // Convert the output bytes to a string
+        let hash = std::str::from_utf8(&output.stdout).expect("Failed to convert output to string");
+
+        // Print the commit hash
+        let partial_hash =
+            u64::from_str_radix(&hash.trim_end_matches('\n')[hash.trim_end_matches('\n').len() - 16..], 16)
+                .expect("couldn't convert git hash");
+        println!("Current commit hash: {}  Extracted nonce: {:x}", hash, partial_hash);
+        partial_hash
+    } else {
+        // Print an error message if the command failed
+        let error_message =
+            std::str::from_utf8(&output.stderr).expect("Failed to convert error message to string");
+        panic!("Failed to get commit hash: {}", error_message);
+    }
+}
+
 pub struct SwapWriter {
     pub buffer: Cursor<Vec<u8>>,
 }
 
-struct SwapHeader {
+pub struct SwapHeader {
     aad: Vec<u8>,
     partial_nonce: u64,
     /// offset, starting from the top of payload data (so, +0x1000 to get absolute offset after the header)
@@ -21,10 +45,9 @@ struct SwapHeader {
 }
 impl SwapHeader {
     pub fn new(swap_len: usize) -> Self {
-        let mut rng = rand::thread_rng();
         SwapHeader {
             aad: "swap".as_bytes().to_vec(),
-            partial_nonce: rng.gen(),
+            partial_nonce: git_rev(),
             mac_offset: ((swap_len + 0xFFF) / 0x1000) * 0x1000,
         }
     }

tools/src/tags/inis.rs

@@ -17,9 +17,6 @@ pub struct IniS {
 
     /// Actual program data
     data: Vec<u8>,
-
-    /// Alignment offset
-    alignment_offset: usize,
 }
 
 impl fmt::Display for IniS {
@@ -39,17 +36,12 @@ impl fmt::Display for IniS {
 }
 
 impl IniS {
-    pub fn new(
-        entrypoint: u32,
-        sections: Vec<MiniElfSection>,
-        mut data: Vec<u8>,
-        alignment_offset: usize,
-    ) -> IniS {
+    pub fn new(entrypoint: u32, sections: Vec<MiniElfSection>, mut data: Vec<u8>) -> IniS {
         // pad the data to 4 bytes
         while data.len() & 3 != 0 {
             data.push(0);
         }
-        IniS { load_offset: 0, entrypoint, sections, data, alignment_offset }
+        IniS { load_offset: 0, entrypoint, sections, data }
     }
 }
 
@@ -61,14 +53,14 @@ impl XousArgument for IniS {
     fn finalize(&mut self, offset: usize) -> usize {
         self.load_offset = offset as u32;
 
-        assert!(offset % crate::tags::PAGE_SIZE == self.alignment_offset, "IniS load offset is not aligned");
+        assert!(offset % crate::tags::PAGE_SIZE == 0, "IniS load offset is not aligned");
         self.data = crate::tags::align_data_up(&self.data, 0);
         self.data.len()
     }
 
     fn last_data(&self) -> &[u8] { &self.data }
 
-    fn alignment_offset(&self) -> usize { self.alignment_offset }
+    fn alignment_offset(&self) -> usize { 0 }
 
     fn serialize(&self, output: &mut dyn io::Write) -> io::Result<usize> {
         let mut written = 0;
@@ -82,4 +74,6 @@ impl XousArgument for IniS {
         }
         Ok(written)
     }
+
+    fn load_offset(&self) -> usize { self.load_offset as usize }
 }

tools/src/tags/mod.rs

@@ -9,6 +9,10 @@ pub mod xkrn;
 
 pub(crate) const PAGE_SIZE: usize = 4096;
 
+/// Given a current `offset`, compute the next possible page address that can
+/// produce a page offset that is aligned with `alignment_offset`. This is
+/// not the same as the aligning to a page boundary -- this is aligning to
+/// an offset within a page boundary.
 pub fn align_size_up(offset: usize, alignment_offset: usize) -> usize {
     if offset % PAGE_SIZE == alignment_offset {
         offset

tools/src/xous_arguments.rs

@@ -37,6 +37,8 @@ pub trait XousArgument: fmt::Display {
     fn last_data(&self) -> &[u8] { &[] }
 
     fn alignment_offset(&self) -> usize { 0 }
+
+    fn load_offset(&self) -> usize { 0 }
 }
 
 pub struct XousArguments {
@@ -111,6 +113,10 @@ impl XousArguments {
         assert!(tag_data.get_ref().len().trailing_zeros() >= 2, "tag data was not a multiple of 4 bytes!");
 
         let mut digest = crc16::Digest::new(crc16::X25);
+
+        // store the header offset
+        let header_offset = w.stream_position()?;
+
         // XArg tag header
         w.write_all(&u32::from_le_bytes(*b"XArg").to_le_bytes())?;
         digest.write(tag_data.get_ref());
@@ -153,10 +159,45 @@ impl XousArguments {
         for arg in &self.arguments {
             // align for FLASH mapping
             let pos = w.stream_position()?;
+            // find the next padding that allows us to align our data such that page sizes align.
             let pad_len = crate::tags::align_size_up(pos as usize, arg.alignment_offset()) - pos as usize;
+            // println!("padding from {:x}, align {:x}, with {:x} bytes", pos, arg.alignment_offset(),
+            // pad_len);
             let pad = vec![0u8; pad_len];
             w.write_all(&pad)?;
 
+            // only do this check on the IniS section (swap generation). Rationale: the
+            // arg.load_offset() function is only implemented for IniS. It may be trivial to
+            // copy this to the other sections, but, the original idea was to make a targeted check of
+            // the swap format at this point in the image creation cycle.
+            if arg.code() == u32::from_le_bytes(*b"IniS") {
+                // println!("header_offset: {:x}", header_offset);
+                // - 0x0 is the valid offset in the case that we are being called by the encrypted partition
+                //   writer, since it computes offsets from the start of the encrypted partition.
+                // - 0x1000 is the expected length of the header when writing the full plaintext version for
+                //   sanity checking
+                assert!(
+                    header_offset == 0x1000 || header_offset == 0x0,
+                    "Header offset assumption for IniS was not met: loader hard-codes this size"
+                );
+                /*
+                println!(
+                    "load offset: {:x}, pos: {:x}",
+                    arg.load_offset(),
+                    w.stream_position()? - header_offset
+                ); */
+                // Debugging tips
+                // If this assert fails, what's happened is that the position of the loader stream
+                // is offset from where it's expected to be. There is a "just so" arrangement that isn't
+                // strictly enforced: the first IniS happens to load at 0x1000; and the XArgs block
+                // will generally fit within a size constraint of 0x1000. If either of these assumptions
+                // break, then, this assert will trigger.
+                assert!(
+                    arg.load_offset() as u64 == w.stream_position()? - header_offset,
+                    "IniS alignment assumption not satisfied, did XArgs overflow? \
+                    Did the IniS section trigger an alignment edge case with align_size_up()?"
+                );
+            }
             // println!("position: {:x}", w.stream_position()?);
             w.write_all(arg.last_data()).expect("couldn't write extra arg data");
         }