Skip to content

efivars: Parse efivar as UTF-16 LE bytes#1633

Merged
cgwalters merged 1 commit intobootc-dev:mainfrom
Johan-Liebert1:efivars
Sep 22, 2025
Merged

efivars: Parse efivar as UTF-16 LE bytes#1633
cgwalters merged 1 commit intobootc-dev:mainfrom
Johan-Liebert1:efivars

Conversation

@Johan-Liebert1
Copy link
Copy Markdown
Collaborator

No description provided.

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
@bootc-bot bootc-bot bot requested a review from jeckersb September 22, 2025 13:16
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the EFI variable reading logic into a new utility function read_uefi_var. This function correctly parses the variables as UTF-16 LE, which is a great improvement for correctness. The overall change improves code organization and maintainability. I've added a few suggestions to further improve code clarity and adhere to Rust idioms.

Comment on lines +168 to +169
EfiError::SystemNotUEFI => return Ok(Bootloader::Grub),
EfiError::MissingVar => return Ok(Bootloader::Grub),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The match arms for EfiError::SystemNotUEFI and EfiError::MissingVar have the same logic. They can be combined into a single arm using the | operator for conciseness and better readability.

Suggested change
EfiError::SystemNotUEFI => return Ok(Bootloader::Grub),
EfiError::MissingVar => return Ok(Bootloader::Grub),
EfiError::SystemNotUEFI | EfiError::MissingVar => return Ok(Bootloader::Grub),
Comment on lines +196 to +199
#[allow(dead_code)]
InvalidData(&'static str),
#[allow(dead_code)]
Io(std::io::Error),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The #[allow(dead_code)] attributes on the InvalidData and Io variants of EfiError are unnecessary. The InvalidData variant is used in read_uefi_var, and the Io variant is used via the From<std::io::Error> implementation and the ? operator. Removing these attributes will make the code cleaner and avoid suppressing potential future warnings about genuinely unused code.

Suggested change
#[allow(dead_code)]
InvalidData(&'static str),
#[allow(dead_code)]
Io(std::io::Error),
InvalidData(&'static str),
Io(std::io::Error),
Comment on lines +210 to +246
pub fn read_uefi_var(var_name: &str) -> Result<String, EfiError> {
use crate::install::EFIVARFS;
use cap_std_ext::cap_std::ambient_authority;

let efivarfs = match Dir::open_ambient_dir(EFIVARFS, ambient_authority()) {
Ok(dir) => dir,
Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Err(EfiError::SystemNotUEFI),
Err(e) => Err(e)?,
};

match efivarfs.read(var_name) {
Ok(loader_bytes) => {
if loader_bytes.len() % 2 != 0 {
return Err(EfiError::InvalidData(
"EFI var length is not valid UTF-16 LE",
));
}

// EFI vars are UTF-16 LE
let loader_u16_bytes: Vec<u16> = loader_bytes
.chunks_exact(2)
.map(|x| u16::from_le_bytes([x[0], x[1]]))
.collect();

let loader = String::from_utf16(&loader_u16_bytes)
.map_err(|_| EfiError::InvalidData("EFI var is not UTF-16"))?;

return Ok(loader);
}

Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
return Err(EfiError::MissingVar);
}

Err(e) => Err(e)?,
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The read_uefi_var function can be refactored to be more idiomatic and readable by using the ? operator for error handling and avoiding multiple match statements with early returns. This improves maintainability.

pub fn read_uefi_var(var_name: &str) -> Result<String, EfiError> {
    use crate::install::EFIVARFS;
    use cap_std_ext::cap_std::ambient_authority;

    let efivarfs = Dir::open_ambient_dir(EFIVARFS, ambient_authority()).map_err(|e| {
        if e.kind() == std::io::ErrorKind::NotFound {
            EfiError::SystemNotUEFI
        } else {
            e.into()
        }
    })?;

    let loader_bytes = efivarfs.read(var_name).map_err(|e| {
        if e.kind() == std::io::ErrorKind::NotFound {
            EfiError::MissingVar
        } else {
            e.into()
        }
    })?;

    if loader_bytes.len() % 2 != 0 {
        return Err(EfiError::InvalidData(
            "EFI var length is not valid UTF-16 LE",
        ));
    }

    // EFI vars are UTF-16 LE
    let loader_u16_bytes: Vec<u16> = loader_bytes
        .chunks_exact(2)
        .map(|x| u16::from_le_bytes([x[0], x[1]]))
        .collect();

    String::from_utf16(&loader_u16_bytes)
        .map_err(|_| EfiError::InvalidData("EFI var is not UTF-16"))
}
return Ok(loader);
}

Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I did coreos/cap-std-ext#78 motivated by this btw

@cgwalters cgwalters merged commit 2898860 into bootc-dev:main Sep 22, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants