Skip to content

Update ctaphid-dispatch to v0.3.0 and usbd-ctaphid to v0.3.0#615

Merged
robin-nitrokey merged 1 commit intomainfrom
message-size
Mar 26, 2025
Merged

Update ctaphid-dispatch to v0.3.0 and usbd-ctaphid to v0.3.0#615
robin-nitrokey merged 1 commit intomainfrom
message-size

Conversation

@robin-nitrokey
Copy link
Member

This makes ctaphid generic over the buffer size and synchronizes the buffer sizes between ctaphid-dispatch and usbd-ctaphid. As the usbd-ctaphid buffer size was only about half the size of the ctaphid-dispatch buffer size, this should lead to a significantly decreased stack usage in ctaphid-dispatch calls.

Depends on:

ctaphid-dispatch v0.2.0 is still in Cargo.lock because of webcrypt.

@nitrokey-ci
Copy link
Collaborator

nitrokey-ci commented Mar 21, 2025

No significant changes.

Insignifcant changes
metric value change
binary-size-nk3am 1,509,372 🔴 +13,119 (+0.88%)
binary-size-nk3am-test 1,880,860 🔴 +12,063 (+0.65%)
binary-size-nk3xn 540,960 🔴 +4,580 (+0.85%)
binary-size-nk3xn-test 541,024 🔴 +4,532 (+0.84%)
binary-size-nkpk 686,523 🔴 +5,543 (+0.81%)

@robin-nitrokey
Copy link
Member Author

This leads to a 5 kB binary size increase because we no longer use the same buffer size for ctaphid-dispatch and apdu-dispatch, so functions that are generic over the buffer size are duplicated. This mostly affects secrets-app and, to a lesser extent, admin-app, for example:

https://github.com/Nitrokey/trussed-secrets-app/blob/700863bdfa90a3616cbb695d6638c7aea7730c03/src/authenticator.rs#L285-L289

I see the following options:

  1. Accept the binary size increase for now with the outlook of an improvement once we can use something like a BytesView. Downside: It is not trivial to get back the binary size in case we need it before we have a BytesView.
  2. Use the same buffer size for ctaphid-dispatch and apdu-dispatch, effectively increasing stack usage for ctaphid. Downside: We don’t have metrics for stack usage at the moment and this could lead to unexpected problems that are hard to debug.
  3. Try to find a solution without this const-generic parameter – either directly using a &mut [u8] or by introducing a view type in heapless-bytes (without waiting for heapless). Downside: While this would be the cleanest solution, it would require some additional implementation effort.
  4. Try to optimize secrets-app to reduce the amount of duplicated code. Downside: Lots of implementation efforts that would be in vain once we have a BytesView.
  5. Postpone this change until a new heapless release with a view type. Downside: This would block the ctaphid-dispatch and usbd-ctaphid PRs and releases and thus also the PQC PRs, and we’ve already been waiting for a heapless release for more than a year.

I think (1) would be the easiest solution and a temporary increase in binary size would be acceptable. What do you think @sosthene-nitrokey @daringer?

This makes ctaphid generic over the buffer size and synchronizes the
buffer sizes between ctaphid-dispatch and usbd-ctaphid.  As the
usbd-ctaphid buffer size was only about half the size of the
ctaphid-dispatch buffer size, this should lead to a significantly
decreased stack usage in ctaphid-dispatch calls.
@robin-nitrokey robin-nitrokey marked this pull request as ready for review March 25, 2025 21:45
@robin-nitrokey robin-nitrokey merged commit 7f6d2cf into main Mar 26, 2025
11 checks passed
@robin-nitrokey robin-nitrokey deleted the message-size branch March 26, 2025 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments