From 1d29c3613cf32d09f7fd5ab023ac3318d442074c Mon Sep 17 00:00:00 2001 From: Luciano Dittgen Date: Tue, 20 Oct 2026 14:32:52 -0300 Subject: [PATCH 01/11] fix(raw): only refuse a write overlapping a mounted partition Writing a bootloader to the gap ahead of the first partition of a disk was refused whenever any partition of that disk was mounted, as is always the case for the running rootfs: target device "/dev/mmcblk1" is in use, it holds the filesystem mounted at "/" The raw handler now checks only the region it writes, so partitions of a whole disk lying outside of it are ignored. The disk itself, its holders, overlapping partitions and partitions with unreadable bounds are still checked, as is any other target. raw-delta keeps the exclusive check, as its written length is not known up front. Signed-off-by: Luciano Dittgen --- updatehub/src/object/installer/raw.rs | 70 ++++++++++++++------ updatehub/src/utils/definitions.rs | 13 +++- updatehub/src/utils/fs.rs | 93 ++++++++++++++++++++++++++- 3 files changed, 153 insertions(+), 23 deletions(-) diff --git a/updatehub/src/object/installer/raw.rs b/updatehub/src/object/installer/raw.rs index 6807ea74..18386c1c 100644 --- a/updatehub/src/object/installer/raw.rs +++ b/updatehub/src/object/installer/raw.rs @@ -24,8 +24,11 @@ impl Installer for objects::Raw { async fn check_requirements(&self, _: &Context) -> Result<()> { info!("'raw' handle checking requirements"); - if let definitions::TargetType::Device(dev) = - self.target_type.valid(Access::Exclusive).log_error_msg("device failed vaidation")? + let (offset, len) = target_region(self); + if let definitions::TargetType::Device(dev) = self + .target_type + .valid(Access::Region { offset, len }) + .log_error_msg("device failed vaidation")? { utils::fs::ensure_disk_space(dev, self.required_install_size()) .log_error_msg("not enough disk space")?; @@ -43,24 +46,12 @@ impl Installer for objects::Raw { }; let source = context.download_dir.join(self.sha256sum()); let chunk_size = self.chunk_size.0; - let seek = self.seek * chunk_size as u64; let skip = self.skip.0 * chunk_size as u64; let truncate = self.truncate.0; - let count = match self.count { - definitions::Count::All => None, - definitions::Count::Limited(n) => { - Some(u64::try_from(n).unwrap_or(0) * chunk_size as u64) - } - }; - - // How much of the target this object writes. The check must stop there, - // otherwise a neighbouring object sharing the device answers for it. - let region_len = if self.compressed { - self.required_uncompressed_size - } else { - let written = self.size.saturating_sub(skip); - count.map_or(written, |n| n.min(written)) - }; + let count = count_bytes(self); + // The check must stop at the end of what this object writes, otherwise + // a neighbouring object sharing the device answers for it. + let (seek, region_len) = target_region(self); let should_skip_install = super::should_skip_install( self.install_if_different.as_ref(), @@ -116,6 +107,28 @@ impl Installer for objects::Raw { } } +fn count_bytes(obj: &objects::Raw) -> Option { + match obj.count { + definitions::Count::All => None, + definitions::Count::Limited(n) => { + Some(u64::try_from(n).unwrap_or(0) * obj.chunk_size.0 as u64) + } + } +} + +/// Offset and length, in bytes, of what `obj` writes to its target. +fn target_region(obj: &objects::Raw) -> (u64, u64) { + let chunk_size = obj.chunk_size.0 as u64; + let len = if obj.compressed { + obj.required_uncompressed_size + } else { + let written = obj.size.saturating_sub(obj.skip.0 * chunk_size); + count_bytes(obj).map_or(written, |n| n.min(written)) + }; + + (obj.seek * chunk_size, len) +} + #[cfg(test)] mod tests { use super::*; @@ -288,6 +301,27 @@ mod tests { (obj, download_dir, source, dest) } + #[test] + fn region_covers_what_is_written() -> Result<()> { + let (obj, ..) = + fake_raw_object(8192, 1024, 1, 4, definitions::Count::Limited(2), false, false)?; + assert_eq!(target_region(&obj), (4096, 2048)); + + let (obj, ..) = fake_raw_object(8192, 1024, 1, 4, definitions::Count::All, false, false)?; + assert_eq!(target_region(&obj), (4096, 7168)); + + let (obj, ..) = + fake_raw_object(8192, 1024, 0, 0, definitions::Count::Limited(16), false, false)?; + assert_eq!(target_region(&obj), (0, 8192)); + + let (mut obj, ..) = + fake_raw_object(8192, 1024, 0, 2, definitions::Count::All, false, true)?; + obj.required_uncompressed_size = 8192; + assert_eq!(target_region(&obj), (2048, 8192)); + + Ok(()) + } + /// The check must stop at the end of the object it guards. Objects install /// largest first, so the neighbour is already carrying the new version by /// the time this one is checked, and reading into it skips an object that diff --git a/updatehub/src/utils/definitions.rs b/updatehub/src/utils/definitions.rs index fd7663ab..e1686518 100644 --- a/updatehub/src/utils/definitions.rs +++ b/updatehub/src/utils/definitions.rs @@ -15,6 +15,8 @@ use std::path::PathBuf; pub(crate) enum Access { /// The handler writes the device itself, so nothing else may be using it. Exclusive, + /// The handler writes only `len` bytes at `offset` of the device. + Region { offset: u64, len: u64 }, /// The handler writes through a filesystem it mounts, which is safe to do /// even on a device already mounted elsewhere. ThroughFilesystem, @@ -32,7 +34,8 @@ impl Access { pub(crate) trait TargetTypeExt { /// Checks whether the device is valid to start installation, i.e., /// device exists, use have write permission, and is free of mounted - /// filesystems when the handler asks for [`Access::Exclusive`]. + /// filesystems when the handler asks for [`Access::Exclusive`] or + /// [`Access::Region`]. fn valid(&self, access: Access) -> Result<&Self>; /// Gets device's path for mounting. @@ -51,8 +54,12 @@ impl TargetTypeExt for TargetType { return Err(Error::MissingWritePermission(device)); } - if let Access::Exclusive = access { - fs::ensure_not_mounted(&device)?; + match access { + Access::Exclusive => fs::ensure_not_mounted(&device)?, + Access::Region { offset, len } => { + fs::ensure_region_not_mounted(&device, offset..offset.saturating_add(len))?; + } + Access::ThroughFilesystem => {} } Ok(self) diff --git a/updatehub/src/utils/fs.rs b/updatehub/src/utils/fs.rs index a76daf83..0d8157fd 100644 --- a/updatehub/src/utils/fs.rs +++ b/updatehub/src/utils/fs.rs @@ -12,6 +12,7 @@ use slog_scope::trace; use std::{ collections::HashSet, io::{self, Seek}, + ops::Range, os::unix::fs::FileTypeExt, path::{Path, PathBuf}, }; @@ -74,6 +75,21 @@ type DeviceId = (u64, u64); pub(crate) fn ensure_not_mounted(target: &Path) -> Result<()> { trace!("checking whether {:?} is in use", target); + ensure_devices_not_mounted(target, related_block_devices) +} + +/// Like [`ensure_not_mounted`], but ignores the partitions of a whole disk +/// lying outside `region`. +pub(crate) fn ensure_region_not_mounted(target: &Path, region: Range) -> Result<()> { + trace!("checking whether {:?} is in use within {:?}", target, region); + + ensure_devices_not_mounted(target, |device| region_block_devices(device, ®ion)) +} + +fn ensure_devices_not_mounted( + target: &Path, + related: impl Fn(DeviceId) -> HashSet, +) -> Result<()> { let metadata = match std::fs::metadata(target) { Ok(metadata) => metadata, // Nothing can be mounted on a device which is not there. Complaining @@ -87,10 +103,10 @@ pub(crate) fn ensure_not_mounted(target: &Path) -> Result<()> { let sources = mtd::mount_sources_for(target).into_iter().collect::>(); let mut devices = HashSet::new(); - devices.extend(block_device_id(&metadata).into_iter().flat_map(related_block_devices)); + devices.extend(block_device_id(&metadata).into_iter().flat_map(&related)); for source in sources.iter().filter(|source| source.starts_with('/')) { if let Ok(metadata) = std::fs::metadata(source) { - devices.extend(block_device_id(&metadata).into_iter().flat_map(related_block_devices)); + devices.extend(block_device_id(&metadata).into_iter().flat_map(&related)); } } @@ -180,6 +196,47 @@ fn related_block_devices(device: DeviceId) -> HashSet { found } +/// Like [`related_block_devices`], without the partitions lying outside +/// `region`. +fn region_block_devices(device: DeviceId, region: &Range) -> HashSet { + let sysfs = PathBuf::from(format!("/sys/dev/block/{}:{}", device.0, device.1)); + let Some(sysfs) = sysfs.canonicalize().ok().filter(|sysfs| !sysfs.join("partition").exists()) + else { + return related_block_devices(device); + }; + + let mut found = HashSet::from([device]); + found.extend( + device_ids_in(&sysfs.join("holders"), |_| true).into_iter().flat_map(related_block_devices), + ); + // Unreadable bounds count as overlapping. + found.extend( + device_ids_in(&sysfs, |path| { + path.join("partition").exists() + && partition_range(path).is_none_or(|partition| overlaps(&partition, region)) + }) + .into_iter() + .flat_map(related_block_devices), + ); + + found +} + +/// sysfs reports `start` and `size` in 512 byte sectors. +fn partition_range(path: &Path) -> Option> { + let sectors = |name: &str| -> Option { + std::fs::read_to_string(path.join(name)).ok()?.trim().parse().ok() + }; + let start = sectors("start")?.checked_mul(512)?; + let end = start.checked_add(sectors("size")?.checked_mul(512)?)?; + + Some(start..end) +} + +fn overlaps(a: &Range, b: &Range) -> bool { + a.start.max(b.start) < a.end.min(b.end) +} + /// Device numbers of the sysfs entries under `dir` which satisfy `wanted`. fn device_ids_in(dir: &Path, wanted: impl Fn(&Path) -> bool) -> Vec { let Ok(entries) = std::fs::read_dir(dir) else { @@ -514,6 +571,38 @@ mod tests { assert!(parse_mount_entry("26 25 bogus / /boot rw - ext4 /dev/sda2 rw").is_none()); } + #[test] + fn regions_overlap_only_when_sharing_a_byte() { + let rootfs = 0x10_0000..0x6a50_0000; + assert!(!overlaps(&(0x2_0000..0x3_415c), &rootfs)); + assert!(!overlaps(&(0x6_0000..0xc_2f48), &rootfs)); + + assert!(!overlaps(&(0x8_0000..0x10_0000), &rootfs)); + assert!(overlaps(&(0x8_0000..0x10_0001), &rootfs)); + assert!(overlaps(&(0x20_0000..0x30_0000), &rootfs)); + assert!(overlaps(&(0..u64::MAX), &rootfs)); + assert!(!overlaps(&(0x20_0000..0x20_0000), &rootfs)); + } + + #[test] + fn partition_range_is_read_in_sectors() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("start"), "2048\n").unwrap(); + std::fs::write(dir.path().join("size"), "3481600\n").unwrap(); + + assert_eq!(partition_range(dir.path()), Some(0x10_0000..0x6a50_0000)); + } + + #[test] + fn partition_range_needs_both_bounds() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("start"), "2048\n").unwrap(); + assert_eq!(partition_range(dir.path()), None); + + std::fs::write(dir.path().join("size"), "bogus\n").unwrap(); + assert_eq!(partition_range(dir.path()), None); + } + #[test] fn regular_file_is_not_in_use() { let file = tempfile::NamedTempFile::new().unwrap(); From 43c7a79f32b23300930a1f54093eefec5c8d1676 Mon Sep 17 00:00:00 2001 From: Luciano Dittgen Date: Tue, 20 Oct 2026 15:47:29 -0300 Subject: [PATCH 02/11] fix(raw): never write past the region checked for the object The in-use check covers the region an object declares, from its size or, when compressed, its required-uncompressed-size, but the write itself went on for as long as the source or the decompressor produced data. Metadata understating the data let the write run into a partition the check had left out. The write now stops at the checked region and fails the install when the data goes past it. A compressed object not declaring its uncompressed size has no known region, so it keeps the exclusive check and is not bounded. Signed-off-by: Luciano Dittgen --- updatehub/src/object/installer/raw.rs | 49 +++++++++++++++--- updatehub/src/utils/io.rs | 74 ++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 7 deletions(-) diff --git a/updatehub/src/object/installer/raw.rs b/updatehub/src/object/installer/raw.rs index 18386c1c..815e5968 100644 --- a/updatehub/src/object/installer/raw.rs +++ b/updatehub/src/object/installer/raw.rs @@ -24,11 +24,10 @@ impl Installer for objects::Raw { async fn check_requirements(&self, _: &Context) -> Result<()> { info!("'raw' handle checking requirements"); - let (offset, len) = target_region(self); - if let definitions::TargetType::Device(dev) = self - .target_type - .valid(Access::Region { offset, len }) - .log_error_msg("device failed vaidation")? + let access = checked_region(self) + .map_or(Access::Exclusive, |(offset, len)| Access::Region { offset, len }); + if let definitions::TargetType::Device(dev) = + self.target_type.valid(access).log_error_msg("device failed vaidation")? { utils::fs::ensure_disk_space(dev, self.required_install_size()) .log_error_msg("not enough disk space")?; @@ -90,7 +89,8 @@ impl Installer for objects::Raw { .log_error_msg("failed to open target file")?, ); target.seek(SeekFrom::Start(seek)).await.log_error_msg("failed to seek target file")?; - target + let limit = checked_region(self).map_or(u64::MAX, |(_, len)| len); + utils::io::BoundedWriter::new(target, limit) }; if self.compressed { @@ -129,6 +129,12 @@ fn target_region(obj: &objects::Raw) -> (u64, u64) { (obj.seek * chunk_size, len) } +/// The region requirements are checked for, when it is known up front. A +/// compressed object not declaring its uncompressed size is not. +fn checked_region(obj: &objects::Raw) -> Option<(u64, u64)> { + (!obj.compressed || obj.required_uncompressed_size != 0).then(|| target_region(obj)) +} + #[cfg(test)] mod tests { use super::*; @@ -322,6 +328,37 @@ mod tests { Ok(()) } + #[test] + fn compressed_object_without_its_size_has_no_checked_region() -> Result<()> { + let (mut obj, ..) = + fake_raw_object(8192, 1024, 0, 2, definitions::Count::All, false, true)?; + assert_eq!(checked_region(&obj), None); + + obj.required_uncompressed_size = 8192; + assert_eq!(checked_region(&obj), Some((2048, 8192))); + + Ok(()) + } + + #[tokio::test] + async fn source_longer_than_declared_is_not_written_past_its_region() { + let size = 2048; + let chunk_size = 8; + let (mut obj, download_dir, _source_guard, target_guard, _) = + fake_raw_object(size, chunk_size, 0, 0, definitions::Count::All, false, false).unwrap(); + // The metadata claims less than the source holds. + obj.size = 1024; + let context = + Context { download_dir: download_dir.path().to_owned(), ..Context::default() }; + + let res = obj.install(&context).await; + assert!(res.is_err(), "a source longer than declared must fail the install"); + + let target = std::fs::read(target_guard.path()).unwrap(); + assert!(target[..1024].iter().all(|b| *b == ORIGINAL_BYTE)); + assert!(target[1024..].iter().all(|b| *b == DEFAULT_BYTE)); + } + /// The check must stop at the end of the object it guards. Objects install /// largest first, so the neighbour is already carrying the new version by /// the time this one is checked, and reading into it skips an object that diff --git a/updatehub/src/utils/io.rs b/updatehub/src/utils/io.rs index e7670e18..cc10e617 100644 --- a/updatehub/src/utils/io.rs +++ b/updatehub/src/utils/io.rs @@ -3,7 +3,12 @@ // SPDX-License-Identifier: Apache-2.0 use slog_scope::trace; -use std::{pin::Pin, time::Duration}; +use std::{ + io, + pin::Pin, + task::{Context, Poll, ready}, + time::Duration, +}; use tokio::io::{AsyncRead, AsyncWrite, BufReader, BufWriter}; use tokio_io_timeout::{TimeoutReader, TimeoutWriter}; @@ -32,3 +37,70 @@ where w.set_timeout(Some(Duration::from_secs(5))); Box::pin(BufWriter::with_capacity(chunk_size, w)) } + +/// Fails a write going past `limit` bytes, so an object cannot reach beyond +/// the region its requirements were checked for. +pub(crate) struct BoundedWriter { + inner: W, + remaining: u64, +} + +impl BoundedWriter { + pub(crate) fn new(inner: W, limit: u64) -> Self { + BoundedWriter { inner, remaining: limit } + } +} + +impl AsyncWrite for BoundedWriter { + fn poll_write( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + if self.remaining == 0 && !buf.is_empty() { + return Poll::Ready(Err(io::Error::other( + "data goes past the region checked for the object", + ))); + } + + let len = buf.len().min(usize::try_from(self.remaining).unwrap_or(usize::MAX)); + let written = ready!(Pin::new(&mut self.inner).poll_write(cx, &buf[..len]))?; + self.remaining -= written as u64; + + Poll::Ready(Ok(written)) + } + + fn poll_flush(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + Pin::new(&mut self.inner).poll_flush(cx) + } + + fn poll_shutdown(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + Pin::new(&mut self.inner).poll_shutdown(cx) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tokio::io::AsyncWriteExt; + + #[tokio::test] + async fn bounded_writer_takes_up_to_its_limit() { + let mut out = Vec::new(); + let mut w = BoundedWriter::new(&mut out, 4); + w.write_all(b"abcd").await.unwrap(); + w.flush().await.unwrap(); + + assert_eq!(out, b"abcd"); + } + + #[tokio::test] + async fn bounded_writer_refuses_what_goes_past_its_limit() { + let mut out = Vec::new(); + let mut w = BoundedWriter::new(&mut out, 4); + let res = w.write_all(b"abcdef").await; + + assert!(res.is_err(), "writing past the limit must fail"); + assert_eq!(out, b"abcd"); + } +} From ff21632af6769c715be6da93d90ff4e4e7fd359e Mon Sep 17 00:00:00 2001 From: Luciano Dittgen Date: Tue, 20 Oct 2026 15:49:32 -0300 Subject: [PATCH 03/11] test(fs): cover the region check on a partitioned loop device The region check only had its pure helpers tested. Exercise it against a loop device holding a mounted partition after a 1 MiB gap: a write to the gap is allowed, while one reaching into the partition, the whole disk and the partition itself are rejected. Signed-off-by: Luciano Dittgen --- updatehub/src/utils/fs.rs | 63 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/updatehub/src/utils/fs.rs b/updatehub/src/utils/fs.rs index 0d8157fd..245eb15a 100644 --- a/updatehub/src/utils/fs.rs +++ b/updatehub/src/utils/fs.rs @@ -489,6 +489,43 @@ mod tests { Ok(FakeLoopDevice { loopdev, device, _backing_file: backing_file }) } + + /// A loop device holding a single partition from `start` to its end, + /// returned along with the partition's own device node. + fn with_partition(size: u64, start: u64) -> Result<(FakeLoopDevice, PathBuf)> { + let backing_file = tempfile::NamedTempFile::new()?; + backing_file.as_file().set_len(size)?; + write_mbr(backing_file.as_file(), start / 512, (size - start) / 512)?; + + let mutex = SERIALIZE.clone(); + let _mutex = mutex.lock().unwrap(); + let loopdev = loopdev::LoopControl::open()?.next_free()?; + let device = loopdev.path().unwrap(); + loopdev.with().part_scan(true).attach(backing_file.path())?; + + // The partition node shows up asynchronously after the scan. + let partition = PathBuf::from(format!("{}p1", device.display())); + for _ in 0..50 { + if partition.exists() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(100)); + } + + Ok((FakeLoopDevice { loopdev, device, _backing_file: backing_file }, partition)) + } + } + + /// Writes an MBR with a single Linux partition of `sectors` from `start`. + fn write_mbr(file: &std::fs::File, start: u64, sectors: u64) -> io::Result<()> { + use std::os::unix::fs::FileExt; + + let mut entry = [0u8; 16]; + entry[4] = 0x83; + entry[8..12].copy_from_slice(&u32::try_from(start).unwrap().to_le_bytes()); + entry[12..16].copy_from_slice(&u32::try_from(sectors).unwrap().to_le_bytes()); + file.write_all_at(&entry, 446)?; + file.write_all_at(&[0x55, 0xaa], 510) } impl Drop for FakeLoopDevice { @@ -637,4 +674,30 @@ mod tests { "a mounted device must be rejected, got {in_use:?}" ); } + + #[test] + #[ignore = "needs root to attach a loop device"] + fn region_check_ignores_partitions_outside_the_region() { + const PARTITION_START: u64 = 1024 * 1024; + let (loop_device, partition) = + FakeLoopDevice::with_partition(LOOP_DEVICE_SIZE, PARTITION_START).unwrap(); + format(&partition, Filesystem::Ext4, None).unwrap(); + let guard = mount(&partition, Filesystem::Ext4, "").unwrap(); + + let gap = ensure_region_not_mounted(&loop_device.device, 0x2_0000..PARTITION_START); + let overlapping = + ensure_region_not_mounted(&loop_device.device, 0x2_0000..PARTITION_START + 1); + let whole_disk = ensure_not_mounted(&loop_device.device); + let on_the_partition = ensure_region_not_mounted(&partition, 0..512); + // Unmount before asserting so the device is always released. + drop(guard); + + assert!(gap.is_ok(), "the gap ahead of the partition should be free, got {gap:?}"); + for in_use in [overlapping, whole_disk, on_the_partition] { + assert!( + matches!(in_use, Err(Error::DeviceInUse { .. })), + "a write reaching the mounted partition must be rejected, got {in_use:?}" + ); + } + } } From 64b93b85f8ff2502b68a4c328d78b20c234dbf99 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 20 Oct 2026 17:14:31 -0300 Subject: [PATCH 04/11] test: detach the loop devices of failing installer tests The copy and tarball tests attached their loop device by hand and detached it only at the end of a successful run, so every failure left one attached. A missing mkfs.ext4 alone leaked one per test. They now use FakeLoopDevice, which detaches on drop. FakeLoopDevice::with_partition went on silently when the partition node never showed up, leaving the caller to fail later on an unrelated error. It now fails on the spot, detaching the device. Signed-off-by: Otavio Salvador --- updatehub/src/object/installer/copy.rs | 26 ++++------------------- updatehub/src/object/installer/tarball.rs | 21 +++--------------- updatehub/src/utils/fs.rs | 21 ++++++++++-------- 3 files changed, 19 insertions(+), 49 deletions(-) diff --git a/updatehub/src/object/installer/copy.rs b/updatehub/src/object/installer/copy.rs index 01db42b2..549f267f 100644 --- a/updatehub/src/object/installer/copy.rs +++ b/updatehub/src/object/installer/copy.rs @@ -125,14 +125,10 @@ impl Installer for objects::Copy { #[cfg(test)] mod tests { use super::*; - use crate::{object::installer::tests::SERIALIZE, utils::definitions::IdExt}; + use crate::utils::{definitions::IdExt, fs::tests::FakeLoopDevice}; use flate2::{Compression, write::GzEncoder}; use pretty_assertions::assert_eq; - use std::{ - io::{Seek, SeekFrom, Write}, - os::unix::fs::MetadataExt, - path::PathBuf, - }; + use std::{io::Write, os::unix::fs::MetadataExt, path::PathBuf}; use tokio::io::AsyncBufReadExt; const DEFAULT_BYTE: u8 = 0xF; @@ -147,21 +143,9 @@ mod tests { where F: FnMut(&mut objects::Copy), { - // Generate a sparse file for the faked device use - let mut image = tempfile::NamedTempFile::new()?; - image.seek(SeekFrom::Start(1024 * 1024 + FILE_SIZE as u64))?; - image.write_all(&[0])?; - // Setup faked device - let (loopdev, device) = { - // Loop device next_free is not thread safe - let mutex = SERIALIZE.clone(); - let _mutex = mutex.lock().unwrap(); - let loopdev = loopdev::LoopControl::open()?.next_free()?; - let device = loopdev.path().unwrap(); - loopdev.attach_file(image.path())?; - (loopdev, device) - }; + let loop_device = FakeLoopDevice::new(1024 * 1024 + FILE_SIZE as u64 + 1)?; + let device = loop_device.device.clone(); // Format the faked device utils::fs::format(&device, definitions::Filesystem::Ext4, None)?; @@ -258,8 +242,6 @@ mod tests { } } - loopdev.detach()?; - Ok(()) } diff --git a/updatehub/src/object/installer/tarball.rs b/updatehub/src/object/installer/tarball.rs index aa378497..22b749c0 100644 --- a/updatehub/src/object/installer/tarball.rs +++ b/updatehub/src/object/installer/tarball.rs @@ -69,10 +69,9 @@ impl Installer for objects::Tarball { #[cfg(test)] mod tests { use super::*; - use crate::object::installer::tests::SERIALIZE; + use crate::utils::fs::tests::FakeLoopDevice; use pretty_assertions::assert_eq; use std::{ - io::{Seek, SeekFrom, Write}, os::unix::fs::MetadataExt, path::{Path, PathBuf}, }; @@ -83,21 +82,9 @@ mod tests { where F: FnMut(&mut objects::Tarball), { - // Generate a sparse file for the faked device use - let mut image = tempfile::NamedTempFile::new()?; - image.seek(SeekFrom::Start(1024 * 1024 + CONTENT_SIZE as u64))?; - image.write_all(&[0])?; - // Setup faked device - let (loopdev, device) = { - // Loop device next_free is not thread safe - let mutex = SERIALIZE.clone(); - let _mutex = mutex.lock().unwrap(); - let loopdev = loopdev::LoopControl::open()?.next_free()?; - let device = loopdev.path().unwrap(); - loopdev.attach_file(image.path())?; - (loopdev, device) - }; + let loop_device = FakeLoopDevice::new(1024 * 1024 + CONTENT_SIZE as u64 + 1)?; + let device = loop_device.device.clone(); // Format the faked device utils::fs::format(&device, definitions::Filesystem::Ext4, None)?; @@ -147,8 +134,6 @@ mod tests { assert_metadata(&dest.join("tree/branch2/leaf"))?; } - loopdev.detach()?; - Ok(()) } diff --git a/updatehub/src/utils/fs.rs b/updatehub/src/utils/fs.rs index 245eb15a..36202f1d 100644 --- a/updatehub/src/utils/fs.rs +++ b/updatehub/src/utils/fs.rs @@ -398,7 +398,7 @@ pub(crate) fn chown(path: &Path, uid: Option<&Uid>, gid: Option<&Gid>) -> Result } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::{search_path::SearchPathGuard, *}; use crate::object::installer::tests::SERIALIZE; use pretty_assertions::assert_eq; @@ -467,16 +467,17 @@ mod tests { // Big enough to tell a loop device apart from the devtmpfs on /dev, small // enough to stay sparse on any builder. - const LOOP_DEVICE_SIZE: u64 = 64 * 1024 * 1024; + pub(crate) const LOOP_DEVICE_SIZE: u64 = 64 * 1024 * 1024; - struct FakeLoopDevice { + /// Detached when dropped, so a failing test does not leak it. + pub(crate) struct FakeLoopDevice { loopdev: loopdev::LoopDevice, - device: PathBuf, + pub(crate) device: PathBuf, _backing_file: tempfile::NamedTempFile, } impl FakeLoopDevice { - fn new(size: u64) -> Result { + pub(crate) fn new(size: u64) -> Result { let backing_file = tempfile::NamedTempFile::new()?; backing_file.as_file().set_len(size)?; @@ -492,7 +493,7 @@ mod tests { /// A loop device holding a single partition from `start` to its end, /// returned along with the partition's own device node. - fn with_partition(size: u64, start: u64) -> Result<(FakeLoopDevice, PathBuf)> { + pub(crate) fn with_partition(size: u64, start: u64) -> Result<(FakeLoopDevice, PathBuf)> { let backing_file = tempfile::NamedTempFile::new()?; backing_file.as_file().set_len(size)?; write_mbr(backing_file.as_file(), start / 512, (size - start) / 512)?; @@ -502,17 +503,19 @@ mod tests { let loopdev = loopdev::LoopControl::open()?.next_free()?; let device = loopdev.path().unwrap(); loopdev.with().part_scan(true).attach(backing_file.path())?; + let loop_device = FakeLoopDevice { loopdev, device, _backing_file: backing_file }; // The partition node shows up asynchronously after the scan. - let partition = PathBuf::from(format!("{}p1", device.display())); + let partition = PathBuf::from(format!("{}p1", loop_device.device.display())); for _ in 0..50 { if partition.exists() { - break; + return Ok((loop_device, partition)); } std::thread::sleep(std::time::Duration::from_millis(100)); } - Ok((FakeLoopDevice { loopdev, device, _backing_file: backing_file }, partition)) + Err(io::Error::new(io::ErrorKind::NotFound, format!("{partition:?} did not show up")) + .into()) } } From f6b9c7c2556c9ebea5883acc617aad3b22d2519e Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 20 Oct 2026 17:19:40 -0300 Subject: [PATCH 05/11] fix(raw): check the object fits its region before writing beside a mount Bounding every write to the declared region turned understated metadata into failed installs. uhu takes required-uncompressed-size from `gzip -l`, which before gzip 1.12 reports it modulo 4 GiB, so images that always installed failed after partly writing their target. The bound also only stopped the write once it was under way, leaving the device partly written. The region only matters when the check let the write through beside a mounted partition, so only then is the write bounded. Whatever else is written whole, as before the region check. Beside a mount, the source is checked to fit before the device is opened, uncompressing it when compressed, and a larger one is refused untouched. Signed-off-by: Otavio Salvador --- updatehub/src/object/installer/raw.rs | 192 ++++++++++++++++++++++---- updatehub/src/object/mod.rs | 2 + updatehub/src/utils/io.rs | 10 +- 3 files changed, 175 insertions(+), 29 deletions(-) diff --git a/updatehub/src/object/installer/raw.rs b/updatehub/src/object/installer/raw.rs index 815e5968..b8836af7 100644 --- a/updatehub/src/object/installer/raw.rs +++ b/updatehub/src/object/installer/raw.rs @@ -13,7 +13,7 @@ use crate::{ }; use pkg_schema::{definitions, objects}; use slog_scope::info; -use std::io::SeekFrom; +use std::{io::SeekFrom, path::Path}; use tokio::{ fs, io::{AsyncRead, AsyncReadExt, AsyncSeekExt}, @@ -45,9 +45,7 @@ impl Installer for objects::Raw { }; let source = context.download_dir.join(self.sha256sum()); let chunk_size = self.chunk_size.0; - let skip = self.skip.0 * chunk_size as u64; let truncate = self.truncate.0; - let count = count_bytes(self); // The check must stop at the end of what this object writes, otherwise // a neighbouring object sharing the device answers for it. let (seek, region_len) = target_region(self); @@ -67,17 +65,19 @@ impl Installer for objects::Raw { return Ok(()); } - let mut input: Box = { - let mut input = utils::io::timed_buf_reader( - chunk_size, - fs::File::open(source).await.log_error_msg("failed to open source file")?, - ); - input.seek(SeekFrom::Start(skip)).await.log_error_msg("failed to seek source file")?; - match count { - None => Box::new(input), - Some(n) => Box::new(input.take(n)), + // The region check lets a whole disk be written beside the partitions + // it does not reach, even when they are mounted. Only then does the + // region matter, so only then is the write held to it, and the data + // is checked to fit before the device is touched. + let limit = match checked_region(self) { + Some((_, len)) if utils::fs::ensure_not_mounted(device).is_err() => { + ensure_fits(self, &source, len).await?; + Some(len) } + _ => None, }; + + let mut input = open_source(self, &source).await?; let mut target = { let mut target = utils::io::timed_buf_writer( chunk_size, @@ -89,8 +89,7 @@ impl Installer for objects::Raw { .log_error_msg("failed to open target file")?, ); target.seek(SeekFrom::Start(seek)).await.log_error_msg("failed to seek target file")?; - let limit = checked_region(self).map_or(u64::MAX, |(_, len)| len); - utils::io::BoundedWriter::new(target, limit) + utils::io::BoundedWriter::new(target, limit.unwrap_or(u64::MAX)) }; if self.compressed { @@ -107,6 +106,52 @@ impl Installer for objects::Raw { } } +async fn open_source(obj: &objects::Raw, source: &Path) -> Result> { + let mut input = utils::io::timed_buf_reader( + obj.chunk_size.0, + fs::File::open(source).await.log_error_msg("failed to open source file")?, + ); + input + .seek(SeekFrom::Start(obj.skip.0 * obj.chunk_size.0 as u64)) + .await + .log_error_msg("failed to seek source file")?; + + Ok(match count_bytes(obj) { + None => Box::new(input), + Some(n) => Box::new(input.take(n)), + }) +} + +/// A compressed source is uncompressed to measure it, only its metadata +/// telling its size otherwise. +async fn ensure_fits(obj: &objects::Raw, source: &Path, len: u64) -> Result<()> { + let fits = if obj.compressed { + let mut input = open_source(obj, source).await?; + let mut sink = utils::io::BoundedWriter::new(tokio::io::sink(), len); + let res = compress_tools::tokio_support::uncompress_data(&mut input, &mut sink).await; + // Going past the limit fails the uncompression too, that is not a + // corrupted source. + if !sink.overflowed() { + res.log_error_msg("failed to uncompress data")?; + } + !sink.overflowed() + } else { + let available = fs::metadata(source) + .await + .log_error_msg("failed to read source file metadata")? + .len() + .saturating_sub(obj.skip.0 * obj.chunk_size.0 as u64); + count_bytes(obj).map_or(available, |n| n.min(available)) <= len + }; + + if !fits { + return Err(Error::LargerThanDeclared(obj.filename.clone())) + .log_error_msg("object does not fit the region checked for it"); + } + + Ok(()) +} + fn count_bytes(obj: &objects::Raw) -> Option { match obj.count { definitions::Count::All => None, @@ -340,23 +385,114 @@ mod tests { Ok(()) } + fn gzip(data: &[u8]) -> Vec { + let mut e = GzEncoder::new(Vec::new(), Compression::default()); + e.write_all(data).unwrap(); + e.finish().unwrap() + } + + fn raw_object( + download_dir: &std::path::Path, + name: &str, + data: &[u8], + compressed: bool, + seek: u64, + target: &std::path::Path, + ) -> objects::Raw { + let source = if compressed { gzip(data) } else { data.to_vec() }; + std::fs::write(download_dir.join(name), &source).unwrap(); + + objects::Raw { + filename: name.to_string(), + size: source.len() as u64, + sha256sum: name.to_string(), + target_type: definitions::TargetType::Device(target.into()), + + install_if_different: None, + compressed, + required_uncompressed_size: if compressed { data.len() as u64 } else { 0 }, + chunk_size: definitions::ChunkSize(512), + skip: definitions::Skip(0), + seek: seek / 512, + count: definitions::Count::All, + truncate: definitions::Truncate(false), + } + } + #[tokio::test] - async fn source_longer_than_declared_is_not_written_past_its_region() { - let size = 2048; - let chunk_size = 8; - let (mut obj, download_dir, _source_guard, target_guard, _) = - fake_raw_object(size, chunk_size, 0, 0, definitions::Count::All, false, false).unwrap(); - // The metadata claims less than the source holds. - obj.size = 1024; - let context = - Context { download_dir: download_dir.path().to_owned(), ..Context::default() }; + async fn source_fitting_its_region_fits() { + let dir = tempdir().unwrap(); + let data = vec![ORIGINAL_BYTE; 4096]; + for compressed in [false, true] { + let obj = raw_object(dir.path(), "obj", &data, compressed, 0, dir.path()); + let source = dir.path().join("obj"); + ensure_fits(&obj, &source, 4096).await.unwrap(); + } + } + + #[tokio::test] + async fn source_longer_than_its_region_does_not_fit() { + let dir = tempdir().unwrap(); + let data = vec![ORIGINAL_BYTE; 4096]; + for compressed in [false, true] { + let obj = raw_object(dir.path(), "obj", &data, compressed, 0, dir.path()); + let source = dir.path().join("obj"); + let res = ensure_fits(&obj, &source, 4095).await; + assert!( + matches!(res, Err(Error::LargerThanDeclared(_))), + "compressed: {compressed}, got {res:?}" + ); + } + } + + #[tokio::test] + async fn source_fits_once_cut_by_skip_and_count() { + let dir = tempdir().unwrap(); + let mut obj = raw_object(dir.path(), "obj", &[ORIGINAL_BYTE; 4096], false, 0, dir.path()); + let source = dir.path().join("obj"); + obj.skip = definitions::Skip(2); + ensure_fits(&obj, &source, 3072).await.unwrap(); + assert!(ensure_fits(&obj, &source, 3071).await.is_err()); + + obj.count = definitions::Count::Limited(1); + ensure_fits(&obj, &source, 512).await.unwrap(); + } - let res = obj.install(&context).await; - assert!(res.is_err(), "a source longer than declared must fail the install"); + #[tokio::test] + async fn corrupted_compressed_source_is_not_reported_as_too_large() { + let dir = tempdir().unwrap(); + let mut obj = raw_object(dir.path(), "obj", &[ORIGINAL_BYTE; 4096], true, 0, dir.path()); + let source = dir.path().join("obj"); + let mut gz = std::fs::read(&source).unwrap(); + gz.truncate(gz.len() / 2); + std::fs::write(&source, gz).unwrap(); + obj.size /= 2; + + let res = ensure_fits(&obj, &source, 4096).await; + assert!(matches!(res, Err(Error::Uncompress(_))), "got {res:?}"); + } - let target = std::fs::read(target_guard.path()).unwrap(); - assert!(target[..1024].iter().all(|b| *b == ORIGINAL_BYTE)); - assert!(target[1024..].iter().all(|b| *b == DEFAULT_BYTE)); + /// Nothing else being in use, the region is irrelevant and the object is + /// written whole, however much its metadata understates it. + #[tokio::test] + async fn source_longer_than_declared_is_written_whole_when_nothing_is_mounted() { + for compressed in [false, true] { + let (mut obj, download_dir, _source_guard, target_guard, _) = + fake_raw_object(2048, 8, 0, 0, definitions::Count::All, false, compressed).unwrap(); + if compressed { + obj.required_uncompressed_size = 1024; + } else { + obj.size = 1024; + } + let context = + Context { download_dir: download_dir.path().to_owned(), ..Context::default() }; + + obj.check_requirements(&context).await.unwrap(); + obj.install(&context).await.unwrap(); + + let target = std::fs::read(target_guard.path()).unwrap(); + assert!(target.iter().all(|b| *b == ORIGINAL_BYTE), "compressed: {compressed}"); + } } /// The check must stop at the end of the object it guards. Objects install diff --git a/updatehub/src/object/mod.rs b/updatehub/src/object/mod.rs index 46036252..6bf6e887 100644 --- a/updatehub/src/object/mod.rs +++ b/updatehub/src/object/mod.rs @@ -21,6 +21,8 @@ pub enum Error { FwSetEnvNoScriptOption, #[display("unsupported object model")] Unsupported, + #[display("object {_0:?} holds more data than its metadata declares")] + LargerThanDeclared(#[error(not(source))] String), Utils(crate::utils::Error), Firmware(crate::firmware::Error), diff --git a/updatehub/src/utils/io.rs b/updatehub/src/utils/io.rs index cc10e617..b4bb91ba 100644 --- a/updatehub/src/utils/io.rs +++ b/updatehub/src/utils/io.rs @@ -43,11 +43,16 @@ where pub(crate) struct BoundedWriter { inner: W, remaining: u64, + overflowed: bool, } impl BoundedWriter { pub(crate) fn new(inner: W, limit: u64) -> Self { - BoundedWriter { inner, remaining: limit } + BoundedWriter { inner, remaining: limit, overflowed: false } + } + + pub(crate) fn overflowed(&self) -> bool { + self.overflowed } } @@ -58,6 +63,7 @@ impl AsyncWrite for BoundedWriter { buf: &[u8], ) -> Poll> { if self.remaining == 0 && !buf.is_empty() { + self.overflowed = true; return Poll::Ready(Err(io::Error::other( "data goes past the region checked for the object", ))); @@ -90,6 +96,7 @@ mod tests { let mut w = BoundedWriter::new(&mut out, 4); w.write_all(b"abcd").await.unwrap(); w.flush().await.unwrap(); + assert!(!w.overflowed()); assert_eq!(out, b"abcd"); } @@ -101,6 +108,7 @@ mod tests { let res = w.write_all(b"abcdef").await; assert!(res.is_err(), "writing past the limit must fail"); + assert!(w.overflowed()); assert_eq!(out, b"abcd"); } } From e9a37a81faeb33a1d5a5b446de583dcf9abaf344 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 20 Oct 2026 17:19:40 -0300 Subject: [PATCH 06/11] test(raw): install beside a mounted partition of a loop disk Only the region check itself was exercised on a device, never the raw installer going through it. Lay out an am335x on a loop disk, its rootfs partition mounted from 1 MiB: MLO and U-Boot install in the gap, plain and compressed, without touching the partition; an object reaching into it is refused; and one holding more data than declared is refused without writing. Signed-off-by: Otavio Salvador --- updatehub/src/object/installer/raw.rs | 97 +++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/updatehub/src/object/installer/raw.rs b/updatehub/src/object/installer/raw.rs index b8836af7..fa5ce2d3 100644 --- a/updatehub/src/object/installer/raw.rs +++ b/updatehub/src/object/installer/raw.rs @@ -183,6 +183,7 @@ fn checked_region(obj: &objects::Raw) -> Option<(u64, u64)> { #[cfg(test)] mod tests { use super::*; + use crate::utils::fs::tests::{FakeLoopDevice, LOOP_DEVICE_SIZE}; use flate2::{Compression, write::GzEncoder}; use pretty_assertions::assert_eq; use std::io::{Seek, Write}; @@ -495,6 +496,102 @@ mod tests { } } + /// Bootloader layout of an am335x: MLO at 128 KiB and U-Boot at 384 KiB of + /// the disk, the rootfs mounted from 1 MiB. + const PARTITION_START: u64 = 1024 * 1024; + const MLO_AT: u64 = 0x2_0000; + const UBOOT_AT: u64 = 0x6_0000; + + fn mounted_disk() -> (FakeLoopDevice, utils::fs::MountGuard) { + let (disk, partition) = + FakeLoopDevice::with_partition(LOOP_DEVICE_SIZE, PARTITION_START).unwrap(); + utils::fs::format(&partition, definitions::Filesystem::Ext4, None).unwrap(); + let guard = utils::fs::mount(&partition, definitions::Filesystem::Ext4, "").unwrap(); + std::fs::write(guard.mount_point().join("canary"), b"alive").unwrap(); + + (disk, guard) + } + + fn read_at(device: &std::path::Path, offset: u64, len: usize) -> Vec { + use std::os::unix::fs::FileExt; + + let mut buf = vec![0; len]; + std::fs::File::open(device).unwrap().read_exact_at(&mut buf, offset).unwrap(); + buf + } + + #[tokio::test] + #[ignore = "needs root to attach a loop device"] + async fn bootloader_installs_beside_a_mounted_partition() { + let (disk, guard) = mounted_disk(); + let download_dir = tempdir().unwrap(); + let context = + Context { download_dir: download_dir.path().to_owned(), ..Context::default() }; + let rootfs_before = read_at(&disk.device, PARTITION_START, 64 * 1024); + + for compressed in [false, true] { + let mlo = vec![0x5a; 0x1_415c]; + let uboot = vec![0xa5; 0x6_2f48]; + for (name, data, at) in [("mlo", &mlo, MLO_AT), ("uboot", &uboot, UBOOT_AT)] { + let obj = raw_object(download_dir.path(), name, data, compressed, at, &disk.device); + obj.check_requirements(&context).await.unwrap(); + obj.install(&context).await.unwrap(); + + assert_eq!(&read_at(&disk.device, at, data.len()), data, "{name}, {compressed}"); + } + } + + assert_eq!(read_at(&disk.device, PARTITION_START, 64 * 1024), rootfs_before); + assert_eq!(std::fs::read(guard.mount_point().join("canary")).unwrap(), b"alive"); + } + + #[tokio::test] + #[ignore = "needs root to attach a loop device"] + async fn object_reaching_a_mounted_partition_is_refused() { + let (disk, _guard) = mounted_disk(); + let download_dir = tempdir().unwrap(); + let context = + Context { download_dir: download_dir.path().to_owned(), ..Context::default() }; + + // Ends a single sector into the partition. + let data = vec![0x5a; usize::try_from(PARTITION_START - UBOOT_AT).unwrap() + 512]; + let obj = raw_object(download_dir.path(), "uboot", &data, false, UBOOT_AT, &disk.device); + let res = obj.check_requirements(&context).await; + + assert!(matches!(res, Err(Error::Utils(utils::Error::DeviceInUse { .. }))), "got {res:?}"); + } + + /// Beside a mounted partition the write is held to the declared region, so + /// an object holding more data must be refused before touching the disk. + #[tokio::test] + #[ignore = "needs root to attach a loop device"] + async fn object_larger_than_declared_beside_a_mounted_partition_is_refused() { + let (disk, _guard) = mounted_disk(); + let download_dir = tempdir().unwrap(); + let context = + Context { download_dir: download_dir.path().to_owned(), ..Context::default() }; + + for compressed in [false, true] { + let data = vec![0x5a; 8192]; + let mut obj = + raw_object(download_dir.path(), "mlo", &data, compressed, MLO_AT, &disk.device); + if compressed { + obj.required_uncompressed_size = 4096; + } else { + obj.size = 4096; + } + obj.check_requirements(&context).await.unwrap(); + let before = read_at(&disk.device, MLO_AT, data.len()); + + let res = obj.install(&context).await; + assert!( + matches!(res, Err(Error::LargerThanDeclared(_))), + "compressed: {compressed}, got {res:?}" + ); + assert_eq!(read_at(&disk.device, MLO_AT, data.len()), before, "{compressed}"); + } + } + /// The check must stop at the end of the object it guards. Objects install /// largest first, so the neighbour is already carrying the new version by /// the time this one is checked, and reading into it skips an object that From 408323c68eb81ec9603ea2ee1935f6d812c52efa Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 20 Oct 2026 17:19:40 -0300 Subject: [PATCH 07/11] build(nix): add e2fsprogs to the dev shell The tests attaching loop devices format them as ext4, which failed for lack of mkfs.ext4 wherever the host did not happen to provide it. Signed-off-by: Otavio Salvador --- flake.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/flake.nix b/flake.nix index 8ef16f7d..5c4aafaf 100644 --- a/flake.nix +++ b/flake.nix @@ -54,6 +54,7 @@ socat # used by excluded tests + e2fsprogs mtdutils ]; From 6c3005a92397925c78dee1c44ae9250f462d3651 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 20 Oct 2026 17:19:40 -0300 Subject: [PATCH 08/11] ci: run the tests that need root to attach loop devices These tests are ignored by default, and CI never ran them, so the in-use and region checks were only exercised on hardware. Build the test binary as the runner and run just it as root, keeping the cached build owned by the runner. The MTD tests stay out, as they load simulator modules. Signed-off-by: Otavio Salvador --- .github/workflows/linux.yml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index 60094d96..50671146 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -50,6 +50,19 @@ jobs: timeout-minutes: 10 run: cargo test --locked --release --all --all-features --no-fail-fast -- --nocapture + # The tests attaching loop devices need root, so the run above leaves + # them out. Only the test binary runs as root, which keeps the cached + # build owned by the runner. The MTD ones stay out, as they load the + # mtdram and nandsim kernel modules. + - name: Tests needing root + timeout-minutes: 10 + working-directory: updatehub + run: | + bin=$(cargo test --locked --release --all-features --lib --no-run --message-format=json \ + | sed -n 's/.*"executable":"\([^"]*\)".*/\1/p') + sudo env "PATH=$PATH" "$bin" --ignored --test-threads=1 \ + --skip utils::mtd --skip installer::flash --skip installer::ubifs + - name: Run listener test run: | export UH_LISTENER_TEST=updatehub-statechange.sock From f66c3fb634b41e67d3c9a0d4ade79115a9b1849f Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 20 Oct 2026 19:11:17 -0300 Subject: [PATCH 09/11] fix(raw): refuse to overwrite the partition table beside a mount A whole disk may be written beside its mounted partitions as long as the write misses them, but the partition tables lie outside every partition. Overwriting them left the mount working while the next boot found no partition at all. A write reaching the MBR, an EBR or a GPT of a disk with partitions is now refused while anything on that disk is mounted, as a write to the whole disk is. With nothing mounted, an image carrying the table still installs. Only what holds partitions in place is protected, so the boot loaders sharing that area keep installing: - MBR: bytes 440 to 512, leaving the boot code ahead of them to BIOS loaders such as GRUB. - EBR: every sector in the chain of an extended partition. - GPT: both headers and the whole entry arrays they declare, located from the headers in logical blocks, so tables shrunk or moved out of the way of a boot loader, as on Allwinner, are honoured. As the kernel does, a GPT is only looked for behind a protective or hybrid MBR. A table which cannot be read or makes no sense holds no partition in place and is left out, so disks partitioned from the kernel command line keep writing beside their mounts. Signed-off-by: Otavio Salvador --- updatehub/src/utils/fs.rs | 56 ++- updatehub/src/utils/mod.rs | 11 + updatehub/src/utils/partition_table.rs | 489 +++++++++++++++++++++++++ 3 files changed, 552 insertions(+), 4 deletions(-) create mode 100644 updatehub/src/utils/partition_table.rs diff --git a/updatehub/src/utils/fs.rs b/updatehub/src/utils/fs.rs index 36202f1d..fb36a25b 100644 --- a/updatehub/src/utils/fs.rs +++ b/updatehub/src/utils/fs.rs @@ -3,12 +3,12 @@ // SPDX-License-Identifier: Apache-2.0 use super::{Error, Result}; -use crate::utils::{definitions::IdExt, mtd}; +use crate::utils::{definitions::IdExt, mtd, partition_table}; use pkg_schema::definitions::{ Filesystem, target_permissions::{Gid, Uid}, }; -use slog_scope::trace; +use slog_scope::{trace, warn}; use std::{ collections::HashSet, io::{self, Seek}, @@ -79,11 +79,59 @@ pub(crate) fn ensure_not_mounted(target: &Path) -> Result<()> { } /// Like [`ensure_not_mounted`], but ignores the partitions of a whole disk -/// lying outside `region`. +/// lying outside `region`, unless `region` reaches the partition table which +/// holds every one of them in place. pub(crate) fn ensure_region_not_mounted(target: &Path, region: Range) -> Result<()> { trace!("checking whether {:?} is in use within {:?}", target, region); - ensure_devices_not_mounted(target, |device| region_block_devices(device, ®ion)) + ensure_devices_not_mounted(target, |device| region_block_devices(device, ®ion))?; + if reaches_partition_table(target, ®ion) { + trace!("{:?} holds its partition table within {:?}", target, region); + return ensure_not_mounted(target).map_err(|e| match e { + Error::DeviceInUse { device, mount_point } => { + Error::PartitionTableInUse { device, mount_point } + } + e => e, + }); + } + + Ok(()) +} + +/// Whether `region` holds part of the partition table of `target`, when it is +/// a whole disk with partitions. +fn reaches_partition_table(target: &Path, region: &Range) -> bool { + let Some(sysfs) = std::fs::metadata(target) + .ok() + .and_then(|metadata| block_device_id(&metadata)) + .and_then(|device| { + PathBuf::from(format!("/sys/dev/block/{}:{}", device.0, device.1)).canonicalize().ok() + }) + .filter(|sysfs| { + !sysfs.join("partition").exists() + && !device_ids_in(sysfs, |path| path.join("partition").exists()).is_empty() + }) + else { + return false; + }; + + let read = |name: &str| -> Option { + std::fs::read_to_string(sysfs.join(name)).ok()?.trim().parse().ok() + }; + // sysfs reports the size in 512 byte sectors, whatever the logical block + // size of the disk. + let disk = || -> Option<_> { + let size = read("size")?.checked_mul(512)?; + Some((std::fs::File::open(target).ok()?, read("queue/logical_block_size")?, size)) + }; + let Some((disk, sector_size, size)) = disk() else { + warn!("failed to read {:?}, its partition table is left unprotected", target); + return false; + }; + + partition_table::protected_ranges(&disk, sector_size, size) + .iter() + .any(|protected| overlaps(protected, region)) } fn ensure_devices_not_mounted( diff --git a/updatehub/src/utils/mod.rs b/updatehub/src/utils/mod.rs index 7c5324b0..7a1146ce 100644 --- a/updatehub/src/utils/mod.rs +++ b/updatehub/src/utils/mod.rs @@ -8,6 +8,7 @@ pub(crate) mod fs; pub(crate) mod io; pub(crate) mod log; pub(crate) mod mtd; +pub(crate) mod partition_table; #[cfg(any(test, feature = "test-env"))] pub(crate) mod test_env; @@ -45,6 +46,16 @@ pub enum Error { mount_point: std::path::PathBuf, }, + #[display( + "writing to {device:?} would overwrite its partition table, while it holds the \ + filesystem mounted at {mount_point:?}" + )] + #[from(ignore)] + PartitionTableInUse { + device: std::path::PathBuf, + mount_point: std::path::PathBuf, + }, + #[display( "{available} is not enough storage space for installation, at least {required} is required" )] diff --git a/updatehub/src/utils/partition_table.rs b/updatehub/src/utils/partition_table.rs new file mode 100644 index 00000000..668a4738 --- /dev/null +++ b/updatehub/src/utils/partition_table.rs @@ -0,0 +1,489 @@ +// Copyright (C) 2026 O.S. Systems Sofware LTDA +// +// SPDX-License-Identifier: Apache-2.0 + +//! Locates the partition tables of a disk. They lie outside every partition, +//! yet damaging them takes every partition with them. + +use slog_scope::warn; +use std::{fs::File, ops::Range, os::unix::fs::FileExt}; + +/// The disk signature, the partition entries and the boot signature. The boot +/// code ahead of them is left for a BIOS loader to replace. +const MBR_TABLE: Range = 440..512; +const EXTENDED_TYPES: [u8; 3] = [0x05, 0x0f, 0x85]; +const GPT_PROTECTIVE_TYPE: u8 = 0xee; +const GPT_SIGNATURE: &[u8] = b"EFI PART"; +/// A corrupted chain of EBRs may loop, so its walk is bounded. +const MAX_LOGICAL_PARTITIONS: usize = 256; +/// Far above the 16 KiB a GPT usually reserves for its entries. +const MAX_GPT_ENTRIES_SIZE: u64 = 1024 * 1024; + +/// Byte ranges of `disk` holding its MBR, EBRs and GPT. `sector_size` is the +/// logical block size, `disk_size` is in bytes. +/// +/// What cannot be read or makes no sense holds no partition in place, so it +/// is left out, while the parts of a table which do make sense are kept. +pub(crate) fn protected_ranges(disk: &File, sector_size: u64, disk_size: u64) -> Vec> { + let mut ranges = Vec::new(); + if sector_size < 512 || !sector_size.is_power_of_two() { + warn!("unsupported logical block size {}, partition table left unprotected", sector_size); + return ranges; + } + + let disk = Disk { file: disk, sector_size, sectors: disk_size / sector_size }; + let Some(mbr) = disk.read(0, 512).filter(|mbr| has_boot_signature(mbr)) else { + return ranges; + }; + ranges.push(MBR_TABLE); + + for entry in mbr_entries(&mbr).filter(|entry| EXTENDED_TYPES.contains(&entry.kind)) { + disk.ebrs(&entry, &mut ranges); + } + // The kernel only looks for a GPT behind a protective or hybrid MBR, a + // stale GPT header on an MBR disk holds nothing in place. + if mbr_entries(&mbr).any(|entry| entry.kind == GPT_PROTECTIVE_TYPE) { + disk.gpt(&mut ranges); + } + + ranges +} + +struct Disk<'a> { + file: &'a File, + sector_size: u64, + sectors: u64, +} + +impl Disk<'_> { + /// Reads `len` bytes from sector `lba`, which must lie on the disk. + fn read(&self, lba: u64, len: usize) -> Option> { + if lba >= self.sectors { + warn!("partition table at sector {} lies past the end of the disk", lba); + return None; + } + let mut buf = vec![0; len]; + match self.file.read_exact_at(&mut buf, lba * self.sector_size) { + Ok(()) => Some(buf), + Err(e) => { + warn!("failed to read the partition table at sector {}: {}", lba, e); + None + } + } + } + + fn sectors_range(&self, lba: u64, count: u64) -> Range { + lba * self.sector_size..(lba + count) * self.sector_size + } + + /// Walks the chain of EBRs of an extended partition. Each EBR sits in + /// the sector ahead of its logical partition and links to the next one, + /// relative to the start of the extended partition. + fn ebrs(&self, extended: &MbrEntry, ranges: &mut Vec>) { + let container = extended.start..extended.start.saturating_add(extended.sectors); + let mut next = container.start; + + for _ in 0..MAX_LOGICAL_PARTITIONS { + if !container.contains(&next) { + warn!("EBR at sector {} lies outside its extended partition", next); + return; + } + let Some(ebr) = self.read(next, 512).filter(|ebr| has_boot_signature(ebr)) else { + return; + }; + ranges.push(self.sectors_range(next, 1)); + + match mbr_entries(&ebr).nth(1) { + Some(link) if EXTENDED_TYPES.contains(&link.kind) => { + next = container.start.saturating_add(link.start); + } + _ => return, + } + } + + warn!("chain of EBRs longer than {} logical partitions", MAX_LOGICAL_PARTITIONS); + } + + /// The primary GPT and its backup. As the kernel does, the backup is + /// looked for where the primary says, or on the last sector when the + /// primary is unusable. + fn gpt(&self, ranges: &mut Vec>) { + let primary = self.gpt_header(1, ranges); + let alternate = + primary.as_ref().map_or(self.sectors.saturating_sub(1), |primary| primary.alternate); + let backup = self.gpt_header(alternate, ranges); + + for header in primary.iter().chain(&backup) { + let count = header.entries_len.div_ceil(self.sector_size); + if header.entries_lba.checked_add(count).is_none_or(|end| end > self.sectors) { + warn!("GPT entries at sector {} lie past the end of the disk", header.entries_lba); + } else { + ranges.push(self.sectors_range(header.entries_lba, count)); + } + } + } + + /// The header at `lba`. Carrying the GPT signature is enough to protect + /// its sector, its fields have to make sense to locate its entries. + fn gpt_header(&self, lba: u64, ranges: &mut Vec>) -> Option { + let raw = self.read(lba, 92).filter(|raw| raw.starts_with(GPT_SIGNATURE))?; + ranges.push(self.sectors_range(lba, 1)); + + let header = GptHeader::parse(&raw); + if header.is_none() { + warn!("GPT header at sector {} does not describe its entries", lba); + } + header + } +} + +struct MbrEntry { + kind: u8, + start: u64, + sectors: u64, +} + +fn mbr_entries(sector: &[u8]) -> impl Iterator + '_ { + sector[446..510].chunks_exact(16).map(|entry| MbrEntry { + kind: entry[4], + start: le32(entry, 8), + sectors: le32(entry, 12), + }) +} + +fn has_boot_signature(sector: &[u8]) -> bool { + sector[510..512] == [0x55, 0xaa] +} + +struct GptHeader { + alternate: u64, + entries_lba: u64, + /// The whole array the header declares, its checksum covering the + /// unused entries too. + entries_len: u64, +} + +impl GptHeader { + fn parse(raw: &[u8]) -> Option { + let entry_size = le32(raw, 84); + if entry_size < 128 || entry_size % 8 != 0 { + return None; + } + + Some(GptHeader { + alternate: le64(raw, 32), + entries_lba: le64(raw, 72), + entries_len: le32(raw, 80) + .checked_mul(entry_size) + .filter(|len| *len <= MAX_GPT_ENTRIES_SIZE && *len > 0)?, + }) + } +} + +fn le32(bytes: &[u8], at: usize) -> u64 { + u32::from_le_bytes(bytes[at..at + 4].try_into().unwrap()).into() +} + +fn le64(bytes: &[u8], at: usize) -> u64 { + u64::from_le_bytes(bytes[at..at + 8].try_into().unwrap()) +} + +#[cfg(test)] +pub(crate) mod tests { + use super::*; + use pretty_assertions::assert_eq; + use std::io; + + pub(crate) const LINUX: u8 = 0x83; + pub(crate) const EXTENDED: u8 = 0x05; + + /// Writes an MBR holding `entries`, as `(type, start, sectors)`. + pub(crate) fn write_mbr(file: &File, entries: &[(u8, u64, u64)]) -> io::Result<()> { + write_mbr_at(file, 0, entries) + } + + fn write_mbr_at(file: &File, offset: u64, entries: &[(u8, u64, u64)]) -> io::Result<()> { + for (i, (kind, start, sectors)) in (0u64..).zip(entries) { + let mut entry = [0u8; 16]; + entry[4] = *kind; + entry[8..12].copy_from_slice(&u32::try_from(*start).unwrap().to_le_bytes()); + entry[12..16].copy_from_slice(&u32::try_from(*sectors).unwrap().to_le_bytes()); + file.write_all_at(&entry, offset + 446 + 16 * i)?; + } + file.write_all_at(&[0x55, 0xaa], offset + 510) + } + + /// Writes the EBR at `ebr`, an offset into the extended partition at + /// `extended`, describing a logical partition of `sectors` from + /// `start` sectors after the EBR and linking to the EBR at `next`. + pub(crate) fn write_ebr( + file: &File, + sector_size: u64, + extended: u64, + ebr: u64, + (start, sectors): (u64, u64), + next: Option, + ) -> io::Result<()> { + let mut entries = vec![(LINUX, start, sectors)]; + entries.extend(next.map(|next| (EXTENDED, next, start + sectors))); + write_mbr_at(file, (extended + ebr) * sector_size, &entries) + } + + /// Lays out a GPT the kernel accepts: a protective MBR, the primary + /// header with its entries at `entries_lba` and the backup at the end. + pub(crate) struct Gpt { + pub(crate) sector_size: u64, + pub(crate) sectors: u64, + pub(crate) entries_lba: u64, + pub(crate) entries: u32, + /// First and last sector of each partition. + pub(crate) partitions: Vec<(u64, u64)>, + } + + impl Gpt { + pub(crate) fn new(sector_size: u64, sectors: u64, partitions: Vec<(u64, u64)>) -> Gpt { + Gpt { sector_size, sectors, entries_lba: 2, entries: 128, partitions } + } + + fn array_sectors(&self) -> u64 { + (u64::from(self.entries) * 128).div_ceil(self.sector_size) + } + + pub(crate) fn write(&self, file: &File) -> io::Result<()> { + let last = self.sectors - 1; + write_mbr(file, &[(GPT_PROTECTIVE_TYPE, 1, last.min(u64::from(u32::MAX)))])?; + + let mut array = vec![0u8; usize::try_from(self.entries).unwrap() * 128]; + for (entry, (first, last)) in array.chunks_exact_mut(128).zip(&self.partitions) { + // Linux filesystem data, any non-zero type would do. + entry[0..16].copy_from_slice(&[ + 0xaf, 0x3d, 0xc6, 0x0f, 0x83, 0x84, 0x72, 0x47, 0x8e, 0x79, 0x3d, 0x69, 0xd8, + 0x47, 0x7d, 0xe4, + ]); + entry[16] = 1; + entry[32..40].copy_from_slice(&first.to_le_bytes()); + entry[40..48].copy_from_slice(&last.to_le_bytes()); + } + let backup_entries = last - self.array_sectors(); + let first_usable = (self.entries_lba + self.array_sectors()).max(34); + + for (lba, alternate, entries_lba) in + [(1, last, self.entries_lba), (last, 1, backup_entries)] + { + let mut header = vec![0u8; 92]; + header[0..8].copy_from_slice(GPT_SIGNATURE); + header[8..12].copy_from_slice(&0x0001_0000u32.to_le_bytes()); + header[12..16].copy_from_slice(&92u32.to_le_bytes()); + header[24..32].copy_from_slice(&lba.to_le_bytes()); + header[32..40].copy_from_slice(&alternate.to_le_bytes()); + header[40..48].copy_from_slice(&first_usable.to_le_bytes()); + header[48..56].copy_from_slice(&(backup_entries - 1).to_le_bytes()); + header[56] = 1; + header[72..80].copy_from_slice(&entries_lba.to_le_bytes()); + header[80..84].copy_from_slice(&self.entries.to_le_bytes()); + header[84..88].copy_from_slice(&128u32.to_le_bytes()); + header[88..92].copy_from_slice(&crc32(&array).to_le_bytes()); + let crc = crc32(&header); + header[16..20].copy_from_slice(&crc.to_le_bytes()); + + file.write_all_at(&header, lba * self.sector_size)?; + file.write_all_at(&array, entries_lba * self.sector_size)?; + } + + Ok(()) + } + } + + fn crc32(data: &[u8]) -> u32 { + let mut crc = !0u32; + for byte in data { + crc ^= u32::from(*byte); + for _ in 0..8 { + crc = if crc & 1 == 0 { crc >> 1 } else { (crc >> 1) ^ 0xedb8_8320 }; + } + } + !crc + } + + const SIZE: u64 = 64 * 1024 * 1024; + + fn image() -> tempfile::NamedTempFile { + let file = tempfile::NamedTempFile::new().unwrap(); + file.as_file().set_len(SIZE).unwrap(); + file + } + + fn ranges_of(file: &tempfile::NamedTempFile, sector_size: u64) -> Vec> { + let mut ranges = protected_ranges(file.as_file(), sector_size, SIZE); + ranges.sort_by_key(|range| (range.start, range.end)); + ranges.dedup(); + ranges + } + + #[test] + fn crc32_matches_the_reference_value() { + assert_eq!(crc32(b"123456789"), 0xcbf4_3926); + } + + #[test] + fn blank_disk_has_nothing_to_protect() { + assert_eq!(ranges_of(&image(), 512), vec![]); + } + + #[test] + fn mbr_protects_its_table_but_not_the_boot_code() { + let file = image(); + write_mbr(file.as_file(), &[(LINUX, 2048, 8192)]).unwrap(); + + assert_eq!(ranges_of(&file, 512), vec![440..512]); + } + + #[test] + fn stale_gpt_on_an_mbr_disk_is_ignored() { + let file = image(); + Gpt::new(512, SIZE / 512, vec![(2048, 10239)]).write(file.as_file()).unwrap(); + write_mbr(file.as_file(), &[(LINUX, 2048, 8192)]).unwrap(); + + assert_eq!(ranges_of(&file, 512), vec![440..512]); + } + + #[test] + fn every_ebr_of_an_extended_partition_is_protected() { + let file = image(); + let f = file.as_file(); + write_mbr(f, &[(LINUX, 2048, 8192), (EXTENDED, 16384, 65536)]).unwrap(); + write_ebr(f, 512, 16384, 0, (2048, 8192), Some(16384)).unwrap(); + write_ebr(f, 512, 16384, 16384, (2048, 8192), Some(32768)).unwrap(); + write_ebr(f, 512, 16384, 32768, (2048, 8192), None).unwrap(); + + assert_eq!( + ranges_of(&file, 512), + vec![ + 440..512, + 16384 * 512..16385 * 512, + 32768 * 512..32769 * 512, + 49152 * 512..49153 * 512 + ] + ); + } + + #[test] + fn looping_chain_of_ebrs_keeps_what_it_walked() { + let file = image(); + let f = file.as_file(); + write_mbr(f, &[(EXTENDED, 16384, 65536)]).unwrap(); + write_ebr(f, 512, 16384, 0, (2048, 8192), Some(16384)).unwrap(); + write_ebr(f, 512, 16384, 16384, (2048, 8192), Some(0)).unwrap(); + + assert_eq!( + ranges_of(&file, 512), + vec![440..512, 16384 * 512..16385 * 512, 32768 * 512..32769 * 512] + ); + } + + #[test] + fn ebr_linking_outside_its_extended_partition_ends_the_chain() { + let file = image(); + let f = file.as_file(); + write_mbr(f, &[(EXTENDED, 16384, 32768)]).unwrap(); + write_ebr(f, 512, 16384, 0, (2048, 8192), Some(65536)).unwrap(); + + assert_eq!(ranges_of(&file, 512), vec![440..512, 16384 * 512..16385 * 512]); + } + + #[test] + fn gpt_protects_both_headers_and_their_entries() { + let sectors = SIZE / 512; + let file = image(); + Gpt::new(512, sectors, vec![(2048, sectors - 34)]).write(file.as_file()).unwrap(); + + assert_eq!( + ranges_of(&file, 512), + vec![ + 440..512, + 512..1024, + 1024..34 * 512, + (sectors - 33) * 512..(sectors - 1) * 512, + (sectors - 1) * 512..SIZE, + ] + ); + } + + #[test] + fn gpt_follows_the_logical_block_size() { + let sectors = SIZE / 4096; + let file = image(); + Gpt::new(4096, sectors, vec![(256, sectors - 6)]).write(file.as_file()).unwrap(); + + assert_eq!( + ranges_of(&file, 4096), + vec![ + 440..512, + 4096..8192, + 8192..6 * 4096, + (sectors - 5) * 4096..(sectors - 1) * 4096, + (sectors - 1) * 4096..SIZE, + ] + ); + } + + #[test] + fn gpt_protects_only_the_entries_it_declares() { + let sectors = SIZE / 512; + let file = image(); + let gpt = Gpt { entries: 4, ..Gpt::new(512, sectors, vec![(2048, sectors - 3)]) }; + gpt.write(file.as_file()).unwrap(); + + assert_eq!( + ranges_of(&file, 512), + vec![ + 440..512, + 512..1024, + 1024..1536, + (sectors - 2) * 512..(sectors - 1) * 512, + (sectors - 1) * 512..SIZE, + ] + ); + } + + #[test] + fn gpt_entries_may_be_relocated() { + let sectors = SIZE / 512; + let file = image(); + let gpt = Gpt { entries_lba: 2016, ..Gpt::new(512, sectors, vec![(2048, sectors - 34)]) }; + gpt.write(file.as_file()).unwrap(); + + assert_eq!(ranges_of(&file, 512)[2], 2016 * 512..2048 * 512); + } + + #[test] + fn missing_backup_gpt_leaves_the_primary_protected() { + let sectors = SIZE / 512; + let file = image(); + Gpt::new(512, sectors, vec![(2048, sectors - 34)]).write(file.as_file()).unwrap(); + file.as_file().write_all_at(&[0; 512], SIZE - 512).unwrap(); + + assert_eq!(ranges_of(&file, 512), vec![440..512, 512..1024, 1024..34 * 512]); + } + + #[test] + fn gpt_header_making_no_sense_protects_only_its_sector() { + let sectors = SIZE / 512; + let file = image(); + Gpt::new(512, sectors, vec![(2048, sectors - 34)]).write(file.as_file()).unwrap(); + // An entry size of zero, and no backup to fall back to. + file.as_file().write_all_at(&[0; 4], 512 + 84).unwrap(); + file.as_file().write_all_at(&[0; 512], SIZE - 512).unwrap(); + + assert_eq!(ranges_of(&file, 512), vec![440..512, 512..1024]); + } + + #[test] + fn unsupported_block_size_protects_nothing() { + let file = image(); + write_mbr(file.as_file(), &[(LINUX, 2048, 8192)]).unwrap(); + + assert_eq!(ranges_of(&file, 0), vec![]); + assert_eq!(ranges_of(&file, 1000), vec![]); + } +} From 4ea44ab4f782ed77402fecddd448f207b12bdc14 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 20 Oct 2026 19:11:49 -0300 Subject: [PATCH 10/11] test(fs): attach loop devices over any disk image FakeLoopDevice only offered a single MBR partition. Covering the partition tables needs GPT and extended layouts, several partitions and 4 KiB logical blocks, which loopdev cannot set, so the image is laid out by the caller and the block size set through LOOP_SET_BLOCK_SIZE, rescanning the partitions afterwards. Signed-off-by: Otavio Salvador --- updatehub/src/utils/fs.rs | 61 +++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/updatehub/src/utils/fs.rs b/updatehub/src/utils/fs.rs index fb36a25b..f1648992 100644 --- a/updatehub/src/utils/fs.rs +++ b/updatehub/src/utils/fs.rs @@ -542,9 +542,33 @@ pub(crate) mod tests { /// A loop device holding a single partition from `start` to its end, /// returned along with the partition's own device node. pub(crate) fn with_partition(size: u64, start: u64) -> Result<(FakeLoopDevice, PathBuf)> { + let (loop_device, mut partitions) = FakeLoopDevice::with_image(size, 512, &[1], |f| { + partition_table::tests::write_mbr( + f, + &[(partition_table::tests::LINUX, start / 512, (size - start) / 512)], + ) + })?; + + Ok((loop_device, partitions.remove(0))) + } + + /// A loop device of `sector_size` logical blocks over the image + /// `write` lays out, returned along with the device nodes of the + /// partitions numbered `partitions`. + pub(crate) fn with_image( + size: u64, + sector_size: u64, + partitions: &[u32], + write: impl FnOnce(&std::fs::File) -> io::Result<()>, + ) -> Result<(FakeLoopDevice, Vec)> { + use std::os::fd::AsRawFd; + + nix::ioctl_write_int_bad!(loop_set_block_size, 0x4c09); + nix::ioctl_none_bad!(blkrrpart, nix::request_code_none!(0x12, 95)); + let backing_file = tempfile::NamedTempFile::new()?; backing_file.as_file().set_len(size)?; - write_mbr(backing_file.as_file(), start / 512, (size - start) / 512)?; + write(backing_file.as_file())?; let mutex = SERIALIZE.clone(); let _mutex = mutex.lock().unwrap(); @@ -553,32 +577,33 @@ pub(crate) mod tests { loopdev.with().part_scan(true).attach(backing_file.path())?; let loop_device = FakeLoopDevice { loopdev, device, _backing_file: backing_file }; - // The partition node shows up asynchronously after the scan. - let partition = PathBuf::from(format!("{}p1", loop_device.device.display())); + // The partitions were scanned with 512 byte blocks, so they are + // scanned again once the block size changes. + if sector_size != 512 { + let fd = loop_device.loopdev.as_raw_fd(); + unsafe { + loop_set_block_size(fd, sector_size.try_into().unwrap())?; + blkrrpart(fd)?; + } + } + + // The partition nodes show up asynchronously after the scan. + let partitions = partitions + .iter() + .map(|n| PathBuf::from(format!("{}p{n}", loop_device.device.display()))) + .collect::>(); for _ in 0..50 { - if partition.exists() { - return Ok((loop_device, partition)); + if partitions.iter().all(|partition| partition.exists()) { + return Ok((loop_device, partitions)); } std::thread::sleep(std::time::Duration::from_millis(100)); } - Err(io::Error::new(io::ErrorKind::NotFound, format!("{partition:?} did not show up")) + Err(io::Error::new(io::ErrorKind::NotFound, format!("{partitions:?} did not show up")) .into()) } } - /// Writes an MBR with a single Linux partition of `sectors` from `start`. - fn write_mbr(file: &std::fs::File, start: u64, sectors: u64) -> io::Result<()> { - use std::os::unix::fs::FileExt; - - let mut entry = [0u8; 16]; - entry[4] = 0x83; - entry[8..12].copy_from_slice(&u32::try_from(start).unwrap().to_le_bytes()); - entry[12..16].copy_from_slice(&u32::try_from(sectors).unwrap().to_le_bytes()); - file.write_all_at(&entry, 446)?; - file.write_all_at(&[0x55, 0xaa], 510) - } - impl Drop for FakeLoopDevice { fn drop(&mut self) { if let Err(e) = self.loopdev.detach() { From 550d0170aa973d71ba77d0a36832b6f0d2709fbf Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 20 Oct 2026 19:11:55 -0300 Subject: [PATCH 11/11] test: cover the partition table check on loop disks Beside a mounted partition, writing the MBR, an EBR or either GPT, also with 4 KiB logical blocks, is refused, and allowed once nothing is mounted. The boot loader layouts sharing that area keep installing: BIOS boot code ahead of the MBR table, i.MX6 at 1 KiB, i.MX8M and Rockchip at 32 KiB, am335x from 128 KiB, and Allwinner's SPL at 8 KiB beside a GPT whose entries were shrunk or moved out of its way. Signed-off-by: Otavio Salvador --- updatehub/src/object/installer/raw.rs | 57 ++++++++++ updatehub/src/utils/fs.rs | 154 ++++++++++++++++++++++++++ 2 files changed, 211 insertions(+) diff --git a/updatehub/src/object/installer/raw.rs b/updatehub/src/object/installer/raw.rs index fa5ce2d3..608192f3 100644 --- a/updatehub/src/object/installer/raw.rs +++ b/updatehub/src/object/installer/raw.rs @@ -561,6 +561,63 @@ mod tests { assert!(matches!(res, Err(Error::Utils(utils::Error::DeviceInUse { .. }))), "got {res:?}"); } + /// Overwriting the partition table beside a mounted partition leaves the + /// mount working, but the next boot finds no partition to mount. + #[tokio::test] + #[ignore = "needs root to attach a loop device"] + async fn partition_table_beside_a_mounted_partition_is_refused() { + let download_dir = tempdir().unwrap(); + let context = + Context { download_dir: download_dir.path().to_owned(), ..Context::default() }; + let sectors = LOOP_DEVICE_SIZE / 512; + let mbr = FakeLoopDevice::with_partition(LOOP_DEVICE_SIZE, PARTITION_START).unwrap(); + let gpt = FakeLoopDevice::with_image(LOOP_DEVICE_SIZE, 512, &[1], |f| { + utils::partition_table::tests::Gpt::new(512, sectors, vec![(2048, sectors - 34)]) + .write(f) + }) + .map(|(disk, mut partitions)| (disk, partitions.remove(0))) + .unwrap(); + + for ((disk, partition), regions) in + [(mbr, vec![(0, 512)]), (gpt, vec![(0, 34 * 512), ((sectors - 33) * 512, 33 * 512)])] + { + utils::fs::format(&partition, definitions::Filesystem::Ext4, None).unwrap(); + let _guard = utils::fs::mount(&partition, definitions::Filesystem::Ext4, "").unwrap(); + + for (at, len) in regions { + let data = vec![0; usize::try_from(len).unwrap()]; + let obj = raw_object(download_dir.path(), "table", &data, false, at, &disk.device); + let res = obj.check_requirements(&context).await; + + assert!( + matches!(res, Err(Error::Utils(utils::Error::PartitionTableInUse { .. }))), + "{at:#x}+{len:#x}: got {res:?}" + ); + } + } + } + + /// A BIOS loader such as GRUB's boot.img replaces the boot code ahead of + /// the partition table. + #[tokio::test] + #[ignore = "needs root to attach a loop device"] + async fn boot_code_installs_beside_a_mounted_partition() { + let (disk, guard) = mounted_disk(); + let download_dir = tempdir().unwrap(); + let context = + Context { download_dir: download_dir.path().to_owned(), ..Context::default() }; + let table_before = read_at(&disk.device, 440, 72); + + let boot_code = vec![0x5a; 440]; + let obj = raw_object(download_dir.path(), "boot", &boot_code, false, 0, &disk.device); + obj.check_requirements(&context).await.unwrap(); + obj.install(&context).await.unwrap(); + + assert_eq!(read_at(&disk.device, 0, 440), boot_code); + assert_eq!(read_at(&disk.device, 440, 72), table_before); + assert_eq!(std::fs::read(guard.mount_point().join("canary")).unwrap(), b"alive"); + } + /// Beside a mounted partition the write is held to the declared region, so /// an object holding more data must be refused before touching the disk. #[tokio::test] diff --git a/updatehub/src/utils/fs.rs b/updatehub/src/utils/fs.rs index f1648992..326bd5af 100644 --- a/updatehub/src/utils/fs.rs +++ b/updatehub/src/utils/fs.rs @@ -776,4 +776,158 @@ pub(crate) mod tests { ); } } + + /// Mounts `partition` and checks that each region of `disk` in `free` + /// may be written while each in `table` is refused for holding the + /// partition table, and that nothing is refused once unmounted. + fn check_partition_table_regions( + disk: &Path, + partition: &Path, + free: &[Range], + table: &[Range], + ) { + format(partition, Filesystem::Ext4, None).unwrap(); + let guard = mount(partition, Filesystem::Ext4, "").unwrap(); + let check = |regions: &[Range]| { + regions + .iter() + .map(|region| (region.clone(), ensure_region_not_mounted(disk, region.clone()))) + .collect::>() + }; + let mounted_free = check(free); + let mounted_table = check(table); + // Unmount before asserting so the device is always released. + drop(guard); + + for (region, res) in mounted_free { + assert!(res.is_ok(), "{region:x?} should be free beside the mount, got {res:?}"); + } + for (region, res) in mounted_table { + assert!( + matches!(res, Err(Error::PartitionTableInUse { .. })), + "{region:x?} holds the partition table, got {res:?}" + ); + } + for (region, res) in check(table) { + assert!(res.is_ok(), "{region:x?} should be free with nothing mounted, got {res:?}"); + } + } + + /// i.MX6 U-Boot at 1 KiB, i.MX8M and Rockchip at 32 KiB, am335x from + /// 128 KiB, all ahead of a partition at 1 MiB. + const BOOTLOADERS: [Range; 3] = [0x400..0x8_0000, 0x8000..0x8_0000, 0x2_0000..0x10_0000]; + + #[test] + #[ignore = "needs root to attach a loop device"] + fn region_check_protects_the_mbr() { + let (disk, partition) = + FakeLoopDevice::with_partition(LOOP_DEVICE_SIZE, 1024 * 1024).unwrap(); + + check_partition_table_regions( + &disk.device, + &partition, + &std::iter::once(0..440).chain(BOOTLOADERS).collect::>(), + &[0..512, 440..441, 446..447, 510..512], + ); + } + + #[test] + #[ignore = "needs root to attach a loop device"] + fn region_check_protects_the_ebrs() { + use partition_table::tests::{EXTENDED, LINUX, write_ebr, write_mbr}; + + const EXTENDED_START: u64 = 16384; + let (disk, partitions) = FakeLoopDevice::with_image(LOOP_DEVICE_SIZE, 512, &[5, 6], |f| { + write_mbr( + f, + &[ + (LINUX, 2048, 8192), + (EXTENDED, EXTENDED_START, LOOP_DEVICE_SIZE / 512 - EXTENDED_START), + ], + )?; + write_ebr(f, 512, EXTENDED_START, 0, (2048, 8192), Some(16384))?; + write_ebr(f, 512, EXTENDED_START, 16384, (2048, 8192), None) + }) + .unwrap(); + let first = EXTENDED_START * 512; + let second = (EXTENDED_START + 16384) * 512; + + check_partition_table_regions( + &disk.device, + &partitions[0], + &[0x2_0000..0x10_0000, first + 512..first + 1024 * 1024], + &[first..first + 1, second..second + 512, second + 446..second + 447], + ); + } + + #[test] + #[ignore = "needs root to attach a loop device"] + fn region_check_protects_the_gpt() { + let sectors = LOOP_DEVICE_SIZE / 512; + let (disk, partitions) = FakeLoopDevice::with_image(LOOP_DEVICE_SIZE, 512, &[1], |f| { + partition_table::tests::Gpt::new(512, sectors, vec![(2048, sectors - 34)]).write(f) + }) + .unwrap(); + + check_partition_table_regions( + &disk.device, + &partitions[0], + // The GPT entries take the place of i.MX6's U-Boot. + &std::iter::once(0..440).chain(BOOTLOADERS[1..].iter().cloned()).collect::>(), + &[ + 440..512, + 512..513, + 1024..1025, + 34 * 512 - 1..34 * 512, + (sectors - 33) * 512..(sectors - 33) * 512 + 1, + LOOP_DEVICE_SIZE - 1..LOOP_DEVICE_SIZE, + ], + ); + } + + #[test] + #[ignore = "needs root to attach a loop device"] + fn region_check_protects_the_gpt_of_4k_sectors() { + let sectors = LOOP_DEVICE_SIZE / 4096; + let (disk, partitions) = FakeLoopDevice::with_image(LOOP_DEVICE_SIZE, 4096, &[1], |f| { + partition_table::tests::Gpt::new(4096, sectors, vec![(256, sectors - 6)]).write(f) + }) + .unwrap(); + + check_partition_table_regions( + &disk.device, + &partitions[0], + &[&[0..440, 512..4096][..], &BOOTLOADERS[1..]].concat(), + &[ + 440..512, + 4096..4097, + 8192..8193, + 6 * 4096 - 1..6 * 4096, + LOOP_DEVICE_SIZE - 1..LOOP_DEVICE_SIZE, + ], + ); + } + + /// Allwinner's SPL sits at 8 KiB, inside the entries a GPT reserves by + /// default, so its images shrink or relocate them. + #[test] + #[ignore = "needs root to attach a loop device"] + fn region_check_lets_sunxi_spl_beside_its_gpt() { + use partition_table::tests::Gpt; + + let sectors = LOOP_DEVICE_SIZE / 512; + let shrunk = Gpt { entries: 4, ..Gpt::new(512, sectors, vec![(2048, sectors - 34)]) }; + let relocated = + Gpt { entries_lba: 2016, ..Gpt::new(512, sectors, vec![(2048, sectors - 34)]) }; + + for (gpt, free, table) in [ + (shrunk, 1536..0x8_0000, 1535..1536), + (relocated, 1024..2016 * 512, 2016 * 512..2016 * 512 + 1), + ] { + let (disk, partitions) = + FakeLoopDevice::with_image(LOOP_DEVICE_SIZE, 512, &[1], |f| gpt.write(f)).unwrap(); + + check_partition_table_regions(&disk.device, &partitions[0], &[free], &[table]); + } + } }