The spec for the GetPfmId Request says that the identifier field is optional. This is because it defaults to Identifier::Version if not provided. We can save sending this field over wire in this case.

Currently, some tests have #[cfg_attr(miri, ignore)] because they call into ring which calls into some assembly.

The best way to deal with this is probably to mock out all of the ring components to do nothing when #[cfg(miri)] holds.

We need a generic facility for tracking pairs of "pending" and 'active" manifests of a particular type (this is used mostly for manifest updates, and is referenced in protocol messages that distinguish between pending and active.

This needs some design work on my part.

Here the use case:

1. Host sends request to device via SPI "WRITE"
2. Device sets SPI "BUSY" bit
3. Host continuously polls SPI "BUSY" bit - potentially forever.
4. Device processes incoming requests.
5. Device generates response and stores it in SPI HW buffer
6. Device clears SPI "BUSY" bit. This releases the SPI interface for subsequent transactions.
7. Host sees SPI "BUSY" bit cleared.
8. Host performs SPI "READ"

The SPI flash protocol requires the "BUSY" bit to be set for a "WRITE" transaction. If the host expects a response for a particular request, it'll perform a "READ" after the "BUSY" bit cleared. One option would be to just send and "invalid" response, e.g. all bytes 0xFF, which would be the default for an "empty SPI flash region".

But it might be useful to give the host a positive response to indicate if there was an error.

Currently, we ignore:

• The signature type in the manifest container.
• Allowable Firmware flags.
• The "manifest type", "signature type", and "hash type" bytes in the owned::Container parser.

These are defined in sections 6.22 and 6.23 of the Challenge Protocol Specification. Implementing support for them in the PA-RoT server is pending on #43, but implementing the message parsing itself can be done before that.

Since we would be implementing these on top of the FPM, it's unclear if we want to re-use the Cerberus message type bytes for these, or use our own until we reconcile the PFM and our FPM. We implement the PFM now, so this bit is irrelevant. =)

This is a straight-forward device state request defined in section 6.21 of the Challenge Protocol Specification.

• Protocol Messages
  • device_id
  • device_info
  • device_uptime
  • capabilities
  • firmware_version
  • reset_counter
  • request_counter
• Manifests
  • [ ] FPM Deferred until after release.

It seems that clippy in CI is not running over tests:

    - name: Check clippy lints
      run: cargo clippy --verbose

