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

shim: ShimLock protocol uses "sysv64" function ABI #227

Merged
merged 3 commits into from Jun 12, 2021

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Jun 1, 2021

See the systemd-boot declaration:
https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/boot/efi/shim.c#L23-L31

This is because the shim is built with sysv ABI funcs by defualt.

This PR also adds bindings to the hashing functionality of the shim. For more information see:

Signed-off-by: Joe Richey joerichey@google.com

@josephlr
Copy link
Contributor Author

josephlr commented Jun 1, 2021

CC: #226, @nicholasbishop

@nicholasbishop
Copy link
Contributor

Thanks for the fix!

Are the hash/context methods useful for other bootloaders? I had sort of thought they were just used by MOK manager and so basically internal to shim. I took a quick look around Grub and only saw the verify method being called, but I might have missed something.

@josephlr
Copy link
Contributor Author

josephlr commented Jun 2, 2021

they were just used by MOK manager and so basically internal to shim.

I'm not sure here. CC: @mjg59. Matthew, do you know if the non-verify methods of the Shim protocol are considered implementation details?

Are the hash/context methods useful for other bootloaders?

I can think of three uses:

  • Doing something similar to the MOK manager. Taking an EFI Binary and computing its Authenticode hash for enrollment w/ EFI_SIGNATURE_LIST/EFI_SIGNATURE_DATA
  • Some sort of boot-time Signer of EFI Binaries (which would require computing such a hash). However, most binaries would probably just do the parsing/hashing directly, as they might not be loaded by the SHIM.
  • A second stage bootloader might have its own type of MokList or Internal Certificates, so it might want to verify a binary but against a different root of trust.

They would certainly see less use than the Verify method itself.

@nicholasbishop
Copy link
Contributor

I verified that the Context struct matches up, and the new function pointers look correct to me other than my question about passing an array pointer. (I assume it's fine, since Rust knows array lengths at compile time it shouldn't be a fat pointer, but I don't know enough details to be sure.)

Maybe worth breaking this up into 3 PRs, one for each commit? Fixing the verify ABI seems uncontroversial, but I don't have enough knowledge to be sure about the rest.

@GabrielMajeri
Copy link
Collaborator

@nicholasbishop I think it's ok to keep it in the same PR. It's small enough to be manageable for review.

src/proto/shim/mod.rs Outdated Show resolved Hide resolved
See the systemd-boot declaration:
  https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/boot/efi/shim.c#L23-L31

This is because the shim is built with sysv ABI funcs by defualt.

Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Contributor Author

@nicholasbishop I think it's ok to keep it in the same PR. It's small enough to be manageable for review.

@GabrielMajeri let me know if you thing this PR needs anything else.

they were just used by MOK manager and so basically internal to shim.

I'm not sure here. CC: @mjg59. Matthew, do you know if the non-verify methods of the Shim protocol are considered implementation details?

Given that the MokManager uses these methods, removing/changing them in the shim wouldn't be backwards compatible, as the shim and MokManger don't have to exactly match in version. So I think it should be fine to rely on these two methods.

@GabrielMajeri
Copy link
Collaborator

Everything looks fine to me, I'm merging it

@GabrielMajeri GabrielMajeri merged commit 9db727c into rust-osdev:master Jun 12, 2021
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.

None yet

3 participants