fix: use checked arithmetic for 32-bit FDT offset and length calculations by anjandhumalrao · Pull Request #107 · google/dtoolkit

anjandhumalrao · 2026-11-03T15:56:49Z

On 32-bit targets (such as riscv32 and i686, where usize is 32 bits), several offset and size calculations on untrusted or caller-supplied u32 / usize values could panic (with overflow checks enabled) or wrap modulo 2^32 (with overflow checks disabled):

- **`Fdt` (`src/fdt.rs`)**: `Fdt::validate_header`, `Fdt::string`, `Fdt::next_property_offset`, and `Fdt::align_tag_offset` could wrap `prop_offset + len`, `str_block_start + nameoff`, or tag alignment on crafted DTBs, bypassing `Fdt::validate` and later panicking during property iteration.
- **`values` (`src/values.rs`)**: `PropEncodedArrayIterator::new` could overflow `usize` when summing `#address-cells` / `#size-cells` or multiplying by `size_of::<u32>()`, and `<[&str] as ToPropertyValue>::property_value_len` / `<Vec<&str> as ToPropertyValue>::property_value_len` could wrap `.sum()` on aliased string slices.
- **`FdtMut` (`src/fdt_mut.rs`, `src/fdt_mut/node.rs`, `src/fdt_mut/property.rs`)**: `FdtNodeMut::add_property`, `FdtPropertyMut::set_value`, `FdtMut::shift_dt_struct`, and `FdtMut::append_raw_string` could wrap growth calculations or panic in `align_tag_offset` instead of returning `BufferError::FdtLimitExceeded`.
- **`model` (`src/model/writer.rs`)**: `DeviceTree` DTB serialization size helpers used unchecked `usize` additions before `u32::try_from` (which is infallible on 32-bit targets).

