New Upgrade Interface (#543)

* includes metadata inside partition, introduces the partition helper

* style improvements

src/api/upgrade_storage/helper.rs

@@ -15,6 +15,7 @@
 // For compiling with std outside of tests.
 #![cfg_attr(feature = "std", allow(dead_code))]
 
+use alloc::vec::Vec;
 use core::iter::Iterator;
 use persistent_store::{StorageError, StorageResult};
 
@@ -55,6 +56,7 @@ pub fn is_aligned(block_size: usize, address: usize) -> bool {
 ///
 /// The range is treated as the interval `[start, start + length)`.
 /// All objects with length of 0, regardless of the start value, are considered empty.
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub struct ModRange {
     start: usize,
     length: usize,
@@ -86,25 +88,45 @@ impl ModRange {
         self.length == 0
     }
 
-    /// Returns the disjoint union with the other range, if is consecutive.
+    /// Returns the disjoint union with the other range, if consecutive.
     ///
     /// Appending empty ranges is not possible.
     /// Appending to the empty range returns the other range.
-    pub fn append(&self, other: ModRange) -> Option<ModRange> {
+    ///
+    /// Returns true if successful.
+    pub fn append(&mut self, other: &ModRange) -> bool {
         if self.is_empty() {
-            return Some(other);
+            self.start = other.start;
+            self.length = other.length;
+            return true;
         }
         if other.is_empty() {
-            return None;
+            return false;
         }
         if self.start >= other.start {
-            return None;
+            return false;
         }
         if self.length != other.start - self.start {
-            return None;
+            return false;
         }
-        let new_length = self.length.checked_add(other.length);
+        if let Some(new_length) = self.length.checked_add(other.length) {
-        new_length.map(|l| ModRange::new(self.start, l))
+            self.length = new_length;
+            true
+        } else {
+            false
+        }
+    }
+
+    /// Helper function to check whether a range starts within another.
+    fn starts_inside(&self, range: &ModRange) -> bool {
+        !range.is_empty() && self.start >= range.start && self.start - range.start < range.length
+    }
+
+    /// Returns whether the given range has intersects.
+    ///
+    /// Mathematically, we calculate whether: `self ∩ range ≠ ∅`.
+    pub fn intersects_range(&self, range: &ModRange) -> bool {
+        self.starts_inside(range) || range.starts_inside(self)
     }
 
     /// Returns whether the given range is fully contained.
@@ -128,6 +150,73 @@ impl ModRange {
     }
 }
 
+pub struct Partition {
+    ranges: Vec<ModRange>,
+}
+
+impl Partition {
+    pub fn new() -> Partition {
+        Partition { ranges: Vec::new() }
+    }
+
+    /// Total length of all ranges.
+    pub fn length(&self) -> usize {
+        self.ranges.iter().map(|r| r.length()).sum()
+    }
+
+    /// Appends the given range.
+    ///
+    /// Ranges should be appending with ascending start addresses.
+    pub fn append(&mut self, range: ModRange) -> bool {
+        if let Some(last_range) = self.ranges.last_mut() {
+            if range.start() <= last_range.start()
+                || range.start() - last_range.start() < last_range.length()
+            {
+                return false;
+            }
+            if !last_range.append(&range) {
+                self.ranges.push(range);
+            }
+        } else {
+            self.ranges.push(range);
+        }
+        true
+    }
+
+    /// Returns the start address that corresponds to the given offset.
+    ///
+    /// If the offset bigger than the accumulated length or the requested slice doesn't fit a
+    /// connected component, return `None`.
+    pub fn find_address(&self, mut offset: usize, length: usize) -> Option<usize> {
+        for range in &self.ranges {
+            if offset < range.length() {
+                return if range.length() - offset >= length {
+                    Some(range.start() + offset)
+                } else {
+                    None
+                };
+            }
+            offset -= range.length()
+        }
+        None
+    }
+
+    pub fn ranges_from(&self, start_address: usize) -> Vec<ModRange> {
+        let mut result = Vec::new();
+        for range in &self.ranges {
+            match start_address.checked_sub(range.start()) {
+                None | Some(0) => result.push(range.clone()),
+                Some(offset) => {
+                    if range.length() > offset {
+                        result.push(ModRange::new(start_address, range.length() - offset));
+                    }
+                }
+            }
+        }
+        result
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -186,18 +275,17 @@ mod tests {
 
     #[test]
     fn mod_range_append() {
-        let range = ModRange::new(200, 100);
+        let mut range = ModRange::new(200, 100);
-        let new_range = range.append(ModRange::new(300, 400)).unwrap();
+        assert!(range.append(&ModRange::new(300, 400)));
-        assert!(new_range.start() == 200);
+        assert!(range.start() == 200);
-        assert!(new_range.length() == 500);
+        assert!(range.length() == 500);
-        assert!(range.append(ModRange::new(299, 400)).is_none());
+        assert!(!range.append(&ModRange::new(499, 400)));
-        assert!(range.append(ModRange::new(301, 400)).is_none());
+        assert!(!range.append(&ModRange::new(501, 400)));
-        assert!(range.append(ModRange::new(200, 400)).is_none());
+        assert!(!range.append(&ModRange::new(300, 400)));
-        let empty_append = ModRange::new_empty()
+        let mut range = ModRange::new_empty();
-            .append(ModRange::new(200, 100))
+        assert!(range.append(&ModRange::new(200, 100)));
-            .unwrap();
+        assert!(range.start() == 200);
-        assert!(empty_append.start() == 200);
+        assert!(range.length() == 100);
-        assert!(empty_append.length() == 100);
     }
 
     #[test]
@@ -216,6 +304,20 @@ mod tests {
         assert!(ModRange::new(usize::MAX, 2).contains_range(&ModRange::new(usize::MAX, 2)));
     }
 
+    #[test]
+    fn mod_range_intersects_range() {
+        let range = ModRange::new(200, 100);
+        assert!(range.intersects_range(&ModRange::new(200, 1)));
+        assert!(range.intersects_range(&ModRange::new(299, 1)));
+        assert!(!range.intersects_range(&ModRange::new(199, 1)));
+        assert!(!range.intersects_range(&ModRange::new(300, 1)));
+        assert!(!ModRange::new_empty().intersects_range(&ModRange::new_empty()));
+        assert!(!ModRange::new_empty().intersects_range(&ModRange::new(200, 100)));
+        assert!(!ModRange::new(200, 100).intersects_range(&ModRange::new_empty()));
+        assert!(ModRange::new(usize::MAX, 1).intersects_range(&ModRange::new(usize::MAX, 1)));
+        assert!(ModRange::new(usize::MAX, 2).intersects_range(&ModRange::new(usize::MAX, 2)));
+    }
+
     #[test]
     fn mod_range_aligned_iter() {
         let mut iter = ModRange::new(200, 100).aligned_iter(100);
@@ -234,4 +336,49 @@ mod tests {
         assert_eq!(iter.next(), Some(0xffff_ffff_ffff_fff0));
         assert_eq!(iter.next(), None);
     }
+
+    #[test]
+    fn partition_append() {
+        let mut partition = Partition::new();
+        partition.append(ModRange::new(0x4000, 0x1000));
+        partition.append(ModRange::new(0x20000, 0x20000));
+        partition.append(ModRange::new(0x40000, 0x20000));
+        assert_eq!(partition.find_address(0, 1), Some(0x4000));
+        assert_eq!(partition.length(), 0x41000);
+    }
+
+    #[test]
+    fn partition_find_address() {
+        let mut partition = Partition::new();
+        partition.append(ModRange::new(0x4000, 0x1000));
+        partition.append(ModRange::new(0x20000, 0x20000));
+        partition.append(ModRange::new(0x40000, 0x20000));
+        assert_eq!(partition.find_address(0, 0x1000), Some(0x4000));
+        assert_eq!(partition.find_address(0x1000, 0x1000), Some(0x20000));
+        assert_eq!(partition.find_address(0x20000, 0x1000), Some(0x3F000));
+        assert_eq!(partition.find_address(0x21000, 0x1000), Some(0x40000));
+        assert_eq!(partition.find_address(0x40000, 0x1000), Some(0x5F000));
+        assert_eq!(partition.find_address(0x41000, 0x1000), None);
+        assert_eq!(partition.find_address(0x40000, 0x2000), None);
+    }
+
+    #[test]
+    fn partition_ranges_from() {
+        let mut partition = Partition::new();
+        partition.append(ModRange::new(0x4000, 0x1000));
+        partition.append(ModRange::new(0x20000, 0x20000));
+        partition.append(ModRange::new(0x40000, 0x20000));
+        let all_ranges = partition.ranges_from(0);
+        let from_start_ranges = partition.ranges_from(0x4000);
+        assert_eq!(&all_ranges, &from_start_ranges);
+        assert_eq!(all_ranges.len(), 2);
+        assert_eq!(all_ranges[0], ModRange::new(0x4000, 0x1000));
+        assert_eq!(all_ranges[1], ModRange::new(0x20000, 0x40000));
+        let second_range = partition.ranges_from(0x20000);
+        let same_second_range = partition.ranges_from(0x1F000);
+        assert_eq!(&second_range, &same_second_range);
+        assert_eq!(&second_range, &all_ranges[1..]);
+        let partial_range = partition.ranges_from(0x30000);
+        assert_eq!(partial_range[0], ModRange::new(0x30000, 0x30000));
+    }
 }

src/api/upgrade_storage/mod.rs

@@ -20,7 +20,9 @@ pub(crate) mod helper;
 pub trait UpgradeStorage {
     /// Reads a slice of the partition, if within bounds.
     ///
-    /// The offset is relative to the start of the partition.
+    /// The offset is relative to the start of the partition, excluding holes. The partition is
+    /// presented as one connected component. Therefore, the offset does not easily translate
+    /// to physical memory address address of the slice.
     ///
     /// # Errors
     ///
@@ -29,31 +31,23 @@ pub trait UpgradeStorage {
 
     /// Writes the given data to the given offset address, if within bounds of the partition.
     ///
-    /// The offset is relative to the start of the partition.
+    /// The offset is relative to the start of the partition, excluding holes.
+    /// See `read_partition`.
     ///
     /// # Errors
     ///
-    /// Returns [`StorageError::OutOfBounds`] if the data does not fit the partition.
+    /// - Returns [`StorageError::OutOfBounds`] if the data does not fit the partition.
+    /// - Returns [`StorageError::CustomError`] if any Metadata check fails.
     fn write_partition(&mut self, offset: usize, data: &[u8]) -> StorageResult<()>;
 
-    /// Returns the address of the partition.
+    /// Returns an identifier for the partition.
-    fn partition_address(&self) -> usize;
+    ///
+    /// Use this to determine whether you are writing to A or B.
+    fn partition_identifier(&self) -> u32;
 
     /// Returns the length of the partition.
     fn partition_length(&self) -> usize;
 
-    /// Reads the metadata location.
-    fn read_metadata(&self) -> StorageResult<&[u8]>;
-
-    /// Writes the given data into the metadata location.
-    ///
-    /// The passed in data is appended with 0xFF bytes if shorter than the metadata storage.
-    ///
-    /// # Errors
-    ///
-    /// Returns [`StorageError::OutOfBounds`] if the data is too long to fit the metadata storage.
-    fn write_metadata(&mut self, data: &[u8]) -> StorageResult<()>;
-
     /// Returns the currently running firmware version.
     fn running_firmware_version(&self) -> u64;
 }

src/ctap/command.rs

@@ -592,7 +592,7 @@ impl TryFrom<cbor::Value> for AuthenticatorVendorConfigureParameters {
 
 #[derive(Debug, PartialEq, Eq)]
 pub struct AuthenticatorVendorUpgradeParameters {
-    pub address: Option<usize>,
+    pub offset: usize,
     pub data: Vec<u8>,
     pub hash: Vec<u8>,
 }
@@ -603,22 +603,15 @@ impl TryFrom<cbor::Value> for AuthenticatorVendorUpgradeParameters {
     fn try_from(cbor_value: cbor::Value) -> Result<Self, Ctap2StatusCode> {
         destructure_cbor_map! {
             let {
-                0x01 => address,
+                0x01 => offset,
                 0x02 => data,
                 0x03 => hash,
             } = extract_map(cbor_value)?;
         }
-        let address = address
+        let offset = extract_unsigned(ok_or_missing(offset)?)? as usize;
-            .map(extract_unsigned)
-            .transpose()?
-            .map(|u| u as usize);
         let data = extract_byte_string(ok_or_missing(data)?)?;
         let hash = extract_byte_string(ok_or_missing(hash)?)?;
-        Ok(AuthenticatorVendorUpgradeParameters {
+        Ok(AuthenticatorVendorUpgradeParameters { offset, data, hash })
-            address,
-            data,
-            hash,
-        })
     }
 }
 
@@ -1067,6 +1060,16 @@ mod test {
         let command = Command::deserialize(&cbor_bytes);
         assert_eq!(command, Err(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR));
 
+        // Missing offset
+        let cbor_value = cbor_map! {
+            0x02 => [0xFF; 0x100],
+            0x03 => [0x44; 32],
+        };
+        assert_eq!(
+            AuthenticatorVendorUpgradeParameters::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER)
+        );
+
         // Missing data
         let cbor_value = cbor_map! {
             0x01 => 0x1000,
@@ -1087,21 +1090,7 @@ mod test {
             Err(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER)
         );
 
-        // Valid without address
+        // Valid
-        let cbor_value = cbor_map! {
-            0x02 => [0xFF; 0x100],
-            0x03 => [0x44; 32],
-        };
-        assert_eq!(
-            AuthenticatorVendorUpgradeParameters::try_from(cbor_value),
-            Ok(AuthenticatorVendorUpgradeParameters {
-                address: None,
-                data: vec![0xFF; 0x100],
-                hash: vec![0x44; 32],
-            })
-        );
-
-        // Valid with address
         let cbor_value = cbor_map! {
             0x01 => 0x1000,
             0x02 => [0xFF; 0x100],
@@ -1110,7 +1099,7 @@ mod test {
         assert_eq!(
             AuthenticatorVendorUpgradeParameters::try_from(cbor_value),
             Ok(AuthenticatorVendorUpgradeParameters {
-                address: Some(0x1000),
+                offset: 0x1000,
                 data: vec![0xFF; 0x100],
                 hash: vec![0x44; 32],
             })

src/ctap/mod.rs

@@ -1398,30 +1398,16 @@ impl CtapState {
         env: &mut impl Env,
         params: AuthenticatorVendorUpgradeParameters,
     ) -> Result<ResponseData, Ctap2StatusCode> {
-        let AuthenticatorVendorUpgradeParameters {
+        let AuthenticatorVendorUpgradeParameters { offset, data, hash } = params;
-            address,
-            data,
-            hash,
-        } = params;
         let upgrade_locations = env
             .upgrade_storage()
             .ok_or(Ctap2StatusCode::CTAP1_ERR_INVALID_COMMAND)?;
-        let written_slice = if let Some(address) = address {
         upgrade_locations
-                .write_partition(address, &data)
+            .write_partition(offset, &data)
+            .map_err(|_| Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)?;
+        let written_slice = upgrade_locations
+            .read_partition(offset, data.len())
             .map_err(|_| Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)?;
-            upgrade_locations
-                .read_partition(address, data.len())
-                .map_err(|_| Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)?
-        } else {
-            // Write the metadata page after verifying it.
-            upgrade_locations
-                .write_metadata(&data)
-                .map_err(|_| Ctap2StatusCode::CTAP2_ERR_INTEGRITY_FAILURE)?;
-            &upgrade_locations
-                .read_metadata()
-                .map_err(|_| Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)?[..data.len()]
-        };
         let written_hash = Sha256::hash(written_slice);
         if hash != written_hash {
             return Err(Ctap2StatusCode::CTAP2_ERR_INTEGRITY_FAILURE);
@@ -1438,7 +1424,7 @@ impl CtapState {
             .ok_or(Ctap2StatusCode::CTAP1_ERR_INVALID_COMMAND)?;
         Ok(ResponseData::AuthenticatorVendorUpgradeInfo(
             AuthenticatorVendorUpgradeInfoResponse {
-                info: upgrade_locations.partition_address() as u32,
+                info: upgrade_locations.partition_identifier(),
             },
         ))
     }
@@ -1493,7 +1479,6 @@ mod test {
     use crate::api::user_presence::UserPresenceResult;
     use crate::env::test::TestEnv;
     use crate::test_helpers;
-    use byteorder::LittleEndian;
     use cbor::{cbor_array, cbor_array_vec, cbor_map};
 
     // The keep-alive logic in the processing of some commands needs a channel ID to send
@@ -3404,70 +3389,63 @@ mod test {
         // The test metadata storage has size 0x1000.
         // The test identifier matches partition B.
         let mut env = TestEnv::new();
-        let private_key = ecdsa::SecKey::gensk(env.rng());
         let mut ctap_state = CtapState::new(&mut env, CtapInstant::new(0));
-        const METADATA_LEN: usize = 0x1000;
-        const METADATA_SIGN_OFFSET: usize = 0x800;
-        let mut metadata = vec![0xFF; METADATA_LEN];
-        LittleEndian::write_u32(&mut metadata[METADATA_SIGN_OFFSET + 8..][..4], 0x60000);
 
+        const METADATA_LEN: usize = 0x1000;
+        let metadata = vec![0xFF; METADATA_LEN];
+        let metadata_hash = Sha256::hash(&metadata).to_vec();
         let data = vec![0xFF; 0x1000];
         let hash = Sha256::hash(&data).to_vec();
-        let upgrade_locations = env.upgrade_storage().unwrap();
-        let partition_length = upgrade_locations.partition_length();
-        let mut signed_over_data = metadata[METADATA_SIGN_OFFSET..].to_vec();
-        signed_over_data.extend(
-            upgrade_locations
-                .read_partition(0, partition_length)
-                .unwrap(),
-        );
-        let signed_hash = Sha256::hash(&signed_over_data);
-
-        metadata[..32].copy_from_slice(&signed_hash);
-        let signature = private_key.sign_rfc6979::<Sha256>(&signed_over_data);
-        let mut signature_bytes = [0; ecdsa::Signature::BYTES_LENGTH];
-        signature.to_bytes(&mut signature_bytes);
-        metadata[32..96].copy_from_slice(&signature_bytes);
-        let metadata_hash = Sha256::hash(&metadata).to_vec();
 
         // Write to partition.
         let response = ctap_state.process_vendor_upgrade(
             &mut env,
             AuthenticatorVendorUpgradeParameters {
-                address: Some(0x20000),
+                offset: 0x20000,
                 data: data.clone(),
                 hash: hash.clone(),
             },
         );
         assert_eq!(response, Ok(ResponseData::AuthenticatorVendorUpgrade));
 
-        // TestEnv doesn't check the metadata, test parsing that in your Env.
+        // TestEnv doesn't check the metadata, test its parser in your Env.
         let response = ctap_state.process_vendor_upgrade(
             &mut env,
             AuthenticatorVendorUpgradeParameters {
-                address: None,
+                offset: 0,
                 data: metadata.clone(),
                 hash: metadata_hash.clone(),
             },
         );
         assert_eq!(response, Ok(ResponseData::AuthenticatorVendorUpgrade));
 
+        // TestEnv doesn't check the metadata, test its parser in your Env.
+        let response = ctap_state.process_vendor_upgrade(
+            &mut env,
+            AuthenticatorVendorUpgradeParameters {
+                offset: METADATA_LEN,
+                data: data.clone(),
+                hash: hash.clone(),
+            },
+        );
+        assert_eq!(response, Ok(ResponseData::AuthenticatorVendorUpgrade));
+
         // Write metadata of a wrong size.
         let response = ctap_state.process_vendor_upgrade(
             &mut env,
             AuthenticatorVendorUpgradeParameters {
-                address: None,
+                offset: 0,
                 data: metadata[..METADATA_LEN - 1].to_vec(),
                 hash: metadata_hash,
             },
         );
-        assert_eq!(response, Err(Ctap2StatusCode::CTAP2_ERR_INTEGRITY_FAILURE));
+        assert_eq!(response, Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER));
 
         // Write outside of the partition.
         let response = ctap_state.process_vendor_upgrade(
             &mut env,
             AuthenticatorVendorUpgradeParameters {
-                address: Some(0x40000),
+                offset: 0x41000,
                 data: data.clone(),
                 hash,
             },
@@ -3478,7 +3456,7 @@ mod test {
         let response = ctap_state.process_vendor_upgrade(
             &mut env,
             AuthenticatorVendorUpgradeParameters {
-                address: Some(0x20000),
+                offset: 0x20000,
                 data,
                 hash: [0xEE; 32].to_vec(),
             },
@@ -3497,7 +3475,7 @@ mod test {
         let response = ctap_state.process_vendor_upgrade(
             &mut env,
             AuthenticatorVendorUpgradeParameters {
-                address: Some(0),
+                offset: 0,
                 data,
                 hash,
             },
@@ -3509,14 +3487,14 @@ mod test {
     fn test_vendor_upgrade_info() {
         let mut env = TestEnv::new();
         let ctap_state = CtapState::new(&mut env, CtapInstant::new(0));
-        let partition_address = env.upgrade_storage().unwrap().partition_address();
+        let partition_identifier = env.upgrade_storage().unwrap().partition_identifier();
 
         let upgrade_info_reponse = ctap_state.process_vendor_upgrade_info(&mut env);
         assert_eq!(
             upgrade_info_reponse,
             Ok(ResponseData::AuthenticatorVendorUpgradeInfo(
                 AuthenticatorVendorUpgradeInfoResponse {
-                    info: partition_address as u32,
+                    info: partition_identifier,
                 }
             ))
         );

src/env/test/upgrade_storage.rs

@@ -17,22 +17,18 @@ use crate::api::upgrade_storage::UpgradeStorage;
 use alloc::boxed::Box;
 use persistent_store::{StorageError, StorageResult};
 
-const PARTITION_LENGTH: usize = 0x40000;
+const PARTITION_LENGTH: usize = 0x41000;
 const METADATA_LENGTH: usize = 0x1000;
 
 pub struct BufferUpgradeStorage {
     /// Content of the partition storage.
     partition: Box<[u8]>,
-
-    /// Content of the metadata storage.
-    metadata: Box<[u8]>,
 }
 
 impl BufferUpgradeStorage {
     pub fn new() -> StorageResult<BufferUpgradeStorage> {
         Ok(BufferUpgradeStorage {
             partition: vec![0xff; PARTITION_LENGTH].into_boxed_slice(),
-            metadata: vec![0xff; METADATA_LENGTH].into_boxed_slice(),
         })
     }
 }
@@ -51,6 +47,9 @@ impl UpgradeStorage for BufferUpgradeStorage {
     }
 
     fn write_partition(&mut self, offset: usize, data: &[u8]) -> StorageResult<()> {
+        if offset == 0 && data.len() != METADATA_LENGTH {
+            return Err(StorageError::OutOfBounds);
+        }
         if data.is_empty() {
             return Err(StorageError::OutOfBounds);
         }
@@ -63,7 +62,7 @@ impl UpgradeStorage for BufferUpgradeStorage {
         }
     }
 
-    fn partition_address(&self) -> usize {
+    fn partition_identifier(&self) -> u32 {
         0x60000
     }
 
@@ -71,20 +70,6 @@ impl UpgradeStorage for BufferUpgradeStorage {
         PARTITION_LENGTH
     }
 
-    fn read_metadata(&self) -> StorageResult<&[u8]> {
-        Ok(&self.metadata[..])
-    }
-
-    fn write_metadata(&mut self, data: &[u8]) -> StorageResult<()> {
-        if data.len() <= METADATA_LENGTH {
-            self.metadata.copy_from_slice(&[0xff; METADATA_LENGTH]);
-            self.metadata[..data.len()].copy_from_slice(data);
-            Ok(())
-        } else {
-            Err(StorageError::OutOfBounds)
-        }
-    }
-
     fn running_firmware_version(&self) -> u64 {
         0
     }
@@ -130,23 +115,7 @@ mod tests {
     #[test]
     fn partition_slice() {
         let storage = BufferUpgradeStorage::new().unwrap();
-        assert_eq!(storage.partition_address(), 0x60000);
+        assert_eq!(storage.partition_identifier(), 0x60000);
         assert_eq!(storage.partition_length(), PARTITION_LENGTH);
     }
-
-    #[test]
-    fn read_write_metadata() {
-        let mut storage = BufferUpgradeStorage::new().unwrap();
-        assert_eq!(storage.read_metadata().unwrap(), &[0xFF; METADATA_LENGTH]);
-        assert!(storage.write_metadata(&[0x88, 0x88]).is_ok());
-        assert_eq!(
-            storage.write_metadata(&[0x88; METADATA_LENGTH + 1]),
-            Err(StorageError::OutOfBounds)
-        );
-        let new_metadata = storage.read_metadata().unwrap();
-        assert_eq!(&new_metadata[0..2], &[0x88, 0x88]);
-        assert_eq!(&new_metadata[2..], &[0xFF; METADATA_LENGTH - 2]);
-        assert!(storage.write_metadata(&[]).is_ok());
-        assert_eq!(storage.read_metadata().unwrap(), &[0xFF; METADATA_LENGTH]);
-    }
 }

src/env/tock/storage.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::api::upgrade_storage::helper::{find_slice, is_aligned, ModRange};
+use crate::api::upgrade_storage::helper::{find_slice, is_aligned, ModRange, Partition};
 use crate::api::upgrade_storage::UpgradeStorage;
 use alloc::borrow::Cow;
 use alloc::vec::Vec;
@@ -231,16 +231,17 @@ impl Storage for TockStorage {
 
 pub struct TockUpgradeStorage {
     page_size: usize,
-    partition: ModRange,
+    partition: Partition,
     metadata: ModRange,
     running_metadata: ModRange,
+    identifier: u32,
 }
 
 impl TockUpgradeStorage {
     // Ideally, the kernel should tell us metadata and partitions directly.
     // This code only works for one layout, refactor this into the storage driver to support more.
     const METADATA_ADDRESS: usize = 0x4000;
-    const _PARTITION_ADDRESS_A: usize = 0x20000;
+    const PARTITION_ADDRESS_A: usize = 0x20000;
     const PARTITION_ADDRESS_B: usize = 0x60000;
 
     /// Provides access to the other upgrade partition and metadata if available.
@@ -260,13 +261,15 @@ impl TockUpgradeStorage {
     pub fn new() -> StorageResult<TockUpgradeStorage> {
         let mut locations = TockUpgradeStorage {
             page_size: get_info(command_nr::get_info_nr::PAGE_SIZE, 0)?,
-            partition: ModRange::new_empty(),
+            partition: Partition::new(),
             metadata: ModRange::new_empty(),
             running_metadata: ModRange::new_empty(),
+            identifier: Self::PARTITION_ADDRESS_A as u32,
         };
         if !locations.page_size.is_power_of_two() {
             return Err(StorageError::CustomError);
         }
+        let mut firmware_range = ModRange::new_empty();
         for i in 0..memop(memop_nr::STORAGE_CNT, 0)? {
             let storage_type = memop(memop_nr::STORAGE_TYPE, i)?;
             if !matches!(storage_type, storage_type::PARTITION) {
@@ -286,21 +289,27 @@ impl TockUpgradeStorage {
                         ModRange::new(range.start() + locations.page_size, locations.page_size);
                 }
                 _ => {
-                    locations.partition = locations
+                    if !firmware_range.append(&range) {
-                        .partition
+                        return Err(StorageError::NotAligned);
-                        .append(range)
-                        .ok_or(StorageError::NotAligned)?
                     }
                 }
             }
-        if locations.partition.is_empty()
+        }
+        if firmware_range.is_empty()
             || locations.metadata.is_empty()
             || locations.running_metadata.is_empty()
         {
             return Err(StorageError::CustomError);
         }
-        if locations.partition.start() == Self::PARTITION_ADDRESS_B {
+        if firmware_range.start() == Self::PARTITION_ADDRESS_B {
             core::mem::swap(&mut locations.metadata, &mut locations.running_metadata);
+            locations.identifier = Self::PARTITION_ADDRESS_B as u32;
+        }
+        if !locations.partition.append(locations.metadata.clone()) {
+            return Err(StorageError::NotAligned);
+        }
+        if !locations.partition.append(firmware_range) {
+            return Err(StorageError::NotAligned);
         }
         Ok(locations)
     }
@@ -308,6 +317,38 @@ impl TockUpgradeStorage {
     fn is_page_aligned(&self, x: usize) -> bool {
         is_aligned(self.page_size, x)
     }
+
+    /// Returns whether the metadata is contained in this range or not.
+    ///
+    /// Assumes that metadata is written in one call per range. If the metadata is only partially
+    /// contained, returns an error.
+    fn contains_metadata(&self, checked_range: &ModRange) -> StorageResult<bool> {
+        if checked_range.intersects_range(&self.metadata) {
+            if checked_range.contains_range(&self.metadata) {
+                Ok(true)
+            } else {
+                Err(StorageError::NotAligned)
+            }
+        } else {
+            Ok(false)
+        }
+    }
+
+    /// Checks if the metadata's hash matches the partition's content.
+    fn check_partition_hash(&self, metadata: &[u8]) -> StorageResult<()> {
+        let start_address = self.metadata.start() + METADATA_SIGN_OFFSET;
+        let mut hasher = Sha256::new();
+        for range in self.partition.ranges_from(start_address) {
+            let partition_slice = unsafe { read_slice(range.start(), range.length()) };
+            // The hash implementation handles this in chunks, so no memory issues.
+            hasher.update(partition_slice);
+        }
+        let computed_hash = hasher.finalize();
+        if &computed_hash != parse_metadata_hash(metadata) {
+            return Err(StorageError::CustomError);
+        }
+        Ok(())
+    }
 }
 
 impl UpgradeStorage for TockUpgradeStorage {
@@ -315,11 +356,7 @@ impl UpgradeStorage for TockUpgradeStorage {
         if length == 0 {
             return Err(StorageError::OutOfBounds);
         }
-        let address = self.partition.start() + offset;
+        if let Some(address) = self.partition.find_address(offset, length) {
-        if self
-            .partition
-            .contains_range(&ModRange::new(address, length))
-        {
             Ok(unsafe { read_slice(address, length) })
         } else {
             Err(StorageError::OutOfBounds)
@@ -330,44 +367,38 @@ impl UpgradeStorage for TockUpgradeStorage {
         if data.is_empty() {
             return Err(StorageError::OutOfBounds);
         }
-        let address = self.partition.start() + offset;
+        let address = self
+            .partition
+            .find_address(offset, data.len())
+            .ok_or(StorageError::OutOfBounds)?;
         let write_range = ModRange::new(address, data.len());
-        if self.partition.contains_range(&write_range) {
+        if self.contains_metadata(&write_range)? {
+            let new_metadata = &data[self.metadata.start() - address..][..self.metadata.length()];
+            check_metadata(self, UPGRADE_PUBLIC_KEY, new_metadata)?;
+        }
+
         // Erases all pages that have their first byte in the write range.
         // Since we expect calls in order, we don't want to erase half-written pages.
         for address in write_range.aligned_iter(self.page_size) {
             erase_page(address, self.page_size)?;
         }
-            write_slice(address, data)
+        write_slice(address, data)?;
-        } else {
+        // Case: Last slice is written.
-            Err(StorageError::OutOfBounds)
+        if data.len() == self.partition_length() - offset {
+            let metadata = unsafe { read_slice(self.metadata.start(), self.metadata.length()) };
+            self.check_partition_hash(&metadata)?;
         }
+        Ok(())
     }
 
-    fn partition_address(&self) -> usize {
+    fn partition_identifier(&self) -> u32 {
-        self.partition.start()
+        self.identifier
     }
 
     fn partition_length(&self) -> usize {
         self.partition.length()
     }
 
-    fn read_metadata(&self) -> StorageResult<&[u8]> {
-        Ok(unsafe { read_slice(self.metadata.start(), self.metadata.length()) })
-    }
-
-    fn write_metadata(&mut self, data: &[u8]) -> StorageResult<()> {
-        if data.len() != self.metadata.length() {
-            return Err(StorageError::CustomError);
-        }
-        // Compares the hash inside the metadata to the actual hash.
-        parse_metadata(self, UPGRADE_PUBLIC_KEY, &data)?;
-        for address in self.metadata.aligned_iter(self.page_size) {
-            erase_page(address, self.page_size)?;
-        }
-        write_slice(self.metadata.start(), data)
-    }
-
     fn running_firmware_version(&self) -> u64 {
         let running_metadata = unsafe {
             read_slice(
@@ -389,8 +420,9 @@ impl UpgradeStorage for TockUpgradeStorage {
 /// -  4 B partition address in little endian encoding
 /// written at METADATA_SIGN_OFFSET.
 ///
-/// Checks hash and signature correctness, and whether the partition offset matches.
+/// Checks signature correctness against the hash, and whether the partition offset matches.
-fn parse_metadata(
+/// Whether the hash matches the partition content is not tested here!
+fn check_metadata(
     upgrade_locations: &impl UpgradeStorage,
     public_key_bytes: &[u8],
     metadata: &[u8],
@@ -406,29 +438,23 @@ fn parse_metadata(
     }
 
     let metadata_address = LittleEndian::read_u32(&metadata[METADATA_SIGN_OFFSET + 8..][..4]);
-    if metadata_address as usize != upgrade_locations.partition_address() {
+    if metadata_address != upgrade_locations.partition_identifier() {
-        return Err(StorageError::CustomError);
-    }
-
-    // The hash implementation handles this in chunks, so no memory issues.
-    let partition_slice =
-        upgrade_locations.read_partition(0, upgrade_locations.partition_length())?;
-    let mut hasher = Sha256::new();
-    hasher.update(&metadata[METADATA_SIGN_OFFSET..]);
-    hasher.update(partition_slice);
-    let computed_hash = hasher.finalize();
-    if &computed_hash != array_ref!(metadata, 0, 32) {
         return Err(StorageError::CustomError);
     }
 
     verify_signature(
         array_ref!(metadata, 32, 64),
         public_key_bytes,
-        &computed_hash,
+        parse_metadata_hash(metadata),
     )?;
     Ok(())
 }
 
+/// Parses the metadata, returns the hash.
+fn parse_metadata_hash(data: &[u8]) -> &[u8; 32] {
+    array_ref!(data, 0, 32)
+}
+
 /// Parses the metadata, returns the firmware version.
 fn parse_metadata_version(data: &[u8]) -> u64 {
     LittleEndian::read_u64(&data[METADATA_SIGN_OFFSET..][..8])
@@ -459,7 +485,7 @@ mod test {
     use crate::env::Env;
 
     #[test]
-    fn test_parse_metadata() {
+    fn test_check_metadata() {
         let mut env = TestEnv::new();
         let private_key = crypto::ecdsa::SecKey::gensk(env.rng());
         let upgrade_locations = env.upgrade_storage().unwrap();
@@ -489,42 +515,37 @@ mod test {
         public_key.to_bytes_uncompressed(&mut public_key_bytes);
 
         assert_eq!(
-            parse_metadata(upgrade_locations, &public_key_bytes, &metadata),
+            check_metadata(upgrade_locations, &public_key_bytes, &metadata),
             Ok(())
         );
 
         // Manipulating the partition address fails.
-        metadata[METADATA_SIGN_OFFSET] = 0x88;
+        metadata[METADATA_SIGN_OFFSET + 8] = 0x88;
         assert_eq!(
-            parse_metadata(upgrade_locations, &public_key_bytes, &metadata),
+            check_metadata(upgrade_locations, &public_key_bytes, &metadata),
             Err(StorageError::CustomError)
         );
-        metadata[METADATA_SIGN_OFFSET] = 0x00;
+        metadata[METADATA_SIGN_OFFSET + 8] = 0x00;
-        // Any manipulation of signed data fails.
+        // Wrong metadata length fails.
-        metadata[METADATA_LEN - 1] = 0x88;
         assert_eq!(
-            parse_metadata(upgrade_locations, &public_key_bytes, &metadata),
+            check_metadata(
+                upgrade_locations,
+                &public_key_bytes,
+                &metadata[..METADATA_LEN - 1]
+            ),
             Err(StorageError::CustomError)
         );
-        metadata[METADATA_LEN - 1] = 0xFF;
         // Manipulating the hash fails.
         metadata[0] ^= 0x01;
         assert_eq!(
-            parse_metadata(upgrade_locations, &public_key_bytes, &metadata),
+            check_metadata(upgrade_locations, &public_key_bytes, &metadata),
             Err(StorageError::CustomError)
         );
         metadata[0] ^= 0x01;
         // Manipulating the signature fails.
         metadata[32] ^= 0x01;
         assert_eq!(
-            parse_metadata(upgrade_locations, &public_key_bytes, &metadata),
+            check_metadata(upgrade_locations, &public_key_bytes, &metadata),
-            Err(StorageError::CustomError)
-        );
-        metadata[32] ^= 0x01;
-        // Manipulating the partition data fails.
-        upgrade_locations.write_partition(0, &[0x88; 1]).unwrap();
-        assert_eq!(
-            parse_metadata(upgrade_locations, &public_key_bytes, &metadata),
             Err(StorageError::CustomError)
         );
     }

tools/deploy_partition.py

@@ -181,6 +181,7 @@ def main(args):
   priv_key = load_priv_key(args.priv_key)
   metadata = create_metadata(firmware_image, partition_address, args.version,
                              priv_key)
+  partition = metadata + firmware_image
 
   if args.use_vendor_hid:
     patcher = patch.object(hid.base, "FIDO_USAGE_PAGE", 0xFF00)
@@ -200,7 +201,7 @@ def main(args):
 
     running_version = authenticator.get_info().firmware_version
     if args.version < running_version:
-      fatal(f"Can not write version {args.version} when version"
+      fatal(f"Can not write version {args.version} when version "
             f"{running_version} is running.")
     else:
       info(f"Running version: {running_version}")
@@ -208,8 +209,8 @@ def main(args):
     try:
       check_info(partition_address, authenticator)
       offset = 0
-      for offset in range(0, len(firmware_image), PAGE_SIZE):
+      for offset in range(0, len(partition), PAGE_SIZE):
-        page = firmware_image[offset:][:PAGE_SIZE]
+        page = partition[offset:][:PAGE_SIZE]
         info(f"Writing at offset 0x{offset:08X}...")
         cbor_data = {1: offset, 2: page, 3: hash_message(page)}
         authenticator.send_cbor(
@@ -217,22 +218,14 @@ def main(args):
             data=cbor_data,
         )
 
-      info("Writing metadata...")
-      # TODO Write the correct address when the metadata is transparent.
-      cbor_data = {2: metadata, 3: hash_message(metadata)}
-      authenticator.send_cbor(
-          OPENSK_VENDOR_UPGRADE,
-          data=cbor_data,
-      )
-
     except ctap.CtapError as ex:
       message = "Failed to upgrade OpenSK"
       if ex.code.value == ctap.CtapError.ERR.INVALID_COMMAND:
         error(f"{message} (unsupported command).")
       elif ex.code.value == ctap.CtapError.ERR.INVALID_PARAMETER:
-        error(f"{message} (invalid parameter, maybe a wrong byte array size?).")
+        error(f"{message} (invalid parameter).")
       elif ex.code.value == ctap.CtapError.ERR.INTEGRITY_FAILURE:
-        error(f"{message} (metadata parsing failed).")
+        error(f"{message} (data hash does not match slice).")
       elif ex.code.value == 0xF2:  # VENDOR_INTERNAL_ERROR
         error(f"{message} (internal conditions not met).")
       elif ex.code.value == 0xF3:  # VENDOR_HARDWARE_FAILURE