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

stub: load companions #306

Merged
RaitoBezarius merged 11 commits from stub-loads-companions into master 2024-07-20 18:27:51 +00:00
Conversation 20 Commits 11 Files changed 10 +451 -27
RaitoBezarius commented 2024-02-11 16:04:44 +00:00 (Migrated from github.com)
Copy link

This makes use of the pio library to discover, charge, measures companions.
Depends on the dynamic initrds PR: https://github.com/nix-community/lanzaboote/pull/305.

This makes use of the pio library to discover, charge, measures companions. Depends on the dynamic initrds PR: https://github.com/nix-community/lanzaboote/pull/305.
RaitoBezarius commented 2024-03-01 15:47:26 +00:00 (Migrated from github.com)
Copy link

I want to cry, I force pushed this thing, but it seems to have no effect.

I want to cry, I force pushed this thing, but it seems to have no effect.
nikstur (Migrated from github.com) approved these changes 2024-03-01 20:33:33 +00:00
nikstur (Migrated from github.com) left a comment
Copy link

Generally looks fine, didn't look into it in too in depth because it's new functionality. I'll defer the final review to @blitz.

Generally looks fine, didn't look into it in too in depth because it's new functionality. I'll defer the final review to @blitz.
rust/uefi/stub/src/thin.rs
nikstur (Migrated from github.com) commented 2024-03-01 20:28:44 +00:00
Copy link

This needs some more context, for example why is it called pad4?

This needs some more context, for example why is it called `pad4`?
blitz (Migrated from github.com) reviewed 2024-03-01 23:43:53 +00:00
blitz (Migrated from github.com) left a comment
Copy link

The code has lots of panics. Is it possible to have warnings instead and return Err?

Feel free to ignore the code suggestions, if they don't make sense to you or result in too much refactoring.

The code has lots of panics. Is it possible to have warnings instead and return `Err`? Feel free to ignore the code suggestions, if they don't make sense to you or result in too much refactoring.
rust/uefi/Cargo.toml
blitz (Migrated from github.com) commented 2024-03-01 16:33:26 +00:00
Copy link

nit: Can you leave a comment when this can go away?

nit: Can you leave a comment when this can go away?
rust/uefi/linux-bootloader/src/companions.rs
blitz (Migrated from github.com) commented 2024-03-01 16:38:28 +00:00
Copy link

Clippy should complain about this (superfluous return). Isn't it?

Clippy should complain about this (superfluous return). Isn't it?
blitz (Migrated from github.com) commented 2024-03-01 16:40:02 +00:00
Copy link

Why do we panic here? I don't understand the comment.

Why do we panic here? I don't understand the comment.
blitz (Migrated from github.com) commented 2024-03-01 16:42:09 +00:00
Copy link

metadata.is_directory().then_some(|| PathBuf::from(target_directory)

`metadata.is_directory().then_some(|| PathBuf::from(target_directory)`
blitz (Migrated from github.com) commented 2024-03-01 23:06:18 +00:00
Copy link

Should we convert the CPIO errors into uefi errors and return instead of panicking? Same below.

Should we convert the CPIO errors into uefi errors and return instead of panicking? Same below.
blitz (Migrated from github.com) commented 2024-03-01 23:08:45 +00:00
Copy link

It's weird to have an enum that has exactly the same payload for all variants. This also leads you to implement the awkward into_cpio function below. Maybe:

pub enum CompanionInitrdType { ... }

pub struct CompanionInitrd {
  type: CompanionInitrdType,
  cpio: Cpio
}
It's weird to have an enum that has exactly the same payload for all variants. This also leads you to implement the awkward `into_cpio` function below. Maybe: ```rust pub enum CompanionInitrdType { ... } pub struct CompanionInitrd { type: CompanionInitrdType, cpio: Cpio } ```
blitz (Migrated from github.com) commented 2024-03-01 23:18:22 +00:00
Copy link

Maybe better:

/// Collect all credentials and return them as CPIO archive.
///
/// There are two variants of credentials:
///   - global: `$ESP/loader.credentials/*.cred`
///   - image-specific: `$path_to_image.extra/*.cred` 
///
/// The credentials are not measured.
Maybe better: ``` /// Collect all credentials and return them as CPIO archive. /// /// There are two variants of credentials: /// - global: `$ESP/loader.credentials/*.cred` /// - image-specific: `$path_to_image.extra/*.cred` /// /// The credentials are not measured. ```
blitz (Migrated from github.com) commented 2024-03-01 23:21:39 +00:00
Copy link

We should not panic here, but propagate the error.

We should not panic here, but propagate the error.
blitz (Migrated from github.com) commented 2024-03-01 23:36:50 +00:00
Copy link

This function and the calling code might become simpler if you split this function into discover_global_credentials and discover_local_credentials.

This function and the calling code might become simpler if you split this function into `discover_global_credentials` and `discover_local_credentials`.
rust/uefi/linux-bootloader/src/companions.rs
@ -0,0 +20,4 @@
) -> uefi::Result<Vec<PathBuf>> {
let mut results = Vec::new();
for maybe_entry in fs.read_dir(search_path).unwrap() {
blitz (Migrated from github.com) commented 2024-03-01 16:37:02 +00:00
Copy link

Shouldn't this propagate the error instead of panicking?

nit: This would be easier to read as read_dir().iter().filter().map().collect()

Shouldn't this propagate the error instead of panicking? nit: This would be easier to read as `read_dir().iter().filter().map().collect()`
rust/uefi/linux-bootloader/src/companions.rs
@ -0,0 +137,4 @@
/// Discover any system image extension, i.e. files ending by .raw
/// They must be present inside $path_to_image.extra/*.raw, specific to this image.
///
/// Those will be unmeasured, you are responsible for measuring them or not.
blitz (Migrated from github.com) commented 2024-03-01 23:14:35 +00:00
Copy link

"The extensions are not measured."

"The extensions are not measured."
rust/uefi/linux-bootloader/src/companions.rs
@ -0,0 +141,4 @@
/// But CPIOs are guaranteed to be stable and independent of file discovery order.
pub fn discover_system_extensions(
fs: &mut uefi::fs::FileSystem,
default_dropin_dir: &Path,
blitz (Migrated from github.com) commented 2024-03-01 23:15:31 +00:00
Copy link

Why is this parameter optional above and not here?

Why is this parameter optional above and not here?
rust/uefi/linux-bootloader/src/measure.rs
blitz (Migrated from github.com) commented 2024-03-01 23:24:52 +00:00
Copy link

"The order of companions matters for the measurement. The caller is responsible for a stable order."

I would suggest making a pass through the code and removing all mentions of "you" from the docstrings.

"The order of companions matters for the measurement. The caller is responsible for a stable order." I would suggest making a pass through the code and removing all mentions of "you" from the docstrings.
rust/uefi/linux-bootloader/src/measure.rs
@ -71,0 +112,4 @@
credentials_measured += 1;
}
}
CompanionInitrdType::GlobalCredentials => {
blitz (Migrated from github.com) commented 2024-03-01 23:28:09 +00:00
Copy link

nit: If you filter the irrelevant initds from the vector before iterating, you can simplify this a bit. measurements doesn't have to be imperatively computed, but is then just relevant_initrds.len(). Might not be worth it, but maybe there is a way to make this function less C-like and remove the mutable state for easier reading.

nit: If you filter the irrelevant initds from the vector before iterating, you can simplify this a bit. `measurements` doesn't have to be imperatively computed, but is then just `relevant_initrds.len()`. Might not be worth it, but maybe there is a way to make this function less C-like and remove the mutable state for easier reading.
rust/uefi/stub/src/main.rs
@ -69,2 +71,3 @@
}
export_efi_variables(STUB_NAME, &system_table).expect("Failed to export stub EFI variables");
if export_efi_variables(STUB_NAME, &system_table).is_err() {
blitz (Migrated from github.com) commented 2024-03-01 23:34:24 +00:00
Copy link

Could maybe be simmplified to:

let default_dropin_directory =  pe_in_memory.file_path().map(...).transpose()?;

This would also remove the panic.

Could maybe be simmplified to: ```rust let default_dropin_directory = pe_in_memory.file_path().map(...).transpose()?; ``` This would also remove the panic.
blitz (Migrated from github.com) commented 2024-03-01 23:37:47 +00:00
Copy link

This function also panics a lot. Is it necessary and can't we propagate the errors instead?

This function also panics a lot. Is it necessary and can't we propagate the errors instead?
rust/uefi/stub/src/thin.rs
blitz (Migrated from github.com) commented 2024-03-01 23:39:01 +00:00
Copy link

And it could just return Vec<u8>, because this can already represent not having to add padding by returning an empty vector and thus simplifying calling code.

And it could just return `Vec<u8>`, because this can already represent not having to add padding by returning an empty vector and thus simplifying calling code.
blitz (Migrated from github.com) commented 2024-03-01 23:41:47 +00:00
Copy link

vec![0u8; (4 - (len % 4)) % 4] ?

`vec![0u8; (4 - (len % 4)) % 4]` ?
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:16:11 +00:00
rust/uefi/linux-bootloader/src/companions.rs
@ -0,0 +20,4 @@
) -> uefi::Result<Vec<PathBuf>> {
let mut results = Vec::new();
for maybe_entry in fs.read_dir(search_path).unwrap() {
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:16:11 +00:00
Copy link

So re-encoding the FileSystemError into uefi::Result is quite difficult because the enum is Io, Path or Utf8Encoding and it's probably going to be very unhelpful to eat the low level errors into high level errors that will produce difficult to debug code IMHO. A better solution would be to have ways to propagate the original error up but there's impedence mismatch with our error management here.

So re-encoding the FileSystemError into `uefi::Result` is quite difficult because the enum is Io, Path or Utf8Encoding and it's probably going to be very unhelpful to eat the low level errors into high level errors that will produce difficult to debug code IMHO. A better solution would be to have ways to propagate the original error up but there's impedence mismatch with our error management here.
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:16:49 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:16:49 +00:00
Copy link

Nope

Nope
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:17:24 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:17:24 +00:00
Copy link

But addressed

But addressed
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:20:30 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:20:30 +00:00
Copy link

The comment is outdated, please ignore.
There's four sources of panic:

  • Out of memory
  • Cannot open the protocol
  • No handle supporting the device path to text
  • Cannot locate a handle buffer with handles associated to the device path to text protocol

So those are low level errors that have no chance for recovery in the current way our code is structured, it seems better to me that our panic handler catch them and report them fully rather than silencing them.

The comment is outdated, please ignore. There's four sources of panic: - Out of memory - Cannot open the protocol - No handle supporting the device path to text - Cannot locate a handle buffer with handles associated to the device path to text protocol So those are low level errors that have no chance for recovery in the current way our code is structured, it seems better to me that our panic handler catch them and report them fully rather than silencing them.
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:28:53 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:28:53 +00:00
Copy link

So interestingly, this doesn't work because:

56  |       Ok(fs
    |  _____--_^
    | |     |
    | |     arguments to this enum variant are incorrect
57  | |         .metadata(target_directory.as_ref())
58  | |         .ok()
59  | |         .and_then(|metadata| {
...   |
62  | |                 .then_some(|| PathBuf::from(target_directory))
63  | |         }))
    | |__________^ expected `Option<PathBuf>`, found `Option<{closure@companions.rs:62:28}>`
    |

So I don't know if I'm fucking up somewhere but I'm not so sure if there is not a closure capturing problem.

So interestingly, this doesn't work because: ``` 56 | Ok(fs | _____--_^ | | | | | arguments to this enum variant are incorrect 57 | | .metadata(target_directory.as_ref()) 58 | | .ok() 59 | | .and_then(|metadata| { ... | 62 | | .then_some(|| PathBuf::from(target_directory)) 63 | | })) | |__________^ expected `Option<PathBuf>`, found `Option<{closure@companions.rs:62:28}>` | ``` So I don't know if I'm fucking up somewhere but I'm not so sure if there is not a closure capturing problem.
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:41:47 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:41:47 +00:00
Copy link

Yep, addressed.

Yep, addressed.
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:42:05 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:42:05 +00:00
Copy link

I can return an LOAD_ERROR but not very sure if that's a good idea for debuggability given that our logging facilities are quite bad.

I can return an LOAD_ERROR but not very sure if that's a good idea for debuggability given that our logging facilities are quite bad.
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:47:48 +00:00
rust/uefi/linux-bootloader/src/measure.rs
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:47:48 +00:00
Copy link

I kind of want to make a remark that you appears in your docstrings for the initrd loader ;) -- but sure, done.

I kind of want to make a remark that `you` appears in your docstrings for the initrd loader ;) -- but sure, done.
RaitoBezarius (Migrated from github.com) reviewed 2024-03-15 18:53:40 +00:00
rust/uefi/linux-bootloader/src/companions.rs
@ -0,0 +141,4 @@
/// But CPIOs are guaranteed to be stable and independent of file discovery order.
pub fn discover_system_extensions(
fs: &mut uefi::fs::FileSystem,
default_dropin_dir: &Path,
RaitoBezarius (Migrated from github.com) commented 2024-03-15 18:53:40 +00:00
Copy link

If there's no dropin directory for system extensions, there's no attempt to load them.
If there's no dropin directory for systemd credentials, we can still try to load them from a default dropin directory location.

We can make both of them take &Path and put the default inside of the caller site if we want but that's weird because the logic comes from the function itself.

If there's no dropin directory for system extensions, there's no attempt to load them. If there's no dropin directory for systemd credentials, we can still try to load them from a default dropin directory location. We can make both of them take &Path and put the default inside of the caller site if we want but that's weird because the logic comes from the function itself.
RaitoBezarius (Migrated from github.com) reviewed 2024-06-10 18:01:07 +00:00
rust/uefi/stub/src/main.rs
@ -69,2 +71,3 @@
}
export_efi_variables(STUB_NAME, &system_table).expect("Failed to export stub EFI variables");
if export_efi_variables(STUB_NAME, &system_table).is_err() {
RaitoBezarius (Migrated from github.com) commented 2024-06-10 18:01:07 +00:00
Copy link

Fixed.

Fixed.
RaitoBezarius (Migrated from github.com) reviewed 2024-06-10 18:01:16 +00:00
rust/uefi/stub/src/main.rs
@ -69,2 +71,3 @@
}
export_efi_variables(STUB_NAME, &system_table).expect("Failed to export stub EFI variables");
if export_efi_variables(STUB_NAME, &system_table).is_err() {
RaitoBezarius (Migrated from github.com) commented 2024-06-10 18:01:16 +00:00
Copy link

Fixed.

Fixed.
RaitoBezarius (Migrated from github.com) reviewed 2024-06-10 18:03:02 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-06-10 18:03:02 +00:00
Copy link

Done.

Done.
RaitoBezarius (Migrated from github.com) reviewed 2024-06-10 18:03:24 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-06-10 18:03:24 +00:00
Copy link

Fixed, it was then, not then_some.

Fixed, it was `then`, not `then_some`.
RaitoBezarius (Migrated from github.com) reviewed 2024-06-10 18:03:32 +00:00
rust/uefi/linux-bootloader/src/companions.rs
RaitoBezarius (Migrated from github.com) commented 2024-06-10 18:03:32 +00:00
Copy link

Fixed.

Fixed.
RaitoBezarius (Migrated from github.com) reviewed 2024-06-10 18:03:43 +00:00
rust/uefi/Cargo.toml
RaitoBezarius (Migrated from github.com) commented 2024-06-10 18:03:43 +00:00
Copy link

Fixed.

Fixed.
RaitoBezarius (Migrated from github.com) reviewed 2024-06-10 18:14:51 +00:00
rust/uefi/stub/src/thin.rs
RaitoBezarius (Migrated from github.com) commented 2024-06-10 18:14:50 +00:00
Copy link

Done.

Done.
RaitoBezarius (Migrated from github.com) reviewed 2024-06-10 18:15:11 +00:00
rust/uefi/stub/src/thin.rs
RaitoBezarius (Migrated from github.com) commented 2024-06-10 18:15:11 +00:00
Copy link

Done for returning just a Vec.

Done for returning just a Vec<u8>.
Sign in to join this conversation.
Reviewers
No reviewers
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#306
No description provided.
Delete branch "stub-loads-companions"

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?