I think it might be useful to have an additional:

 - name: Check clippy lints
   run:` cargo clippy --tests

Unless --tests covers both cases, in which case we could just add --test to the existing run

Right now, they're just raw bytes. We aught to make it possible to specify them as explicitly as enumeration values or similar.

Currently unsupported example

make_fuzz_safe! {
    pub enum MyTupleVariant<'a> {
        Foo(u32),
        Bar(&'a [u8]),
    }
}

We'd like to test our parser on the same blobs that Microsoft uses, rather than just generating blobs on the fly. Doing this requires full conformance with their format though, and we're not quite there yet.

Pesudocode for the algorithm:

fn verify_policy(
  fpm: Fpm,
  flash: &impl Flash,
  hash_builder: &impl ha256::Builder
) -> Result<FirmwareVersion> {
  for fw in fpm.fw_versions {
    let version = flash.read(fw.version_addr, fw.version_len);
    if version != fw.version {
      continue
    }
    let mut sha = hash_builder.new();
    for (ptr, len) in fw.signed_regions {
      // Note: this is grossly exaggerated. There is an expectation that
      // data will be fed into the hashing engine in small packets (O(1K),
      // for example.
      sha.write(flash.read(ptr, len));
    }
    if fw.signed_region_hash != sha.finish() {
      return Err("hash mismatch");
    }

    let unused_regions = regions_except(flash, [fw.signed_regions,
                                                fw.write_regions]);
    for (ptr, len) in unused_regions {
      for byte in flash.read(ptr, len) {
        return Err("bad byte in blank region");
      }
    }
    return fw
  }
  return Err("failed to find an acceptable version")
}

See: https://github.com/lowRISC/misc-linters/blob/master/licence-checker/licence-checker.py

This should probably be easy to tackle; a git submodule is probably unnecessary.

#123 added a crate-wide Error type and some helper macros to add logs gated by the log feature. One could use the new macros everywhere to provide additional context to errors for debugging. But it might be possible to use #[track_caller] and core::panic::Location to automatically track and log where errors originate and where they get converted to other error types, eliminating a bunch of tedious work. Investigate this and implement if possible.

References

• rust-lang/rust#87401
• #123 (comment)

On some devices the maximum size of a "package" that can be transferred on a particular interface is smaller than the maximum supported message size. We need a transfer protocol that works across various interface types (e.g. I2C, SPI) that allows the transfer of "large" messages across multiple "transactions" on the particular interface. This transfer protocol can also be used for smaller messages that fit into one "package" to indicate the size of the message.

Currently, we fail if the type is anything but SHA-256; we should fix that for conformance.

Currently, there's a bit of a problem with io::Read where we must allocate anything variable-length, even if the underlying Read impl can buffer it. It would be great to copy the same function out of Flash, but unfortunately there is a small problem:

fn read_direct<'a: 'c, 'b: 'c, 'c>(
    &'a self,
    region: Region,
    arena: &'b dyn Arena,
    align: usize,
) -> Result<&'c [u8], Error>;

This function takes &self, whereas Read's functions want &mut. Unfortunately, doing this causes the returned reference to pin the Read indefinitely, even though that memory has been "essentially disowned" by the Read impl. One option might be to pursue what we did with Flash originally, and introduce

trait ReadZero<'a>: Read {
  fn read_direct<'b: 'c, 'c>(
     &'a self,
     bytes: usize,
     arena: &'b dyn Arena,
     align: usize,
  ) -> Result<&'c [u8], Error>
  where
    'a: 'c;
}

We can then impl<'a> ReadZero<'a> for &'a [u8]. Perhaps this is the right answer, but I'm not sure if pulling the lifetime out like that will work in the way we'd like it to...

(Pulling out the lifetime kind-of makes sense, because e.g. FromWire already has an ambient lifetime. It's unclear if there is such a thing for PaRot and friends though.)

In order to facilitate low level testing and debugging, we need a tool that allows to generate requests in wire format as well as decode both requests and responses from wire format.

• [ ] Get Log Info, Log Info (Definitions and handler implementation) Defered to post-release.
• Device Information (Handler implementation)

Note that the logging-related messages should not be considered to block release.

Currently, manticore::io::Read<'a> behaves like a "grant me parts of your buffer":

trait Read<'a> {
  // Reads `len` bytes; always blocks.
  fn read_bytes(&mut self, len: usize) -> Result<&'a [u8]>;
}

This is kind of a weird interface, but it's informed by the design of FromWire:

trait FromWire<'a> {
  fn from_wire<R: Read<'a>>(r: R) -> Result<Self>;
}

This allows FromWire implementations to bind any lifetimes in their definition to the Read<'a> the data comes from.

In practice, this is a problem: it means that Read<'a> needs to allocate memory to buffer its entire message, which is undesirable. This also makes Read<'a> implementations, often the purview of an integration, needlessly complicated.

An alternative solution to the above FromWire signature is the following:

trait FromWire<'a> {
  fn from_wire<R: Read, A: Arena>(r: R, arena: &'a mut A) -> Result<Self>;
}

The new trait Arena can be imagined as a thin wrapper over split_at_mut:

trait Arena {
  fn alloc(&mut self, bytes: usize) -> Result<&mut [u8]>;
}
impl Arena for &mut [u8] { ... }

Then, we would be able to change Read to look a lot more like std's version:

trait Read {
  // Reads `out.len()` bytes; always blocks.
  fn read_bytes(&mut self, out: &mut [u8]) -> Result<()>;
}

The main upshot from this change is that an implementation of Read doesn't need to buffer entire messages, while implementations of FromWire can buffer only what they absolutely have to:

impl<'a> FromWire<'a> for FirmwareVersionResponse<'a> {
  fn from_wire<R: Read, A: Arena>(r: R, arena: &'a mut A) -> Result<Self> {
    let version = arena.alloc(32)?;
    r.read_bytes(&mut *version)?;
    Self { version }
  }
}

While this does introduce a copy into FromWire, this is a copy any non-trivial Read implementation was already doing. Read implementations can now simply be think wrappers over syscalls, like they should have been in the first place.