microsoft / microsoft/openvmm

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

Open
#640 1 comment 0 reactions 1 assignee View on GitHub

@mattbodd is already working on this.

Since Jan 9, 2025.

testing
Dominant language
Rust
Stars
1.9k
Forks
238
Avg merge
1d 15h
Merged PRs (30d)
100

Description

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

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.