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

Make SBAT variable payload introspectable #483

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

chrisccoulson
Copy link
Collaborator

This is for after 15.6, but we'll probably add something like this to
our next shim in Ubuntu, so I'd at least like to agree on the correct
way to do this.

Given a set of EFI variables and boot assets, it should be possible
to compute what the value of PCR 7 will be on the next boot.

As shim manages the contents of the SbatLevel variable and this is
measured to PCR 7, export the payloads that shim contains in a new
COFF section (.sbatlevel) so that it can be introspected by code
outside of shim.

The new section works a bit like .vendor_cert - it contains a header
and then the payload. In this case, the header contains no size fields
because the strings are NULL terminated. Shim uses this new section
internally in set_sbat_uefi_variable.

The .sbatlevel section starts with a 4 byte version field which is
not used by shim but may be useful for external auditors if the
format of the section contents change in the future.

@chrisccoulson chrisccoulson force-pushed the add-sbatlevel-section branch 3 times, most recently from dd2877f to 4da89d0 Compare May 31, 2022 22:07
@julian-klode
Copy link
Collaborator

👍

Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

Some minor stuff inline.

sbat_var.S Outdated

# include "include/sbat_var_defs.h"

#if defined(ENABLE_SHIM_DEVEL)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to have this stuff in sbat_ver_defs.h?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I've put it there instead.

sbat_var.S Outdated
@@ -0,0 +1,46 @@
// SPDX-License-Identifier: BSD-2-Clause-Patent

# include "include/sbat_var_defs.h"
Copy link
Member

Choose a reason for hiding this comment

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

Extra whitespace between the '#' and "include".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed now.

sbat_var.S Outdated
SBAT_VAR_LATEST_REVOCATIONS
#endif /* ENABLE_SHIM_DEVEL */

.section .sbatlevel, "a", %progbits
Copy link
Member

Choose a reason for hiding this comment

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

Indentation from here down seems wonky. So far for asm files (i.e., cert.S) we've used hard tabs only. Some of these lines are using tabs, some spaces.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've fixed this to just use tabs now.

@frozencemetery
Copy link
Member

(Also, this needs rebased at some point - conflicts in sbat.h.)

Given a set of EFI variables and boot assets, it should be possible
to compute what the value of PCR 7 will be on the next boot.

As shim manages the contents of the SbatLevel variable and this is
measured to PCR 7, export the payloads that shim contains in a new
COFF section (.sbatlevel) so that it can be introspected by code
outside of shim.

The new section works a bit like .vendor_cert - it contains a header
and then the payload. In this case, the header contains no size fields
because the strings are NULL terminated. Shim uses this new section
internally in set_sbat_uefi_variable.

The .sbatlevel section starts with a 4 byte version field which is
not used by shim but may be useful for external auditors if the
format of the section contents change in the future.

Signed-off-by: Chris Coulson <[email protected]>
@chrisccoulson chrisccoulson force-pushed the add-sbatlevel-section branch from 4da89d0 to 6ff23a6 Compare June 8, 2022 19:59
Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

Thanks, no further objections.

@vathpela vathpela merged commit 0eb07e1 into rhboot:main Aug 3, 2022
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