raito/lanzaboote
0
0
Fork 0
Code Issues 45 Pull requests 6 Projects Releases 6 Packages Wiki Activity Actions

feat: minimal poc for exporting UEFI variables à la sd-boot #166

Merged
RaitoBezarius merged 2 commits from sd-stub-efi-variables into master 2023-05-05 19:32:50 +00:00
Conversation 33 Commits 2 Files changed 5 +324 -7
RaitoBezarius commented 2023-04-29 01:52:28 +00:00 (Migrated from github.com)
Copy link

@nikstur be happy now :>

We only need to polyfill some features, @blitz let me know what is the potential for upstreaming to uefi-rs.

internal todolist:

  • figure out the lifetime for the temporary strings set in export_efi_variables

  • prepare what should be upstreamed to uefi-rs

  • figure out error handling

  • add a test for EFI variables

  • TPM measurements are out of scope for this PR and variables related to them too.

  • Random seed support is out of scope for this PR

This implements a substantial part of #94.

@nikstur be happy now :> We only need to polyfill some features, @blitz let me know what is the potential for upstreaming to uefi-rs. internal todolist: - [x] figure out the lifetime for the temporary strings set in export_efi_variables - [x] prepare what should be upstreamed to uefi-rs - [x] figure out error handling - [x] add a test for EFI variables - TPM measurements are out of scope for this PR and variables related to them too. - Random seed support is out of scope for this PR This implements a substantial part of #94.
blitz (Migrated from github.com) reviewed 2023-04-29 01:52:28 +00:00
nicholasbishop (Migrated from github.com) reviewed 2023-04-29 03:20:56 +00:00
rust/stub/src/uefi_helpers.rs
nicholasbishop (Migrated from github.com) commented 2023-04-29 03:20:56 +00:00
Copy link

You can simplify this a bit with the guid macro:

pub const SD_LOADER: VariableVendor = VariableVendor(guid!("4a67b082-0a4c-41cf-b6c7-440b29bb8c4f"));
You can simplify this a bit with the `guid` macro: ```rust pub const SD_LOADER: VariableVendor = VariableVendor(guid!("4a67b082-0a4c-41cf-b6c7-440b29bb8c4f")); ```
RaitoBezarius (Migrated from github.com) reviewed 2023-04-29 03:21:43 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-04-29 03:21:43 +00:00
Copy link

Thank you!

Thank you!
RaitoBezarius commented 2023-04-30 00:08:21 +00:00 (Migrated from github.com)
Copy link

This is ready for review @blitz @nikstur.

I added a test which should explain what happens here in the userspace and should ensure we have the expected semantics.

Here's the specification for the EFI variables: https://www.freedesktop.org/software/systemd/man/systemd-stub.html#EFI%20Variables (we can share and we are recommended to share their Vendor ID AFAIK).

This is ready for review @blitz @nikstur. I added a test which should explain what happens here in the userspace and should ensure we have the expected semantics. Here's the specification for the EFI variables: https://www.freedesktop.org/software/systemd/man/systemd-stub.html#EFI%20Variables (we can share and we are recommended to share their Vendor ID AFAIK).
nikstur (Migrated from github.com) approved these changes 2023-05-01 22:18:25 +00:00
nikstur (Migrated from github.com) left a comment
Copy link

Looks good. Added some ideas for improvement. These can also be addressed later. This definetely improves the status quo.

Can we maybe get some lovely Nix-style commit message, e.g. "stub: export boot loader interface efivars"

Looks good. Added some ideas for improvement. These can also be addressed later. This definetely improves the status quo. Can we maybe get some lovely Nix-style commit message, e.g. "stub: export boot loader interface efivars"
nix/tests/lanzaboote.nix
nikstur (Migrated from github.com) commented 2023-05-01 21:55:28 +00:00
Copy link

We don't really need this assignment, do we?

We don't really need this assignment, do we?
nix/tests/lanzaboote.nix
@ -270,0 +302,4 @@
)
SD_LOADER_GUID = "4a67b082-0a4c-41cf-b6c7-440b29bb8c4f"
expected_variables = ["LoaderDevicePartUUID",
nikstur (Migrated from github.com) commented 2023-05-01 22:01:54 +00:00
Copy link

A few of these variables might be/will be set by the boot loader. The stub would only set them if the boot loader didn't. So if we wanted to test this fully we would need to get rid of sd-boot and boot the stub directly (e.g. via a custom bootloader).

A few of these variables might be/will be set by the boot loader. The stub would only set them if the boot loader didn't. So if we wanted to test this fully we would need to get rid of sd-boot and boot the stub directly (e.g. via a custom bootloader).
rust/stub/src/main.rs
nikstur (Migrated from github.com) commented 2023-04-30 23:26:00 +00:00
Copy link
        if !features.contains(SystemdLoaderFeatures::RandomSeed) {

I think this is supposed to run if the boot loader does not support the random seed. Otherwise the random seed is processed twice (i.e. combined with the systemd token, hashed and then written to into an efivar): first by the boot loader, then by the stub. See https://github.com/systemd/systemd/blob/main/src/boot/efi/stub.c#L208

```suggestion if !features.contains(SystemdLoaderFeatures::RandomSeed) { ``` I think this is supposed to run if the boot loader does not support the random seed. Otherwise the random seed is processed twice (i.e. combined with the systemd token, hashed and then written to into an efivar): first by the boot loader, then by the stub. See https://github.com/systemd/systemd/blob/main/src/boot/efi/stub.c#L208
rust/stub/src/part_discovery.rs
nikstur (Migrated from github.com) commented 2023-04-30 21:39:25 +00:00
Copy link

All of this could go into the efivars.rs module since we don't need it for any other purpose, right?

All of this could go into the `efivars.rs` module since we don't need it for any other purpose, right?
rust/stub/src/uefi_helpers.rs
nikstur (Migrated from github.com) commented 2023-04-30 23:28:50 +00:00
Copy link

Maybe we can find a more neutral name for this struct? These flags are defined by the BootLoaderInterface and in theory this could be implemented by other bootloaders. So maybe we should just call the struct EfiLoaderFeatures (this is what systemd does) or LoaderFeatures.

Maybe we can find a more neutral name for this struct? These flags are defined by the [BootLoaderInterface](https://systemd.io/BOOT_LOADER_INTERFACE/) and in theory this could be implemented by other bootloaders. So maybe we should just call the struct `EfiLoaderFeatures` ([this is what systemd does](https://github.com/systemd/systemd/blob/main/src/fundamental/efivars-fundamental.h#L12)) or `LoaderFeatures`.
nikstur (Migrated from github.com) commented 2023-04-30 23:29:29 +00:00
Copy link

Same as with the loader: EfiStubFeatures or StubFeatures?

Same as with the loader: `EfiStubFeatures` or `StubFeatures`?
nikstur (Migrated from github.com) commented 2023-05-01 22:06:35 +00:00
Copy link
    ensure_efi_variable(runtime_services,
```suggestion ensure_efi_variable(runtime_services, ```
nikstur (Migrated from github.com) commented 2023-05-01 22:11:00 +00:00
Copy link

The changes in this module deserve their own module: maybe evifars.rs?

The changes in this module deserve their own module: maybe `evifars.rs`?
nikstur (Migrated from github.com) commented 2023-05-01 22:12:38 +00:00
Copy link
pub const BOOT_LOADER_VENDOR_UUID: VariableVendor = VariableVendor(guid!("4a67b082-0a4c-41cf-b6c7-440b29bb8c4f"));
```suggestion pub const BOOT_LOADER_VENDOR_UUID: VariableVendor = VariableVendor(guid!("4a67b082-0a4c-41cf-b6c7-440b29bb8c4f")); ```
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:17:18 +00:00
rust/stub/src/part_discovery.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:17:18 +00:00
Copy link

Correct

Correct
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:17:36 +00:00
rust/stub/src/main.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:17:35 +00:00
Copy link

Good point, 0-4AM hacking gets you this kind of stuff :C

Good point, 0-4AM hacking gets you this kind of stuff :C
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:18:00 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:18:00 +00:00
Copy link

Agreed

Agreed
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:18:04 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:18:04 +00:00
Copy link

EfiStubFeatures sounds good to me

`EfiStubFeatures` sounds good to me
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:18:23 +00:00
nix/tests/lanzaboote.nix
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:18:23 +00:00
Copy link

Correct, I did by extreme correctness, I am planning to add those methods natively in the QEMU test driver in the future

Correct, I did by extreme correctness, I am planning to add those methods natively in the QEMU test driver in the future
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:18:33 +00:00
nix/tests/lanzaboote.nix
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:18:32 +00:00
Copy link

(actually I already opened a PR I believe somewhere in nixpkgs)

(actually I already opened a PR I believe somewhere in nixpkgs)
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:18:46 +00:00
nix/tests/lanzaboote.nix
@ -270,0 +302,4 @@
)
SD_LOADER_GUID = "4a67b082-0a4c-41cf-b6c7-440b29bb8c4f"
expected_variables = ["LoaderDevicePartUUID",
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:18:45 +00:00
Copy link

We should definitely do that in those tests

We should definitely do that in those tests
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:19:02 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:19:01 +00:00
Copy link

Agreed

Agreed
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 06:19:12 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 06:19:11 +00:00
Copy link

Agreed

Agreed
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 08:10:49 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 08:10:49 +00:00
Copy link

Do you mean that error while exporting this variable is fatal?

Do you mean that error while exporting this variable is fatal?
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 08:26:59 +00:00
nix/tests/lanzaboote.nix
@ -270,0 +302,4 @@
)
SD_LOADER_GUID = "4a67b082-0a4c-41cf-b6c7-440b29bb8c4f"
expected_variables = ["LoaderDevicePartUUID",
RaitoBezarius (Migrated from github.com) commented 2023-05-02 08:26:59 +00:00
Copy link

Did a trick to do it

Did a trick to do it
nikstur (Migrated from github.com) reviewed 2023-05-02 09:16:02 +00:00
rust/stub/src/uefi_helpers.rs
nikstur (Migrated from github.com) commented 2023-05-02 09:16:01 +00:00
Copy link

No, I mean we don't need the no-op let _ = assignment. Either way the result of the function is just ignored.

No, I mean we don't need the no-op `let _ =` assignment. Either way the result of the function is just ignored.
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 21:08:51 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 21:08:51 +00:00
Copy link

This will generate a warning?

This will generate a warning?
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 21:09:18 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 21:09:18 +00:00
Copy link

Do you want me to disable unused_must_use?

Do you want me to disable `unused_must_use`?
nikstur (Migrated from github.com) reviewed 2023-05-02 22:15:40 +00:00
rust/stub/src/uefi_helpers.rs
nikstur (Migrated from github.com) commented 2023-05-02 22:15:40 +00:00
Copy link

Ah because it returns a Result. My bad. You could use .ok() and then you dont need to assign the resulting Option to anything. But I don't have a strong preference in that case.

Ah because it returns a Result. My bad. You could use `.ok()` and then you dont need to assign the resulting `Option` to anything. But I don't have a strong preference in that case.
RaitoBezarius (Migrated from github.com) reviewed 2023-05-02 22:29:36 +00:00
rust/stub/src/uefi_helpers.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-02 22:29:36 +00:00
Copy link

Good point, will do the ok

Good point, will do the ok
RaitoBezarius commented 2023-05-03 15:32:17 +00:00 (Migrated from github.com)
Copy link

There's a problem in this PR. I am writing UTF8 strings when bootctl is expecting UTF16 strings I believe.
I will fix this.

There's a problem in this PR. I am writing UTF8 strings when bootctl is expecting UTF16 strings I believe. I will fix this.
RaitoBezarius commented 2023-05-04 21:35:45 +00:00 (Migrated from github.com)
Copy link

This is now fixed, you can see the correct parameters in the VM test on the CI (hopefully).

This is now fixed, you can see the correct parameters in the VM test on the CI (hopefully).
RaitoBezarius commented 2023-05-05 15:38:26 +00:00 (Migrated from github.com)
Copy link

@nikstur can I have a second approval after my latest changes on string handling?

@nikstur can I have a second approval after my latest changes on string handling?
nikstur (Migrated from github.com) requested changes 2023-05-05 16:32:05 +00:00
nikstur (Migrated from github.com) left a comment
Copy link

Looks good but the part_discovery.rs module is now superfluous, right?

Looks good but the `part_discovery.rs` module is now superfluous, right?
rust/stub/src/part_discovery.rs
nikstur (Migrated from github.com) commented 2023-05-05 16:29:24 +00:00
Copy link

The code form this module is also in efivars.rs now. So it can be deleted now, right?

The code form this module is also in `efivars.rs` now. So it can be deleted now, right?
RaitoBezarius (Migrated from github.com) reviewed 2023-05-05 18:12:01 +00:00
rust/stub/src/part_discovery.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-05 18:12:01 +00:00
Copy link

Right, forgot it

Right, forgot it
RaitoBezarius commented 2023-05-05 18:12:09 +00:00 (Migrated from github.com)
Copy link

Fixed

Fixed
🚀 1
nikstur (Migrated from github.com) approved these changes 2023-05-05 19:32:32 +00:00
SuperSandro2000 commented 2023-05-14 22:10:00 +00:00 (Migrated from github.com)
Copy link

Since this PR I am receiving the following error which I don't really know what to do with:

unpacking sources
unpacking source archive /nix/store/3ixzizw6kp8vnkpyhadqc5pych5apv17-source
source root is source
patching sources
Executing configureCargoCommonVars
copying cargo artifacts from /nix/store/m25qkvzzm2ibfbncx0l834j5b0a1hykw-lanzaboote_stub-deps-0.1.0/target to target
configuring
will append /build/source/.cargo-home/config.toml with contents of /nix/store/f6mxb59yd8rscs7jyc5bbs14sryjgj9m-vendor-cargo-deps/config.toml
default configurePhase, nothing to do
building
++ command cargo --version
cargo 1.68.2 (6feb7c9cf 2023-03-26)
++ command cargo build --release --message-format json-render-diagnostics
   Compiling lanzaboote_stub v0.1.0 (/build/source)
error[E0786]: found invalid metadata files for crate `scroll_derive` which `goblin` depends on
 --> src/pe_loader.rs:4:5
  |
4 | use goblin::pe::PE;
  |     ^^^^^^
  |
  = note: no `.rustc` section in '/build/source/target/release/deps/libscroll_derive-3d574779bc3d9074.so'

error[E0786]: found invalid metadata files for crate `scroll_derive` which `goblin` depends on
  --> src/pe_section.rs:11:21
   |
11 |     let pe_binary = goblin::pe::PE::parse(pe_data).ok()?;
   |                     ^^^^^^
   |
   = note: no `.rustc` section in '/build/source/target/release/deps/libscroll_derive-3d574779bc3d9074.so'

For more information about this error, try `rustc --explain E0786`.
error: could not compile `lanzaboote_stub` due to 2 previous errors
Since this PR I am receiving the following error which I don't really know what to do with: ``` unpacking sources unpacking source archive /nix/store/3ixzizw6kp8vnkpyhadqc5pych5apv17-source source root is source patching sources Executing configureCargoCommonVars copying cargo artifacts from /nix/store/m25qkvzzm2ibfbncx0l834j5b0a1hykw-lanzaboote_stub-deps-0.1.0/target to target configuring will append /build/source/.cargo-home/config.toml with contents of /nix/store/f6mxb59yd8rscs7jyc5bbs14sryjgj9m-vendor-cargo-deps/config.toml default configurePhase, nothing to do building ++ command cargo --version cargo 1.68.2 (6feb7c9cf 2023-03-26) ++ command cargo build --release --message-format json-render-diagnostics Compiling lanzaboote_stub v0.1.0 (/build/source) error[E0786]: found invalid metadata files for crate `scroll_derive` which `goblin` depends on --> src/pe_loader.rs:4:5 | 4 | use goblin::pe::PE; | ^^^^^^ | = note: no `.rustc` section in '/build/source/target/release/deps/libscroll_derive-3d574779bc3d9074.so' error[E0786]: found invalid metadata files for crate `scroll_derive` which `goblin` depends on --> src/pe_section.rs:11:21 | 11 | let pe_binary = goblin::pe::PE::parse(pe_data).ok()?; | ^^^^^^ | = note: no `.rustc` section in '/build/source/target/release/deps/libscroll_derive-3d574779bc3d9074.so' For more information about this error, try `rustc --explain E0786`. error: could not compile `lanzaboote_stub` due to 2 previous errors ```
RaitoBezarius commented 2023-05-14 22:17:29 +00:00 (Migrated from github.com)
Copy link

is this yet another flake follows issue?

is this yet another flake `follows` issue?
RaitoBezarius commented 2023-05-14 22:17:35 +00:00 (Migrated from github.com)
Copy link

I cannot reproduce it without flakes.

I cannot reproduce it without flakes.
SuperSandro2000 commented 2023-05-14 22:40:26 +00:00 (Migrated from github.com)
Copy link

I removed all follows but nix just defaulted to use the pinned version, so apparently you must do a flake update after that.

I removed all follows but nix just defaulted to use the pinned version, so apparently you must do a flake update after that.
RaitoBezarius commented 2023-05-14 22:45:51 +00:00 (Migrated from github.com)
Copy link

#147 cannot come soon enough.

#147 cannot come soon enough.
Sign in to join this conversation.
Reviewers
No reviewers
blitz
nikstur
Labels
Clear labels   
bug

Something isn't working

  
dependency

Issues related to our dependencies

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
review-next

   
security

   
stub

   
tool

   
wontfix

This will not be worked on

No labels
bug
dependency
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
review-next
security
stub
tool
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: raito/lanzaboote#166
No description provided.
Delete branch "sd-stub-efi-variables"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?