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
efi: AddPCRProfile support for PCRs 4 and 7 profiles #249
efi: AddPCRProfile support for PCRs 4 and 7 profiles #249
Conversation
d0335f8
to
fdb5796
Compare
fdb5796
to
1ba7336
Compare
1ba7336
to
e4c2c15
Compare
This adds the remaining missing pieces so that AddPCRProfile can generate boot manager code and secure boot policy profiles. This will replace the existing AddBootManagerProfile and AddSecureBootPolicyProfile APIs.
e4c2c15
to
94bdca6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did a first pass, some questions
signatureDBUpdateNoFirmwareQuirk, | ||
signatureDBUpdateFirmwareDedupIgnoresOwner} { | ||
// create branch by copying the root varBranch | ||
branch := *root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably missing something, how does the change propagate out of this function if below we operate on a copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copy maintains a copy of updates, and then each call to WriteVar
propagates those updates to rootVarsCollector
via its registerUpdates
callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see maybe the comment should be:
// create a branch per quirk by copying the root varBranch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a moment I was confused and thought that root.updates was a slice, that is not case, otherwise the shallow copy would not be safe, as append to the same slice again can produce confusing results depending on the allocation history of the slice. I don't think we have code here that shallow copy structs with slices in them?
} | ||
|
||
func (h *ubuntuCoreUKILoadHandler) MeasureImageStart(_ pcrBranchContext) error { | ||
// TODO: Add stuff that the kernel measures here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to fix this before things can be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't - snapd can continue to use the AddSystemdStubProfile
API in addition to this one, although that will be moved here in a follow up. That will be a fairly minor change though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's a good idea to leave a comment also about that:
// for now clients can continue using AddSystemdStubProfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, some small final comments
signatureDBUpdateNoFirmwareQuirk, | ||
signatureDBUpdateFirmwareDedupIgnoresOwner} { | ||
// create branch by copying the root varBranch | ||
branch := *root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see maybe the comment should be:
// create a branch per quirk by copying the root varBranch
} | ||
|
||
func (h *ubuntuCoreUKILoadHandler) MeasureImageStart(_ pcrBranchContext) error { | ||
// TODO: Add stuff that the kernel measures here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's a good idea to leave a comment also about that:
// for now clients can continue using AddSystemdStubProfile
signatureDBUpdateNoFirmwareQuirk, | ||
signatureDBUpdateFirmwareDedupIgnoresOwner} { | ||
// create branch by copying the root varBranch | ||
branch := *root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a moment I was confused and thought that root.updates was a slice, that is not case, otherwise the shallow copy would not be safe, as append to the same slice again can produce confusing results depending on the allocation history of the slice. I don't think we have code here that shallow copy structs with slices in them?
This adds the remaining missing pieces so that AddPCRProfile can
generate boot manager code and secure boot policy profiles. This will
replace the existing AddBootManagerProfile and AddSecureBootPolicyProfile
APIs.