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

install: Fail early if image sector size doesn't match destination #167

Merged
merged 4 commits into from
Feb 25, 2020
Merged
Show file tree
Hide file tree
Changes from 3 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
89 changes: 67 additions & 22 deletions src/blockdev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@

use error_chain::bail;
use nix::errno::Errno;
use nix::{self, ioctl_none, ioctl_write_ptr_bad, mount, request_code_none};
use nix::{self, ioctl_none, ioctl_read_bad, ioctl_write_ptr_bad, mount, request_code_none};
use regex::Regex;
use std::collections::HashMap;
use std::convert::TryInto;
use std::fs::{remove_dir, File};
use std::io::{Seek, SeekFrom};
use std::num::NonZeroU32;
use std::os::raw::c_int;
use std::os::unix::io::AsRawFd;
use std::path::{Path, PathBuf};
use std::process::Command;
Expand All @@ -29,20 +32,38 @@ use tempdir::TempDir;
use crate::errors::*;

pub fn mount_boot(device: &str) -> Result<Mount> {
let dev = get_partition_with_label(device, "boot")?
.chain_err(|| format!("couldn't find boot device for {}", device))?;
match dev.fstype {
// get partition list
let partitions = get_partitions(device)?;
if partitions.is_empty() {
bail!("couldn't find any partitions on {}", device);
}

// find the boot partition
let boot_partitions = partitions
.iter()
.filter(|d| d.label.as_ref().unwrap_or(&"".to_string()) == "boot")
.collect::<Vec<&BlkDev>>();
let dev = match boot_partitions.len() {
0 => bail!("couldn't find boot device for {}", device),
1 => boot_partitions[0],
_ => bail!("found multiple devices on {} with label \"boot\"", device),
};

// mount it
match &dev.fstype {
Some(fstype) => Mount::try_mount(&dev.path, &fstype),
None => Err(format!("couldn't get filesystem type of boot device for {}", device).into()),
}
}

#[derive(Debug)]
struct BlkDev {
path: String,
label: Option<String>,
fstype: Option<String>,
}

fn get_partition_with_label(device: &str, label: &str) -> Result<Option<BlkDev>> {
fn get_partitions(device: &str) -> Result<Vec<BlkDev>> {
// have lsblk enumerate partitions on the device
let result = Command::new("lsblk")
.arg("--pairs")
Expand All @@ -59,21 +80,11 @@ fn get_partition_with_label(device: &str, label: &str) -> Result<Option<BlkDev>>
let output = String::from_utf8(result.stdout).chain_err(|| "decoding lsblk output")?;

// walk each device in the output
let mut found: Option<BlkDev> = None;
let mut result: Vec<BlkDev> = Vec::new();
for line in output.lines() {
// parse key-value pairs
let fields = split_lsblk_line(line);

// does the label match?
match fields.get("LABEL") {
None => continue,
Some(l) => {
if l != label {
continue;
}
}
}

// Older lsblk, e.g. in CentOS 7.6, doesn't support PATH.
// Assemble device path from dirname and NAME.
let mut path = Path::new(device)
Expand All @@ -84,17 +95,18 @@ fn get_partition_with_label(device: &str, label: &str) -> Result<Option<BlkDev>>
None => continue,
Some(name) => path.push(name),
}

// accept
if found.is_some() {
bail!("found multiple devices on {} with label: {}", device, label);
// Skip the device itself
if path == Path::new(device) {
continue;
}
found = Some(BlkDev {

result.push(BlkDev {
path: path.to_str().expect("couldn't round-trip path").to_string(),
label: fields.get("LABEL").map(|v| v.to_string()),
fstype: fields.get("FSTYPE").map(|v| v.to_string()),
});
}
Ok(found)
Ok(result)
}

/// Parse key-value pairs from lsblk --pairs.
Expand Down Expand Up @@ -195,6 +207,21 @@ pub fn reread_partition_table(file: &mut File) -> Result<()> {
Ok(())
}

/// Get the logical sector size of a block device.
pub fn get_sector_size(file: &mut File) -> Result<NonZeroU32> {
let fd = file.as_raw_fd();
let mut size: c_int = 0;
match unsafe { blksszget(fd, &mut size) } {
Ok(_) => {
let size_u32: u32 = size
.try_into()
.chain_err(|| format!("sector size {} doesn't fit in u32", size))?;
NonZeroU32::new(size_u32).ok_or_else(|| "found sector size of zero".into())
}
Err(e) => Err(Error::with_chain(e, "getting sector size")),
}
}

/// Try discarding all blocks from the underlying block device.
/// Return true if successful, false if the underlying device doesn't
/// support discard, or an error otherwise.
Expand Down Expand Up @@ -223,6 +250,7 @@ pub fn try_discard_all(file: &mut File) -> Result<bool> {

// create unsafe ioctl wrappers
ioctl_none!(blkrrpart, 0x12, 95);
ioctl_read_bad!(blksszget, request_code_none!(0x12, 104), c_int);
ioctl_write_ptr_bad!(blkdiscard, request_code_none!(0x12, 119), [u64; 2]);

pub fn udev_settle() -> Result<()> {
Expand Down Expand Up @@ -250,6 +278,23 @@ pub fn udev_settle() -> Result<()> {
Ok(())
}

/// Inspect a buffer from the start of a disk image and return its formatted
/// sector size, if any can be determined.
pub fn detect_formatted_sector_size(buf: &[u8]) -> Option<NonZeroU32> {
let gpt_magic: &[u8; 8] = b"EFI PART";

if buf.len() >= 520 && buf[512..520] == gpt_magic[..] {
// GPT at offset 512
NonZeroU32::new(512)
} else if buf.len() >= 4104 && buf[4096..4104] == gpt_magic[..] {
// GPT at offset 4096
NonZeroU32::new(4096)
} else {
// Unknown
None
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
39 changes: 32 additions & 7 deletions src/download.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@ use nix::unistd::isatty;
use progress_streams::ProgressReader;
use std::fs::{remove_file, File, OpenOptions};
use std::io::{copy, stderr, BufRead, BufReader, Read, Seek, SeekFrom, Write};
use std::num::NonZeroU32;
use std::os::unix::io::AsRawFd;
use std::path::{Path, PathBuf};
use std::time::{Duration, Instant};
use xz2::read::XzDecoder;

use crate::blockdev::detect_formatted_sector_size;
use crate::cmdline::*;
use crate::errors::*;
use crate::source::*;
Expand Down Expand Up @@ -141,7 +143,8 @@ fn write_image_and_sig(
.chain_err(|| format!("opening {}", path.display()))?;

// download and verify image
write_image(source, &mut dest, decompress)?;
// don't check sector size
write_image(source, &mut dest, decompress, None)?;

// write signature, if relevant
if !decompress && source.signature.is_some() {
Expand All @@ -160,7 +163,12 @@ fn write_image_and_sig(
}

/// Copy the image to disk and verify its signature.
pub fn write_image(source: &mut ImageSource, dest: &mut File, decompress: bool) -> Result<()> {
pub fn write_image(
source: &mut ImageSource,
dest: &mut File,
decompress: bool,
expected_sector_size: Option<NonZeroU32>,
) -> Result<()> {
// wrap source for GPG verification
let mut verify_reader: Box<dyn Read> = {
if let Some(signature) = source.signature.as_ref() {
Expand Down Expand Up @@ -239,15 +247,32 @@ pub fn write_image(source: &mut ImageSource, dest: &mut File, decompress: bool)
}
};

// Cache the first MiB of input and write zeroes instead. This ensures
// that the disk image can't be used accidentally before its GPG signature
// is verified.
// Read the first MiB of input and, if requested, check it against the
// image's formatted sector size.
let mut first_mb: [u8; 1024 * 1024] = [0; 1024 * 1024];
dest.write_all(&first_mb)
.chain_err(|| "clearing first MiB of disk")?;
decompress_reader
.read_exact(&mut first_mb)
.chain_err(|| "decoding first MiB of image")?;
// Were we asked to check sector size?
if let Some(expected) = expected_sector_size {
// Can we derive one from source data?
if let Some(actual) = detect_formatted_sector_size(&first_mb) {
Copy link
Member

Choose a reason for hiding this comment

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

If we're asked to expect a sector size, shouldn't we fail if we can't query the sector size from the image?

Copy link
Contributor Author

@bgilbert bgilbert Feb 25, 2020

Choose a reason for hiding this comment

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

It's a best-effort check to improve UX. We don't currently detect sector size on non-EFI CPU architectures.

// Do they match?
if expected != actual {
bail!(
"source has sector size {} but destination has sector size {}",
actual.get(),
expected.get()
);
}
}
}

// Cache the first MiB and write zeroes to dest instead. This ensures
// that the disk image can't be used accidentally before its GPG signature
// is verified.
dest.write_all(&first_mb)
.chain_err(|| "clearing first MiB of disk")?;

// do the rest of the copy
// This physically writes any runs of zeroes, rather than sparsifying,
Expand Down
5 changes: 4 additions & 1 deletion src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,11 @@ fn write_disk(
// leveler or LVM thin pool.
try_discard_all(dest)?;

// Get sector size of destination, for comparing with image
let sector_size = get_sector_size(dest)?;

// copy the image
write_image(source, dest, true)?;
write_image(source, dest, true, Some(sector_size))?;
reread_partition_table(dest)?;
udev_settle()?;

Expand Down