Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly handle overflow when updating openhcl/underhill_attestation_protocol's vmgs::KeyProtector active_kp field #640

Open
mattbodd opened this issue Jan 9, 2025 · 1 comment
Assignees

Comments

@mattbodd
Copy link
Contributor

mattbodd commented Jan 9, 2025

KeyProtectors as defined in openhcl/openhcl_attesation_protocol (link) maintain the active KP index as a u32.

In openhcl/underhill_attestation/src/lib.rs, within persist_all_key_protectors, active KP is updated with a straightforward addition assignment operation, which will implicitly wrap when adding to u32::MAX. All current usages of the active KP index us modulo arithmetic which will handle arithmetic overflow gracefully and produce desired behavior.

Two suggestions for improvement

  1. Make active_kp an enum with ingress and egress variants that desugars to u32 (for VMGS file compatibility)
    • Pros: clearer than using an arbitrary index value, not susceptible to overflow
    • Cons: requires a bit of under-the-hood complexity to allow FromBytes and AsBytes to work for active_kp to be represented as u32 when reading/writing to an actual VMGS
  2. Explicitly use u32::wrapping_add (link) to make the intent clearer
    • Pros: makes wrapping behavior expectation explicit, minimal change
    • Cons: maintains implicit requirement for usage of active_kp to handle wrapping behavior
@mattbodd
Copy link
Contributor Author

mattbodd commented Jan 9, 2025

I can't assign this to myself, but I'll take ownership of this issue

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

No branches or pull requests

1 participant