Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsound implementation of transmute_buf and value_u32_list #11

Open
shinmao opened this issue Aug 8, 2023 · 2 comments
Open

Unsound implementation of transmute_buf and value_u32_list #11

shinmao opened this issue Aug 8, 2023 · 2 comments

Comments

@shinmao
Copy link

shinmao commented Aug 8, 2023

The source of unsoundness

The first unsoundness lies in transmute_buf.

dtb/src/struct_item.rs

Lines 88 to 93 in caf8d1e

unsafe fn transmute_buf<T>(buf: &mut [u8]) -> Result<&mut [T]> {
let buf = align_buf::<T>(buf)?;
Ok(from_raw_parts_mut(
buf.as_ptr() as *mut T,
buf.len() / size_of::<T>(),
))

At line 91, buf.as_ptr() would create an immutable raw pointer; however, it is then casted to a mutable raw pointer and created a slice from it. Changing the readonly permission in type conversion could lead to undefined behavior. Even though the function is not public and declared as unsafe, it was still called by other functions and could have some impact.

The second unsoundness lies in value_u32_list.

dtb/src/struct_item.rs

Lines 115 to 135 in caf8d1e

pub fn value_u32_list<'b>(&self, buf: &'b mut [u8]) -> Result<&'b [u32]> {
let value = self.value()?;
if value.len() % 4 != 0 {
return Err(Error::BadU32List);
}
let len = value.len() / 4;
let buf = unsafe { StructItem::transmute_buf(buf)? };
if buf.len() < len {
return Err(Error::BufferTooSmall);
}
for (i, val) in buf.iter_mut().enumerate().take(len) {
*val = u32::from_be(unsafe {
*(value.as_ptr().add(4 * i as usize) as *const u32)
});
}
Ok(&buf[..len])
}

At line 130, value.as_ptr() was casted to u32 raw pointer and created a misaligned pointer. The misaligned pointer was deref in the same line, leading to undefined behavior.

To reproduce the bug

To reproduce the bug in transmute_buf:

fn main() {
    let mut arr: [u32; 3] = [0; 3];
    let mut buf = unsafe {
        core::slice::from_raw_parts_mut::<u8>(
            arr.as_mut_ptr() as *mut u8,
            core::mem::size_of_val(&arr),
        )
    };

    println!("{:?}", prop.value_u32_list(&mut buf).unwrap())
}

run with miri,

error: Undefined Behavior: trying to retag from <2588> for Unique permission at alloc1308[0x0], but that tag only grants SharedReadOnly permission for this location
   --> /$HOME/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:145:9
    |
145 |         &mut *ptr::slice_from_raw_parts_mut(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to retag from <2588> for Unique permission at alloc1308[0x0], but that tag only grants SharedReadOnly permission for this location
    |         this error occurs as part of retag at alloc1308[0x0..0xc]
    |

To reproduce the bug in value_u32_list, we just need to run cargo test:

running 38 tests
test reader::tests::test_bad_magic ... ok
thread 'struct_item::tests::test_value_u32_list' panicked at 'misaligned pointer dereference: address must be a multiple of 0x4 but is 0x563ee31717fb', src/struct_item.rs:130:17

Hope this could help you debug and fix the bugs:)

@shinmao
Copy link
Author

shinmao commented Aug 8, 2023

Misaligned pointer reference could also happen in following places:

*((&self.struct_block[self.offset..]).as_ptr() as *const u32)

In StructItem::next_item(), the code at line 122 would cast struct_block which was u8 to u32. It created a misaligned pointer, then deref it at the same line.

https://github.com/ababo/dtb/blob/caf8d1ec910c405ef31a8bdf7fee374e73cffc66/src/reader.rs#L85C26-L85C26
In StructItem::read_property(), the code at line 85 would cast struct_block which was u8 to PropertyDesc which was u32. It created a misaligned pointer, then deref it at the same line.

dtb/src/reader.rs

Lines 313 to 314 in caf8d1e

let be_header = blob.as_ptr() as *const Header;
let be_magic = unsafe { (*be_header).magic };

In Reader::get_header(), the code at line 313 would cast blob which was u8 to Header which was u32. It created a misaligned pointer, then deref it at the next line.

dtb/src/reader.rs

Lines 361 to 362 in caf8d1e

let ptr = blob.as_ptr().add(header.reserved_mem_offset as usize)
as *const ReservedMemEntry;

In Reader::get_reserved_mem(), the code at line 361 would cast blob to ReservedMemEntry which was u64. It created a misaligned pointer and was passed to from_raw_parts as parameter.

dtb/src/writer.rs

Lines 37 to 38 in caf8d1e

&mut *(self.buf.as_mut_ptr().add(self.offset)
as *mut ReservedMemEntry)

In ReservedMem::add_entry(), the code at line 37 would cast buf which was u8 to ReservedMemEntry which was u64. The misaligned pointer was deref at the same line.

@ababo
Copy link
Owner

ababo commented Aug 8, 2023

This code was written by me just after starting learning the language. Unfortunately I don't have capacity to fix the bugs at the moment. Feel free to submit PR if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants