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

Specification updates and fixes #23

Open
15 of 19 tasks
krystian-hebel opened this issue Oct 11, 2023 · 7 comments
Open
15 of 19 tasks

Specification updates and fixes #23

krystian-hebel opened this issue Oct 11, 2023 · 7 comments

Comments

@krystian-hebel
Copy link
Contributor

krystian-hebel commented Oct 11, 2023

This issue is made to gather all small fixes, improvements and other TODOs in one place before adding them to next specification revision. It will also be a place for conversation about proposed changes. After enough changes are listed (or a change significant enough to bump specification revision) we can push them all in one PR to keep version in source file synchronized with produced PDFs.

I (or anyone with permissions to edit the issue message) will periodically update the list below to have all the changes in one place. Feel free to put your suggestions in the comment.


  • Appendix B ( Intel TXT OS2MLE) has struct slr_table *slrt; - size of pointer should be explicitly specified
  • Document what SLR_ET_UNUSED can be used for - https://github.com/TrenchBoot/grub/pull/13/files#r1354751740
  • Change value of SLR_ET_UNSPECIFIED - https://github.com/TrenchBoot/grub/pull/13/files#r1354869835
  • Values of TXT_VARIABLE_MTRRS_LENGTH and TPM_EVENT_INFO_LENGTH are not specified (both are 32 in implementation).
  • Not specified whether {slr_policy_entry,slr_uefi_cfg_entry}::evt_info must include trailing \0 or it can be omitted if label fills the whole buffer.
  • 2.2 Acronyms doesn't expand SLRT.
  • Meaning of SLR_POLICY_* and SLR_ET_* aren't explained. With SLR_ET_SLRT marked as required.
  • SLR_ET_* aren't always self-describing, they were made with Linux in mind and some names should reflect this - multiboot2: Switch to using SLRT interface grub#14 (comment)
    • Add entry for Multiboot2 modules.
  • UEFI Config Entry has "address of value" field which need to be explained/updated. It's also not specified what kind of address is expected.
    • It is "address or value", I can't read
  • "TPM Extend Operation" section might contain off-by-one error (0..n instead of 0..n-1).
  • "TPM Extend Operation" section should explain that | denotes concatenation instead of bitwise OR.
  • Description of SLR_ET_SLRT should reference "Appendix A".
  • Entry_n = PCR_n | EntityType_n | EventInfo_n in "Measuring the Policy" should use i for index.
  • Describe the format (entry point, unused cmdline) and location of MLE header (kernel_info for Linux, whatever we'll choose for Multiboot2).
  • mtrr_vcnt array in description of mtrr_vcnt should say mtrr_pair array instead.
  • Add grub_uint64_t dlme_base; and grub_uint32_t dlme_size; to struct slr_entry_dl_info.
  • Change type of dlme_entry in struct slr_entry_dl_info to grub_uint32_t (see).
    • Won't do, this would either break alignment or introduce unnecessary padding.
  • Probably worth referencing https://trustedcomputinggroup.org/resource/d-rtm-architecture-specification/ at least as a source of the nomenclature.
@krystian-hebel
Copy link
Contributor Author

krystian-hebel commented Jan 15, 2024

About a half of currently listed issues was addressed in #25. The rest is planned to land in the specification together with changes specific to AMD processors.

@krystian-hebel
Copy link
Contributor Author

@SergiiDmytruk I think there is no off-by-one error in "TPM Extend Operation". E_0 describes extend operation for Obj_0 and E_n - for Obj_n, not Obj_(n-1). Can we remove that from the list above?

I'm also not sure about using i for index in "Measuring the Policy". Is this just to differentiate those from names used in "TPM Extend Operation"?

@SergiiDmytruk
Copy link
Member

I think there is no off-by-one error in "TPM Extend Operation". E_0 describes extend operation for Obj_0 and E_n - for Obj_n, not Obj_(n-1). Can we remove that from the list above?

I meant that the way it's written there are (n + 1) objects in the range [0;n] (both ends included), instead of n of them ([0;n) or [1; n]). Same in "Measuring the policy".

I'm also not sure about using i for index in "Measuring the Policy". Is this just to differentiate those from names used in "TPM Extend Operation"?

If there is only Entry_n = ... and Entry_0, ..., Entry_n, it looks like definitions of Entry_0 through Entry_{n-1} are missing. i usually suggests iteration, n boundary.

@krystian-hebel
Copy link
Contributor Author

If there is only Entry_n = ... and Entry_0, ..., Entry_n, it looks like definitions of Entry_0 through Entry_{n-1} are missing.

But for the policy it isn't defined through Entry_{n-1}

@SergiiDmytruk
Copy link
Member

But for the policy it isn't defined through Entry_{n-1}

How come? Isn't M_policy a result of extending 0 and Entry_0, then extending result with Entry_1, etc. up to the last entry?

@krystian-hebel
Copy link
Contributor Author

Isn't M_policy a result of extending 0 and Entry_0, then extending result with Entry_1, etc. up to the last entry?

Hmm, I understood it as concatenation of all entries (subset of their fields), but there is a different symbol for concatenation, so maybe you're right. @dpsmith @rossphilipson what are your opinions on this?

@SergiiDmytruk
Copy link
Member

Hmm, I understood it as concatenation of all entries (subset of their fields), but there is a different symbol for concatenation, so maybe you're right.

The reason for my interpretation is definition of E() in 5.1 which is recursive when applied to an array of objects.

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

2 participants