Discussion about usage in tpm-rs project

[jettr]

This is a really nice library, and it seems like it would compliment the tpm-rs project quite well. I wanted to bring up a few topics to get your opinion on:

1. In the generated code for TpmBufferRef (copied directly below) I noticed an Unstable and Stable variant.

#[derive(Clone, Debug)]
pub enum TpmBufferRef<'a> {
    Unstable(&'a [u8]),
    Stable(&'a [u8]),
}

From the README, you mention this library can handle "memory contents [that] are stable or susceptible to modify-after-validate attacks", and I am guessing this is how you describe this. However, if there is a valid rust reference to &[u8], the underlying data cannot/should not change otherwise you will have undefined behavior in rust. I think we should either drop the Unstable/Stable concept or if we really need it, we should drop down to raw pointers and use volatile_read/volatile_write. I also might be conflating two different things, so let me know what your thoughts are.

2. In the README you mention that this is "[no_std] compatible", but when I generated the code with the below command, I see references to Vecs and Boxs. I might have just generated it wrong though.

gen-tpm2-cmd-interface      \
    -t tpm2_algorithms.csv  \
    -t tpm2_structures.csv  \
    -t tpm2_commands.csv    \
    -t tpm2_vendor.csv      \
    -u '.*_COMMAND_PARAMS   \
    -u '.*_COMMAND_HANDLES  \
    -m '.*_RESPONSE_PARAMS  \
    -m **'.*_RESPONSE_HANDLES

I agree that there is a need for temporary "heap" space while processing TPM commands, but we were thinking about using buffer arenas instead (example arenas).

3. Lastly, I hope that we can add a goal of ensuring that none of the generated code has panic paths. For example the core library function split_at has a cold panic path, and you need to either drop down to unsafe or use some kind of other work around to eliminate the panic path. Panic paths 1) cost a decent amount of flash space and 2) give escape vector in the code that we otherwise want to avoid. We have some strategies to help ensure that panics are not present in a code -- but there is nothing in the rust ecosystem so far that provides 1st class support for eliminating panics.

[nicstange]

1. In the generated code for TpmBufferRef (copied directly below) I noticed an Unstable and Stable variant.

#[derive(Clone, Debug)]
pub enum TpmBufferRef<'a> {
    Unstable(&'a [u8]),
    Stable(&'a [u8]),
}

From the README, you mention this library can handle "memory contents [that] are stable or susceptible to modify-after-validate attacks", and I am guessing this is how you describe this.

Yes, FWIW, I had something like an externally mapped communication buffer in my mind. I.e. a command buffer mapped by both the client and the TPM, and which could potentially get modified from under the TPM while it is in the process of unmarshalling the command params. It could have saved a buffer copy in certain situations, but for e.g. HMAC sessions with digested command params, the caller better should have stabilized the buffer anyway before verifying the MAC anyway. So let's consider this a failed experiment, drop the Unstable buffer concept and move on.

However, if there is a valid rust reference to &[u8], the underlying data cannot/should not change otherwise you will have undefined behavior in rust.

Right.

I think we should either drop the Unstable/Stable concept or if we really need it, we should drop down to raw pointers and use volatile_read/volatile_write.

As said, I think at this point it would be better to just ditch the whole concept. We're always free to reimplement it, should it ever be needed.

2. In the README you mention that this is "[no_std] compatible", but when I generated the code with the below command, I see references to Vecs and Boxs. I might have just generated it wrong though.

Not sure I understand the concern, if I'm not mistaken, it is possible to have Vecs and Boxs in [no_std] environments -- IIRC all it takes is for the environment to provide a global allocator (and to add an extern crate alloc to the code using it).

In this context it is noteworthy that the generated code does rely on Box::try_new(), which isn't in Rust stable yet. I was hoping for that to have stabilized by the time this code generator here would hit its first real world use, but perhaps some temporary workaround needs to be implemented.

I agree that there is a need for temporary "heap" space while processing TPM commands, but we were thinking about using buffer arenas instead (example arenas).

Wouldn't there also be a need for non-temporary allocations, like e.g. for sessions state, by other components anyway? I'm not against using a different heap management method for the generated interface code, but I would naively expect that any non-trivial execution environment would have to provide an implementation of the standard Rust allocator interface for one reason or another. So the question is, if my assumption was true, would there still be a strong reason to go with a custom scheme like arena allocators? (Note to myself: the experimental Rust [allocators_api] could provide a way to make the generated code generic over allocators).

3. Lastly, I hope that we can add a goal of ensuring that none of the generated code has panic paths. For example the core library function split_at has a cold panic path, and you need to either drop down to unsafe or use some kind of other work around to eliminate the panic path. Panic paths 1) cost a decent amount of flash space and 2) give escape vector in the code that we otherwise want to avoid. We have some strategies to help ensure that panics are not present in a code -- but there is nothing in the rust ecosystem so far that provides 1st class support for eliminating panics.

Admittedly, I'm lacking the background here. I'm all for eliminating panics where possible, but I think that makes sense only as long as it wouldn't subvert the Rust memory safety guarantees. E.g. going with the split_at() example, how is this conceptually different from a plain slice index access, for which Rust would to my knowledge also emit a panic if that happened to be OOB? Either way, we could surely try to make the generated code panic free, I'm only a bit sceptical that would work out to our satisfaction in the end. But happy to be proven wrong, do you have pointers to further info on the panic avoidance strategies you mentioned?

