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

Spec updates #25

Merged
merged 12 commits into from
Apr 24, 2024
Merged

Spec updates #25

merged 12 commits into from
Apr 24, 2024

Conversation

krystian-hebel
Copy link
Member

@krystian-hebel krystian-hebel commented Dec 19, 2023

This includes some, but not all of problems listed in #23.

This is done as first step to not have different 0.5.0 revisions
between this file and PDF version.

In addition to that, whitespaces issues and a typo was fixed.

Signed-off-by: Krystian Hebel <[email protected]>
These entity types are specific to Linux. This is now marked in their
names to avoid using them for other kernels.

Signed-off-by: Krystian Hebel <[email protected]>
@krystian-hebel krystian-hebel self-assigned this Dec 19, 2023
@krystian-hebel krystian-hebel marked this pull request as ready for review December 19, 2023 13:57
Copy link
Member

@SergiiDmytruk SergiiDmytruk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New constants' name needs to change either here or in GRUB and Xen. Other than that, there are a couple of suggestions.

@@ -7,7 +7,7 @@ Secure Launch Specification

.. class:: center

**Version:** 0.5.0
**Version:** 0.6.0-draft
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop draft

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, but will do after all of #23 are addressed (one way or another)

In the current version (one) of the specification, `TPM_EVENT_INFO_LENGTH` is
defined as 32 bytes. All unused bytes **MUST** be set to `\0`, but the string
**MAY** not be terminated with `\0` if it fills the whole `evt_info`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block should be moved to above the fields definitions and made into a .. note:: block

Note that **RECOMMENDED** solution is to just not include the entry in question,
this entity type is left as a final resort if entry has to be removed after SLRT
was created in memory and defragmenting it after removing an entry isn't
feasible.
Copy link
Contributor

@dpsmith dpsmith Jan 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to go back and review SLR_ET_UNUSED, as I don't think I necessarily agree with this sentiment.

data to be hashed by TPM. In such cases, `SLR_POLICY_FLAG_MEASURED` is set by
DCE for entries it measures, and DLME skips those. Note that all entries still
**MUST** be measured in order.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specifically to handle the situation caused by the Linux TPM maintainers demanding that the mainline code be used if we wanted to do measurements from the compress kernel. This was not possible since the mainline driver makes use of functionality not available and thus creates a situation where we need to measure structures before they are used but don't have a means to access the TPM at the time.

So the general case is to handle situations where a measurement is needed but TPM access is not available yet. BUT the delay of sending the measurement cannot cross measured entity boundaries. So one component cannot measure another and then put the event in the log to be sent to the TPM by that next component.

This should also be made a note block and place above the definitions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspected that to be the reason, included it as well. My original description was made mostly with SKL in mind.


Only some of the entry types can have `SLR_POLICY_IMPLICIT_SIZE` flag set. Such
entries have their `size` specified as zero, and they **SHALL** be measured as
described in Appendix A.
Copy link
Contributor

@dpsmith dpsmith Jan 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say "Only some", this is for the measurement of entities with well know sizes and variable sized entities that have size attribute.

Make a note block and place above the definition list

@@ -562,6 +589,9 @@ Saved MTRR State
struct txt_mtrr_pair mtrr_pair[TXT_VARIABLE_MTRRS_LENGTH];
};

In the current version (one) of the specification, `TXT_VARIABLE_MTRRS_LENGTH`
is defined as 32 entries. All fields in unused entries **MUST** be set to 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note block and move code block

In the current version (one) of the specification, `TPM_EVENT_INFO_LENGTH` is
defined as 32 bytes. All unused bytes **MUST** be set to `\0`, but the string
**MAY** not be terminated with `\0` if it fills the whole `evt_info`.

Appendix A: Measuring the DRTM Policy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to "Recommendations for Measuring"

Measuring the SLRT
------------------

In revision one of the SLRT, the only table that needs to be measured is the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a need to measure the SLRT, the recommendation is that the vendor info table, i.e. one of SLR_ENTRY_INTEL_INFO, SLR_ENTRY_AMD_INFO or SLR_ENTRY_ARM_INFO, is the only one that should be measured. The remainder of the SLRT is meta-data, ......

Measuring OS to MLE data
------------------------

Version 1 of the OS-MLE heap structure has no fields to measure. It just has
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Version 1 of the/The SLRT defined/

Copy link
Contributor

@rossphilipson rossphilipson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to fix the OS2MLE struct in patch "secure-launch-specification.rst: update OS2MLE structure" LGTM, thanks.

Reviewed-by: Ross Philipson [email protected]

@rossphilipson rossphilipson merged commit 22ef6da into master Apr 24, 2024
@krystian-hebel krystian-hebel deleted the spec_updates branch May 9, 2024 09:01
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.

4 participants