codecov · 2026-11-03T16:07:56Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.97%. Comparing base (7662cd1) to head (bd19d1c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
+ Coverage   93.61%   93.97%   +0.36%     
==========================================
  Files          26       26              
  Lines        3428     3585     +157     
==========================================
+ Hits         3209     3369     +160     
+ Misses        219      216       -3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

qwandor · 2026-11-04T17:38:38Z

+            post_string_size
+                .checked_add(required_space)
+                .ok_or(BufferError::FdtLimitExceeded {
+                    requested: usize::MAX,


Why is this usize::MAX?

Removed . Widened FdtLimitExceeded::requested to u64 so the true requested size is always reported.

qwandor · 2026-11-04T17:40:34Z

+            .len()
+            .checked_add(1) // plus null terminator
+            .ok_or(BufferError::FdtLimitExceeded {
+                requested: usize::MAX,


Why usize::MAX?

On 32-bit targets usize::MAX == u32::MAX, so BufferError::FdtLimitExceeded { requested: usize } can't represent any size > u32::MAX — usize::MAX was a placeholder when checked_add overflowed (which awkwardly formats as "requested size 4294967295 exceeds u32::MAX").

Which do you prefer? 1. Remove requested (FdtLimitExceeded unit variant, fix(fdt_mut)!:) — no buffer can exceed u32::MAX anyway, and avoids u64 overflow when val_len == usize::MAX on 64-bit. 2. Widen requested to u64 (or Option<u64> / Option<usize>) (fix(fdt_mut)!:). 3. Keep requested: usize here for a non-breaking 0.5.1, and change FdtLimitExceeded in a follow-up fix

@m4tx thoughts? I guess you have some breaking changes coming up anyway.

Yeah, we've some breaking changes coming very soon in #96, so I'm expecting the next version to be 0.6 anyway, and not 0.5.1. Therefore I think it's fine to do some breaking changes in this PR.

This specific ok_or is, however, dead code. On 32-bit targets we operate on data that is maximum isize::MAX large, because we need to construct a slice over it. isize::MAX + 1 will hence never overflow usize::MAX. Even if that alone wasn't the case, we operate on valid DTBs which contain other data (e.g. 40 byte long header), so even then "name of some property + 1" can never overflow.

Regarding the original question, I think it would be reasonable to change requested in BufferError::FdtLimitExceeded into u64. This way, we'll always be able to provide a true value.

Moreover, I think if we changed the arithmetic here to operate directly on u64 and only did a checked conversion to u32 wherever required, we'd definitely end up having much smaller diff and much more readable code (and most likely very similar performance anyway).

Done. widened FdtLimitExceeded::requested to u64 and switched append_raw_string / ensure_totalsize to u64 arithmetic.

m4tx

Hey, thanks a lot for this PR! dtoolkit was indeed written with 64-bit targets in mind and the codebase reflects that; it'll be great to see the 32-bit-related bugs addressed.

However, I think most of the changes are not actually needed, and they harm the readability of the code quite a bit. I think we should focus on hardening the code around untrusted values from the FDT, and values from the caller. A lot of the changes seem to touch invariants already validated in other parts of the code, so these should be revisited and reverted to keep this change more reviewable.

I've left some comments with the specifics about the above; I'm happy to be wrong about any of them, so let me know what you think.

m4tx · 2026-11-09T02:43:31Z

+            .len()
+            .checked_add(1) // plus null terminator
+            .ok_or(BufferError::FdtLimitExceeded {
+                requested: usize::MAX,


Yeah, we've some breaking changes coming very soon in #96, so I'm expecting the next version to be 0.6 anyway, and not 0.5.1. Therefore I think it's fine to do some breaking changes in this PR.

This specific ok_or is, however, dead code. On 32-bit targets we operate on data that is maximum isize::MAX large, because we need to construct a slice over it. isize::MAX + 1 will hence never overflow usize::MAX. Even if that alone wasn't the case, we operate on valid DTBs which contain other data (e.g. 40 byte long header), so even then "name of some property + 1" can never overflow.

Regarding the original question, I think it would be reasonable to change requested in BufferError::FdtLimitExceeded into u64. This way, we'll always be able to provide a true value.

Moreover, I think if we changed the arithmetic here to operate directly on u64 and only did a checked conversion to u32 wherever required, we'd definitely end up having much smaller diff and much more readable code (and most likely very similar performance anyway).

m4tx · 2026-11-09T02:48:22Z

-    pub(crate) fn align_tag_offset(offset: usize) -> usize {
-        offset.next_multiple_of(FDT_TAGSIZE)
+    #[must_use]
+    pub(crate) fn align_tag_offset(offset: usize) -> Option<usize> {


This is effectively dead code, as this can never overflow in practice. As mentioned in the other comment, the entire DTB that we operate on will be at most isize::MAX large, so there's no way offset can overflow usize::MAX after adding 4.

If we worry about this, let's add a debug_assert!(offset <= isize::MAX), but please keep this function infallible. This will clean up a big portion of this PR.

m4tx · 2026-11-09T03:02:29Z

+        let mem_reservations_size = self
+            .memory_reservations
+            .len()
+            .checked_add(1)
+            .and_then(|n| n.checked_mul(size_of::<MemoryReservation>()))
+            .expect("mem_reservations_size exceeds usize");
        // +FDT_TAGSIZE for FDT_END
-        let dt_struct_size = Self::calculate_node_size(string_map, &self.root) + FDT_TAGSIZE;
+        let dt_struct_size = Self::calculate_node_size(string_map, &self.root)
+            .checked_add(FDT_TAGSIZE)
+            .expect("size_dt_struct exceeds usize");
        let dt_strings_size = string_map.next_offset as usize;

        let header_size = size_of::<FdtHeader>();
        let off_mem_rsvmap = header_size;
-        let off_dt_struct = off_mem_rsvmap + mem_reservations_size;
-        let off_dt_strings = off_dt_struct + dt_struct_size;
-        let totalsize = off_dt_strings + dt_strings_size;
+        let off_dt_struct = off_mem_rsvmap
+            .checked_add(mem_reservations_size)
+            .expect("off_dt_struct exceeds usize");
+        let off_dt_strings = off_dt_struct
+            .checked_add(dt_struct_size)
+            .expect("off_dt_strings exceeds usize");
+        let totalsize = off_dt_strings
+            .checked_add(dt_strings_size)
+            .expect("totalsize exceeds usize");


I don't believe any of these checks can fail. The data that we write comes from owned in-memory allocations; meaning that if we happen to overflow usize::MAX when writing the data in the compact DTB format, it means we already must have run out of memory earlier when writing these to the intermediate memory.

The only meaningful check that we could do is u32::try_from(totalsize), but we do that already just below in this function.

Agreed. Dropped the src/model/writer.rs commit completely.

m4tx · 2026-11-09T03:09:51Z

        }
    }

    fn calculate_node_size(string_map: &mut StringMap, node: &DeviceTreeNode) -> usize {


Similarly to generate_header, each calculation here is strictly dependent on the memory that we've already allocated. This means that we'll run out of memory before we'll be able to overflow usize here anywhere.

Dropped the src/model/writer.rs commit completely.

m4tx · 2026-11-09T03:18:51Z

+        let Some(struct_end) = off_dt_struct
+            .checked_add(size_dt_struct)
+            .filter(|&end| end <= data.len())
+        else {


Maybe to improve readability, we could do something like this instead?

Suggested change

let Some(struct_end) = off_dt_struct

.checked_add(size_dt_struct)

.filter(|&end| end <= data.len())

else {

if size_dt_struct > data.len() - off_dt_struct {

We already know that data.len() > off_dt_struct (checked above), so this can never underflow.

I think we could do similar improvements in other places in this function.

m4tx · 2026-11-09T03:27:59Z

+        let new_size = old_size
+            .checked_add(amount)
+            .ok_or(BufferError::FdtLimitExceeded {
+                requested: usize::MAX,
+            })?;
+        let amount_u32 = u32::try_from(amount)
+            .ok()
+            .ok_or(BufferError::FdtLimitExceeded {
+                requested: new_size,
+            })?;
+        let new_totalsize =
+            old_totalsize
+                .checked_add(amount_u32)
+                .ok_or(BufferError::FdtLimitExceeded {
+                    requested: new_size,
+                })?;
+        let new_size_dt_struct =
+            old_size_dt_struct
+                .checked_add(amount_u32)
+                .ok_or(BufferError::FdtLimitExceeded {
+                    requested: new_size,
+                })?;
+        let new_off_dt_strings =
+            old_off_dt_strings
+                .checked_add(amount_u32)
+                .ok_or(BufferError::FdtLimitExceeded {
+                    requested: new_size,
+                })?;


We don't need to check all of this. It's enough if we check whether the new totalsize fits u32, as for each valid DTB, each of these values is strictly smaller than totalsize, and we add the same amount to all of them.

Done. Simplified shift_dt_struct to check only whether the new totalsize (in u64) fits in u32.

m4tx · 2026-11-09T03:35:03Z

-        self.iter().map(|s| s.len() + 1).sum()
+        self.iter()
+            .try_fold(0usize, |acc, s| acc.checked_add(s.len())?.checked_add(1))
+            .expect("property value length should fit in usize")


This is a good catch in fact, as this can potentially overflow usize in case the same string in the slice is duplicated.

Can we add a code comment saying that &[&str] may contain the same string multiple times and hence the sum of the lengths may exceed usize without exceeding the available memory?

Done. Added the comment explaining duplicate &str references.

m4tx · 2026-11-09T03:39:37Z

+        let chunk_cells = fields_cells
+            .iter()
+            .copied()
+            .try_fold(0usize, usize::checked_add);
+        let Some(chunk_bytes) = chunk_cells
+            .filter(|&cells| cells != 0)
+            .and_then(|cells| cells.checked_mul(size_of::<u32>()))
+            .filter(|&bytes| value.len().is_multiple_of(bytes))
+        else {
            return Err(PropertyError::PropEncodedArraySizeMismatch {
                size: value.len(),
-                chunk: chunk_cells,
+                chunk: fields_cells
+                    .iter()
+                    .fold(0usize, |acc, &cells| acc.saturating_add(cells)),


Can we explicitly use u64 for calculations here instead? Checked arithmetic makes this code much less readable.

m4tx · 2026-11-09T03:55:09Z

+                requested: usize::MAX,
+            })?;
+        let old_size = self.data.data.as_ref().len();
+        let post_string_size = if self.data.find_existing_string(name).is_some() {


This duplicates a search for a string in a string block that we already do in the self.data.add_string() call below. Each such search is O(n) and can be potentially costly if the string block is big, so we should really avoid doing this twice.

If necessary, FdtMut::add_string() (and/or related functions) should be refactored to cover the needs rather than running an expensive computation twice for the same result.

Done, removed the duplicate call.

m4tx · 2026-11-09T04:00:28Z

+                })?;
+        u32::try_from(final_size).map_err(|_e| BufferError::FdtLimitExceeded {
+            requested: final_size,
+        })?;


This is quite a big chunk of new code - what does it do exactly? It looks like it tries to validate the sizes of everything before attempting to modify the FDT, but we already do a similar validation in add_string and shift_dt_struct, so it seems redundant. Furthermore, it only checks if the buffer size doesn't exceed u32::MAX, but if the underlying buffer returns OutOfSpace (e.g. because a fixed capacity buffer will overflow), this will still behave as before.

The main change I see is that the old version will leave an unused string in the string block if the string is added successfully, but shift_dt_struct fails. This is a flaw that needs addressing, but it has nothing to do with arithmetic overflows, and is not documented in the commit message, so I'm not sure if this was the intention here.

You spotted the exact reason, that block was trying to avoid leaving an unused string in dt_strings when the resulting size exceeded u32::MAX, which duplicated add_string / shift_dt_struct and still didn't address the OutOfSpace case anyway. Removed the pre-flight block and kept just the u64 required_space calculation passed into shift_dt_struct.

On 32-bit targets (where `usize` is 32 bits), `Fdt::string` and `Fdt::next_property_offset` added untrusted `u32` `nameoff` and `len` fields to `usize` offsets before checking bounds: - A crafted 84-byte DTB with `len = 0xffff_fffc` and `nameoff = 2` (`FDT_END_NODE`) wrapped `prop_offset + len` backwards into `nameoff` when overflow checks were disabled (or panicked in `Fdt::new` when enabled), bypassing `Fdt::validate` and later panicking in `FdtPropIter`. - A crafted `nameoff` could similarly wrap `str_block_start + nameoff` backwards before `off_dt_strings`. Check `string_block_offset >= str_block_size` and `len > self.data.len() - prop_offset` (along with subtraction bounds in `Fdt::validate_header`) before adding offsets.

…gths On 32-bit targets: - Untrusted `#address-cells` or `#size-cells` values close to `u32::MAX` (such as `0x4000_0001` or `0x8000_0000`) could overflow `usize` when summing `fields_cells` or multiplying by `size_of::<u32>()` in `PropEncodedArrayIterator::new`, wrapping `chunk_bytes` and later panicking in `chunks_exact` or `split_at`. - `ToPropertyValue::property_value_len` for `&[&str]` and `Vec<&str>` summed `s.len() + 1` without overflow checks, allowing duplicated string references (e.g. 4096 references to a ~1 MiB `&str`) to wrap modulo 2^32 to `0` in release builds. Compute `chunk_cells` and `chunk_bytes` in `u64` inside `PropEncodedArrayIterator::new`, and use `try_fold` with `checked_add` in `<&[&str] as ToPropertyValue>`.

On 32-bit targets, `FdtNodeMut::add_property`, `FdtPropertyMut::set_value`, `FdtMut::shift_dt_struct`, and `FdtMut::append_raw_string` previously performed unchecked `usize` additions that could panic or wrap modulo 2^32 in release builds (shrinking `header.totalsize` or overwriting existing tokens in-place instead of returning `BufferError::FdtLimitExceeded`). Widen `BufferError::FdtLimitExceeded::requested` from `usize` to `u64` and compute buffer and structure growth in `u64` before checking whether the new `totalsize` fits in `u32`.

anjandhumalrao force-pushed the fix-32bit-offset-overflow branch from 949f74d to ff889a9 Compare November 3, 2026 17:46

qwandor requested a review from m4tx November 4, 2026 17:08

qwandor reviewed Nov 4, 2026

View reviewed changes

m4tx requested changes Nov 9, 2026

View reviewed changes

anjandhumalrao added 2 commits November 9, 2026 10:14

anjandhumalrao force-pushed the fix-32bit-offset-overflow branch from ff889a9 to ebe995e Compare November 9, 2026 10:27

anjandhumalrao force-pushed the fix-32bit-offset-overflow branch from ebe995e to bd19d1c Compare November 9, 2026 12:30

Conversation

anjandhumalrao commented Nov 3, 2026

Uh oh!

codecov Bot commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4tx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4tx Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Nov 3, 2026 •

edited

Loading

m4tx Nov 9, 2026 •

edited

Loading