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

Misaligned pointer dereference in chunk_entries_at and transmute_chunk #223

Closed
shinmao opened this issue Sep 26, 2023 · 6 comments · Fixed by #225
Closed

Misaligned pointer dereference in chunk_entries_at and transmute_chunk #223

shinmao opened this issue Sep 26, 2023 · 6 comments · Fixed by #225

Comments

@shinmao
Copy link

shinmao commented Sep 26, 2023

The source of unsoundness

Hi, we consider that the following two functions could have misaligned pointer dereference and lead to UB:

parity-db/src/index.rs

Lines 255 to 258 in 4ac2aca

fn chunk_entries_at(index: u64, map: &memmap2::MmapMut) -> Result<&[Entry; CHUNK_ENTRIES]> {
let offset = META_SIZE + index as usize * CHUNK_LEN;
let ptr = unsafe {
&*(map[offset..offset + CHUNK_LEN].as_ptr() as *const [Entry; CHUNK_ENTRIES])

At line 258 of chunk_entries_at,

parity-db/src/index.rs

Lines 418 to 422 in 4ac2aca

fn transmute_chunk(chunk: [u8; CHUNK_LEN]) -> [Entry; CHUNK_ENTRIES] {
let mut result: [Entry; CHUNK_ENTRIES] = unsafe { std::mem::transmute(chunk) };
if !cfg!(target_endian = "little") {
for item in result.iter_mut() {
*item = Entry::from_u64(u64::from_le(item.0));

and at line 419 of transmute_chunk,
they both tried to convert the type aligned to 1 byte to the type aligned to 8 bytes. Please check and would be happy to have any discussion:)

@arkpar
Copy link
Member

arkpar commented Oct 5, 2023

Thanks for the report

Hi, we consider that the following two functions could have misaligned pointer dereference and lead to UB:
parity-db/src/index.rs

Here the offset is guaranteed to point to a 512-byte aligned page.

parity-db/src/index.rs

This one looks technically unsound ineed since chunk maybe moved on to the stack. The function is marked #[inline(always)] but there's no strict guarantee that the stack value will be eliminated. Needs to be fixed.

@shinmao
Copy link
Author

shinmao commented Oct 5, 2023

Hi @arkpar , sorry that I didn't see the your policy of reporting bugs. Do you think we can submit to RUSTSEC or GitHub Security Advisories Database?

@arkpar
Copy link
Member

arkpar commented Oct 5, 2023

I don't think this is a security issue. It's more about formal source code correctness. The compiler still generates correct machine code here and I can't think of a way to exploit this. I guess it is technically an unsoundness, but not the one exposed in the API and not the one causing any real issues.

@shinmao
Copy link
Author

shinmao commented Oct 5, 2023

Yes. @arkpar , that's the unsoundness issue from the perspective of Rust's language model. That's what RUSTSEC collects for unsoundness issues.

@shinmao
Copy link
Author

shinmao commented Oct 5, 2023

@arkpar just for discussion, unaligned access could lead to runtime panic depends on the target machine. Isn't that kind of security issue leading to Denial of Service? 🤔️ At least runtime panic could crash the program.

@arkpar
Copy link
Member

arkpar commented Oct 5, 2023

unaligned access could lead to runtime panic depends on the target machine.

Sure it can, but I'm arguing that unaligned access can't happen here in practice. In paractice the compiler generates code that passes the pointer without moving the data to the stack and that pointer is guaranteed to a be aligned. I don't think anyone can write an actual program for any supported platform using this crate, that can panic because of this issue. But I'd be happy to be proven wrong if you can demonstrate it.

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

Successfully merging a pull request may close this issue.

2 participants