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

Don't adopt systemd boot loaders #462

Merged
merged 2 commits into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ openssl = "^0.10"
serde = { version = "^1.0", features = ["derive"] }
serde_json = "^1.0"
tempfile = "^3.4"
widestring = "1.0.2"

[profile.release]
# We assume we're being delivered via e.g. RPM which supports split debuginfo
Expand Down
76 changes: 75 additions & 1 deletion src/efi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use std::process::Command;

use anyhow::{bail, Context, Result};
use openat_ext::OpenatDirExt;
use widestring::U16CString;

use crate::component::*;
use crate::filetree;
Expand All @@ -21,12 +22,16 @@ use crate::util;
use crate::util::CommandRunExt;

/// Well-known paths to the ESP that may have been mounted external to us.
pub(crate) const ESP_MOUNTS: &[&str] = &["boot/efi", "efi"];
pub(crate) const ESP_MOUNTS: &[&str] = &["boot", "boot/efi", "efi"];

/// The ESP partition label on Fedora CoreOS derivatives
pub(crate) const COREOS_ESP_PART_LABEL: &str = "EFI-SYSTEM";
pub(crate) const ANACONDA_ESP_PART_LABEL: &str = "EFI\\x20System\\x20Partition";

/// Systemd boot loader info EFI variable names
const LOADER_INFO_VAR_STR: &str = "LoaderInfo-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f";
const STUB_INFO_VAR_STR: &str = "StubInfo-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f";

#[derive(Default)]
pub(crate) struct Efi {
mountpoint: RefCell<Option<PathBuf>>,
Expand Down Expand Up @@ -105,6 +110,70 @@ impl Efi {
}
}

/// Convert a nul-terminated UTF-16 byte array to a String.
fn string_from_utf16_bytes(slice: &[u8]) -> String {
// For some reason, systemd appends 3 nul bytes after the string.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, messy. But I wonder if we should care about this? Maybe instead we return a Vec<u8> since in the end all we care about is if it starts with the ASCII string systemd right?

OK as is though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't just send an array of bytes (which you already have from reading the file contents) back since they're be misinterpreted as UTF-8 since every other byte is nul. That's why you have to convert it to Vec<u16> and parse it as UTF-16. I don't believe there's any spec that says you have to write UTF-16 strings to EFI variables. I think that's just a convention systemd follows since that's how other vendors do it. I think I read a comment in the systemd code that they append 3 nul bytes so that in case they start parsing in the middle of the buffer they'll still end up with a valid C string. However, stripping an odd numbered trailing byte seems like good practice.

When I wrote roughly the same code in Python where I didn't have to change types, I walked backwards from the end of the buffer until I found no more pairs of nul bytes. Here I let U16CString handle that part.

Copy link
Member

@cgwalters cgwalters May 8, 2023

Choose a reason for hiding this comment

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

Yes...hm. What about going the other way, and converting from systemd in UTF-8 to UTF-16, and then using https://doc.rust-lang.org/std/primitive.slice.html#method.starts_with ?

That way we can also just ignore the trailing NULs.

// Drop the last byte if there's an odd number.
let size = slice.len() / 2;
let v: Vec<u16> = (0..size)
.map(|i| u16::from_ne_bytes([slice[2 * i], slice[2 * i + 1]]))
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that does look nicer. My rust noobness shows through.

Copy link
Member

Choose a reason for hiding this comment

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

Your code is great honestly! Learning a language (programming or not) is always a journey; even the relatively small std in Rust has a lot of surface area and idioms.

.collect();
U16CString::from_vec(v).unwrap().to_string_lossy()
}

/// Read a nul-terminated UTF-16 string from an EFI variable.
fn read_efi_var_utf16_string(name: &str) -> Option<String> {
let efivars = Path::new("/sys/firmware/efi/efivars");
if !efivars.exists() {
log::warn!("No efivars mount at {:?}", efivars);
return None;
}
let path = efivars.join(name);
if !path.exists() {
Copy link
Member

Choose a reason for hiding this comment

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

Optional cleanup in the future: We do have an open_file_optional helper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't notice that.

log::trace!("No EFI variable {name}");
return None;
}
match std::fs::read(&path) {
Ok(buf) => {
// Skip the first 4 bytes, those are the EFI variable attributes.
if buf.len() < 4 {
log::warn!("Read less than 4 bytes from {:?}", path);
return None;
}
Some(string_from_utf16_bytes(&buf[4..]))
}
Err(reason) => {
log::warn!("Failed reading {:?}: {reason}", path);
None
}
}
}

/// Read the LoaderInfo EFI variable if it exists.
fn get_loader_info() -> Option<String> {
read_efi_var_utf16_string(LOADER_INFO_VAR_STR)
}

/// Read the StubInfo EFI variable if it exists.
fn get_stub_info() -> Option<String> {
read_efi_var_utf16_string(STUB_INFO_VAR_STR)
}

/// Whether to skip adoption if a systemd bootloader is found.
fn skip_systemd_bootloaders() -> bool {
if let Some(loader_info) = get_loader_info() {
if loader_info.starts_with("systemd") {
log::trace!("Skipping adoption for {:?}", loader_info);
return true;
}
}
if let Some(stub_info) = get_stub_info() {
log::trace!("Skipping adoption for {:?}", stub_info);
return true;
}
false
}

impl Component for Efi {
fn name(&self) -> &'static str {
"EFI"
Expand All @@ -130,6 +199,11 @@ impl Component for Efi {
} else {
log::trace!("No CoreOS aleph detected");
}
// Don't adopt if the system is booted with systemd-boot or
// systemd-stub since those will be managed with bootctl.
if skip_systemd_bootloaders() {
return Ok(None);
}
let ostree_deploy_dir = Path::new("/ostree/deploy");
if ostree_deploy_dir.exists() {
let btime = ostree_deploy_dir.metadata()?.created()?;
Expand Down