That being said, the potentially panicking buffer accesses (plain + split_at()/consume()) should always be preceeded by a corresponding explicit bounds check on the remaining buffer lengths, so the compiler might actually be able to conclude the panic paths are dead code and eliminate them, but I'd have to check it's indeed smart enough for doing that.

[jettr]

Sounds good on removing the Stable/Unstable concepts.

It also sounds like you want to only target stable rust features, that is a goal of tpm-rs as well.

I would naively expect that any non-trivial execution environment would have to provide an implementation of the standard Rust allocator interface for one reason or another

In our embedded rust firmware binary, we do not use or have access to any global allocators. In my experience, embedded tries to stay away from dynamic memory allocation and instead favors static memory allocation. The TockOS embedded OS is a good example of a large rust code base targeting embedded that does not use global allocators.

The TPM implementation is going to have to have some sort of dynamic allocation, but using an explicit solution like arenas will give the users of tpm-rs fine grained control over the allocation and clean up,

I'm all for eliminating panics where possible, but I think that makes sense only as long as it wouldn't subvert the Rust memory safety guarantees.

+100. We should never cause UB in our safe or unsafe rust code :)

E.g. going with the split_at() example, how is this conceptually different from a plain slice index access, for which Rust would to my knowledge also emit a panic if that happened to be OOB?

You are correct that my_slice[100] = 5; indeed has a panic path if my_slice has less than 100 elements. The non-panicing code would look like

  if Some(v) = my_slice.get_mut(100) else {
    return Err(OutOfBoundsTpmError);
  }
  *v = 5;

or

   *my_slice.get_mut(100).ok_or(OutOfBoundsTpmError)? = 5;

The out-of-bounds case needs to be handled explicitly for there to be no panic path. If you had an array type, e.g. [u8; 50], then the compiler actually checks at compile time for constant offset access (and will even give you a build error if you try to access outside the range).

Unfortunately at the moment there is no stable version of split_at_mut that does not have a panic path. Other libraries have had to implement their own try_split_at using unsafe under the hood until rust stabilizes something that we can use.

---

Another thing I meant to ask you about was if you would be interested in checking in your code within the tpm-rs project? I haven't asked around on the tpm-rs side yet, but I doubt there would be concerns if you were willing to check in your code there. I know you would be giving up some of the ownership/control/maintenance if you did that, so I wanted to see what your thoughts were first. If you prefer to keep everything separate, we will need to figure out how to pull interop between tpm-rs and this repo smoothly. Either option is definitely okay :)

[nicstange]

Sounds good on removing the Stable/Unstable concepts.

Done.

It also sounds like you want to only target stable rust features, that is a goal of tpm-rs as well.

Yes, definitely.

I would naively expect that any non-trivial execution environment would have to provide an implementation of the standard Rust allocator interface for one reason or another

In our embedded rust firmware binary, we do not use or have access to any global allocators. In my experience, embedded tries to stay away from dynamic memory allocation and instead favors static memory allocation. The TockOS embedded OS is a good example of a large rust code base targeting embedded that does not use global allocators.

The TPM implementation is going to have to have some sort of dynamic allocation, but using an explicit solution like arenas will give the users of tpm-rs fine grained control over the allocation and clean up,

I must admit I'd prefer to avoid hard-coding the generated code against a given allocator XY implementation, mainly because the current code generator might potentially be useful for other projects, like client libraries, as well. That being said, if that would turn out inevitable, so be it.

However, before taking that route, I would like to discuss an IMO more generic approach as an alternative, namely Rust Allocators. These are experimental and likely will stay for quite some more time, but there is a "transitional" crate available which would enable usage with stable Rust in the meantime: allocator-api2. (Besides Allocator, a couple of other experimental features handy for generating panic-free code would become available, too, like Vec::push_within_capacity() or Box::try_new_in()).

The main downside would be the additional dependency on said transitional crate, and, in the specific case of arena allocators, some memory overhead in each Vec instance, as they would probably contain a pointer to a refcounted arena Allocator instance each.

The advantage OTOH would be more flexibility and, once, the allocators have been stabilized, a better interoperability with the overall Rust core library.

In either case, I implemented everything needed to make the generated code generic over Allocators, you will find the changes in the allocators branch. Re panic-freeness of the generated code, note that there's a --enable-unsafe-panic-free CLI flag now.

Please let me know your opinion on this whole approach.

Unfortunately at the moment there is no stable version of split_at_mut that does not have a panic path. Other libraries have had to implement their own try_split_at using unsafe under the hood until rust stabilizes something that we can use.

FWIW, I just noticed that at least with nightly Rust and no experimental features enabled, split_at_mut_unchecked() seems to have become usable very recently.

Another thing I meant to ask you about was if you would be interested in checking in your code within the tpm-rs project? I haven't asked around on the tpm-rs side yet, but I doubt there would be concerns if you were willing to check in your code there. I know you would be giving up some of the ownership/control/maintenance if you did that, so I wanted to see what your thoughts were first. If you prefer to keep everything separate, we will need to figure out how to pull interop between tpm-rs and this repo smoothly. Either option is definitely okay :)

I personally think it would be nice to move this utility to under the tpm-rs GH umbrella.