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

allow() is unsound #129

Closed
jrvanwhy opened this issue Jan 7, 2020 · 9 comments
Closed

allow() is unsound #129

jrvanwhy opened this issue Jan 7, 2020 · 9 comments
Assignees
Labels

Comments

@jrvanwhy
Copy link
Collaborator

jrvanwhy commented Jan 7, 2020

SharedMemory instances returned by allow() contain a reference to a buffer that may be mutated by the kernel. This is undefined behavior in Rust.

It's probably not possible to expose a safe API that allows an application to access an allow()-ed buffer until that buffer has been taken back from the kernel (i.e. overwritten with another allow() call).

@jrvanwhy jrvanwhy added the bug label Jan 7, 2020
@jrvanwhy jrvanwhy self-assigned this Jan 7, 2020
@torfmaster
Copy link
Collaborator

I'm not sure about that. The buffer is private and is never exposed directly. Instead, the only way to obtain data from the buffer is to copy data from/to it. When SharedMemory goes out of scope the kernel is told to not use the memory anymore (this is a change in the kernel we introduced when we created SharedMemory).

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Jan 7, 2020

It is undefined behavior whenever all of the following occur simultaneously:

  1. &mut [T] exists
  2. One of the T values in the slice is mutated through a mechanism other than the slice reference.

In this case, SharedMemory satisfies 1 and the kernel mutates it through an independent reference.

@torfmaster
Copy link
Collaborator

Maybe I am missing a point here, but from my naive point of view this would imply that MMIO and DMA is undefined behavior in general. Maybe you need some non-atomicity assumption here?

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Jan 7, 2020

If I'm reading the nomicon correctly, MMIO works if you use shared references to types containing UnsafeCell (and use volatile reads/writes).

@Woyten
Copy link
Contributor

Woyten commented Jan 7, 2020

Yes, it would probably be more correct to store an UnsafeCell or a *mut internally. But this can be achieved without declaring allow to be unsafe.

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Jan 7, 2020

I'm not suggesting we mark allow unsafe, but we should change its API to a sound API.

@jrvanwhy jrvanwhy changed the title allow() is unsafe allow() is unsound Jan 8, 2020
@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Jan 8, 2020

Ah, I see I put "unsafe" rather than "unsound" in the title. It should have been "unsound".

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Jun 9, 2020

This issue is actually worse than my original description states: it is not currently possible to un-allow a buffer shared with the kernel. See tock/tock#1831 for background.

Note that Tock's design here may change in the future to make un-allow work reliably: tock/tock#1905

jrvanwhy added a commit to jrvanwhy/libtock-rs that referenced this issue Jul 13, 2020
Allowed is the type that will be returned by Platform::allow() in the successful case. It represents a memory buffer shared with the kernel. It uses raw pointers and volatile writes to perform read/write accesses to the memory to avoid encountering undefined behavior.

This is the first step towards fixing tock#129 and tock#143.
jrvanwhy added a commit to jrvanwhy/libtock-rs that referenced this issue Jul 17, 2020
Allowed is the type that will be returned by Platform::allow() in the successful case. It represents a memory buffer shared with the kernel. It uses raw pointers and volatile writes to perform read/write accesses to the memory to avoid encountering undefined behavior.

This is the first step towards fixing tock#129 and tock#143.
bors bot added a commit that referenced this issue Aug 5, 2020
222: Add the Allowed type to libtock_platform. r=hudson-ayers a=jrvanwhy

Allowed is the type that will be returned by `Platform::allow()` in the successful case. It represents a memory buffer shared with the kernel. It uses raw pointers and volatile writes to perform read/write accesses to the memory to avoid encountering undefined behavior.

This is the first step towards fixing #129 and #143.

Co-authored-by: Johnathan Van Why <jrvanwhy@google.com>
@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Feb 9, 2022

Solved in the Tock 2.0 ABI design and libtock-rs crates.

@jrvanwhy jrvanwhy closed this as completed Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants