Put shared buffer into UnsafeCell #185

torfmaster · 2020-05-23T15:10:45Z

Summary

This PR attempts to solve #129: Currently shared memory contains a mutable reference to a buffer which can be mutated by the kernel. This violates the pointer aliasing rules and can lead to undefined behavior. To circumvent this we replace this buffer by an UnsafeCell solving this issue.

Limitations

The following aspects are not touched by this PR:

the shared buffer could be moved on the stack invalidating the kernels mutable reference to the buffer.
the app could mem::forget the kernel without calling drop and reuse the memory still shared with the kernel (see Subscriptions and shared memory have a loophole which can lead to UB #143).

Both issues could be solved by taking one of the approaches proposed in #143, however this is not the content of this PR (and an appropriate solution has not yet been chosen).

Woyten · 2020-05-26T17:14:44Z

core/src/shared_memory.rs

    driver_number: usize,
    allow_number: usize,
-    buffer_to_share: &'a mut [u8],
+    buffer_to_share: UnsafeCell<T>,


I think this is wrong! Instead of enforcing to pass an array reference which would pin the referenced array it is now possible to pass an entire array. This should result in UB once you move the SharedMemory object around.

Why not change the signature to UnsafeCell<&'a mut [u8]>?

Good point, I fixed this.

Woyten · 2020-05-26T17:18:25Z

core/src/shared_memory.rs

-    pub fn write_bytes<T: AsRef<[u8]>>(&mut self, source: T) {
-        safe_copy(source.as_ref(), self.buffer_to_share);
+    pub fn write_bytes<S: AsRef<[u8]>>(&mut self, source: S) {
+        let buf = unsafe { (*self.buffer_to_share.get()).as_mut() };


Why not delegate this to operate_on_mut_ptr?

Woyten · 2020-05-26T17:23:28Z

core/src/shared_memory.rs

+        safe_copy(source.as_ref(), buf);
+    }
+
+    pub(crate) unsafe fn operate_on_mut_ptr<R: Sized, F: FnOnce(*mut u8) -> R>(


Couldn't this be transformed to a safe function where F: FnOnce(&mut u8) ->R?

Shouldn't R: Sized be implicit?

Woyten · 2020-05-29T08:27:22Z

src/adc.rs

@@ -45,6 +45,12 @@ pub struct AdcBuffer {
    buffer: [u8; BUFFER_SIZE],
 }

+impl AsMut<[u8]> for AdcBuffer {


You no longer need this.

I would prefer if all AsRef and AsMut implementations were removed until we see the need for them.

jrvanwhy · 2020-05-29T22:55:16Z

&UnsafeCell is still dereferenceable, so the UB persists with this design. Instead, we need to replace all references to the buffer with raw pointers. The following is an example of how that could be done:

EDIT: "derefenceable" in this post refers to LLVM's dereferenceable attribute: https://llvm.org/docs/LangRef.html#dereferenceable-metadata

    // We cannot store a slice or reference, as all references in Rust are
    // dereferencable and allow regions cannot be (we can't have the compiler
    // insert non-volatile reads or writes). Instead store it in raw and operate
    // entirely with raw pointers.
    pub struct AllowRegion<T> {
        data: core::ptr::NonNull<T>,
        len: usize,
    }

    impl<T> AllowRegion<T> {
        pub fn get(&self, idx: usize) -> Option<T> {
            if idx >= self.len { return None; }
            Some(unsafe { self.data.as_ptr().add(idx).read_volatile() })
        }

        pub fn set(&self, idx: usize, value: T) {
            if idx < self.len {
                unsafe { self.data.as_ptr().add(idx).write_volatile(value); }
            }
        }
    }


    // We take in the allow region as an &mut reference to ensure this is the
    // only reference alive to the region. To prevent the compiler from
    // accessing the region any more (apart from our manual *_volatile() calls),
    // we break the passed slice into raw parts and destroy all references to
    // it.
    pub fn allow<T>(major: usize, minor: usize, region: &'static mut [T]) -> AllowRegion<T> {
        // Split region into its parts (raw data + length), then drop it. This
        // prevents the compiler from touching that memory, apart from our
        // *_volatile calls.
        let data = unsafe { core::ptr::NonNull::new_unchecked(region.as_mut_ptr()) };
        let len = region.len();
        drop(region);
        // TODO: Do the raw allow here. Note that even if the allow failed, we cannot be
        // sure the capsule didn't keep a copy of it, so we cannot return the
        // region reference. However, we can still return an AllowRegion, even
        // if the grant failed, so perhaps should return a tuple of AllowRegion
        // and a success/error code.

        // Return the new allow region.
        AllowRegion { data, len }
    }

torfmaster · 2020-06-05T14:52:09Z

@jrvanwhy I have issues understanding your comment.

&UnsafeCell is still dereferenceable, so the UB persists with this design.

But it is unsafe.

Instead, we need to replace all references to the buffer with raw pointers.

raw pointers are also dereferenceable but this is unsafe.

I have issues understanding your code snippet. In my eyes AllowRegion::get should be unsafe as it dereferences arbitrary pointers. But the whole point of using an AllowRegion should be sharing memory safely (i.e. in absence of aliasing issues).

jrvanwhy · 2020-06-05T19:51:13Z

I should clarify: when I said "dereferenceable" in my previous comment, I was referring to LLVM's dereferenceable attribute: https://llvm.org/docs/LangRef.html#dereferenceable-metadata. In Rust, references (including references to UnsafeCell) are deferenceable but raw pointers are not.

torfmaster · 2020-06-08T12:37:31Z

when I said "dereferenceable" in my previous comment, I was referring to LLVM's dereferenceable attribute:

I don't understand how this exactly relates to the issue this PR is aiming to solve and from what I understand the problem you are trying to solve is beyond the aliasing rules of Rust in particular and probably also beyond the safety guarantuees of Rust in general.

Your proposal (from the code snippet) is about implementing an alternative to UnsafeCell and requires a lot more effort to be really a safe implementation of shared memory (e.g. it requires phantom lifetimes).

My proposal is: Let us merge this PR once it improves the library safety-wise. Open an issue (or more...) describing the exact problem the implementation still has and let us develop a PR fixing this particular problem after this PR is merged.

jrvanwhy · 2020-06-09T00:39:37Z

when I said "dereferenceable" in my previous comment, I was referring to LLVM's dereferenceable attribute:

I don't understand how this exactly relates to the issue this PR is aiming to solve

It is undefined behavior to have a dereferenceable (in the LLVM sense) pointer to memory that is modified by an unknown-to-LLVM entity (such as the kernel). Therefore, it is important that the pointer type used to refer to the shared memory is not dereferenceable. There is a lot of background on this in the following issue and other issues it links to: rust-lang/unsafe-code-guidelines#33.

My proposal is: Let us merge this PR once it improves the library safety-wise.

This PR has no effect on the library's memory safety properties. As-is, it does not remove any source of undefined behavior.

torfmaster · 2020-06-09T12:38:23Z

Thank you for the reference to the issue (#33), I didn't know it and I am happy to learn about that from you.

This PR has no effect on the library's memory safety properties. As-is, it does not remove any source of undefined behavior.

This depends on the perspective, still. From the perspective of what is currently (i.e. merged to master) consensus (i.e. https://doc.rust-lang.org/stable/reference/behavior-considered-undefined.html) this PR removes a source of UB.

However, I still would recommend to not do your suggested change in this PR, for two reasons:

you are referring to an ongoing discussion about the memory model of Rust which has not come to an end (and didn't have any effect on the official Rust reference so far)
re-implementing something safer than UnsafeCell requires some care and I would like to do that change in isolation. Eventually, such a TockCell would probably make sense for us.

I would at least consider https://github.com/rust-embedded/wg/pull/387/files#diff-deb4784915d448685e0ca924234ecc03R476, i.e. waiting until the issues with UnsafeCell/VolatileCell get fixed upstream.

jrvanwhy · 2020-06-09T16:12:40Z

This PR has no effect on the library's memory safety properties. As-is, it does not remove any source of undefined behavior.

This depends on the perspective, still. From the perspective of what is currently (i.e. merged to master) consensus (i.e. https://doc.rust-lang.org/stable/reference/behavior-considered-undefined.html) this PR removes a source of UB.

Ah, I didn't think about that document. I agree, this PR does remove a documented source of UB.

torfmaster · 2020-06-17T09:10:55Z

Ah, I didn't think about that document. I agree, this PR does remove a documented source of UB.

@jrvanwhy So would it make sense to merge this PR in your eyes?

jrvanwhy · 2020-06-17T15:34:10Z

Ah, I didn't think about that document. I agree, this PR does remove a documented source of UB.

@jrvanwhy So would it make sense to merge this PR in your eyes?

I think so. That said, I expect to refactor the syscall/platform code soon. My current TODO list is as follows:

Add the QEMU test invocation to make test #199
Add code size tooling to libtock-rs
Refactor syscall and platform layer (goals: remove UB, better testability)

Note that license header tooling may become my top priority (based on past core WG meetings) somewhere in there, but I don't expect that to take too long.

torfmaster · 2020-06-18T16:22:16Z

So as this PR has two approvals can someone do a "bors r+", please?

Woyten · 2020-06-24T13:58:50Z

Could you remove the AsMuts before I bors r+?

Woyten · 2020-06-24T14:00:04Z

src/adc.rs

        syscalls::allow(DRIVER_NUMBER, allow_nr::BUFFER, &mut buffer.buffer).map_err(Into::into)
    }

-    pub fn init_alt_buffer(&self, alt_buffer: &'a mut AdcBuffer) -> TockResult<SharedMemory> {
+    pub fn init_alt_buffer(&self, alt_buffer: &'a mut AdcBuffer) -> TockResult<SharedMemory<'a>> {


Could you revert this?

Woyten · 2020-06-24T14:00:18Z

src/hmac.rs

@@ -87,15 +123,15 @@ pub struct HmacDriver<'a> {
 }

 impl<'a> HmacDriver<'a> {
-    pub fn init_key_buffer(&self, buffer: &'a mut HmacKeyBuffer) -> TockResult<SharedMemory> {
+    pub fn init_key_buffer(&self, buffer: &'a mut HmacKeyBuffer) -> TockResult<SharedMemory<'a>> {


Woyten · 2020-06-24T14:03:47Z

src/simple_ble.rs

    }
-    pub fn initialize<'a>(
+    pub fn initialize<'a, 'b>(


Do we need a second lifetime here?

Woyten · 2020-06-24T14:04:17Z

src/simple_ble.rs


-const EMPTY_SCAN_BUFFER: ScanBuffer = [0; BUFFER_SIZE_SCAN];
+const EMPTY_SCAN_BUFFER: ScanBuffer = ScanBuffer([0; BUFFER_SIZE_SCAN]);


Woyten · 2020-06-24T14:05:44Z

src/simple_ble.rs

-    pub fn create_advertising_buffer() -> [u8; BUFFER_SIZE_ADVERTISE] {
-        [0; BUFFER_SIZE_ADVERTISE]
+    pub fn create_advertising_buffer() -> BleAdvertisingBuffer {
+        BleAdvertisingBuffer([0; BUFFER_SIZE_ADVERTISE])


Revert this?

jrvanwhy · 2022-02-09T02:01:23Z

The Tock 1.0 libtock-rs crates have been removed from the repository, so this PR is obsolete. The Tock 2.0 libtock-rs crates have a substantially different Read-Write Allow interface, which does not have the soundness issue that this PR was created to address.

Put shared buffer into UnsafeCell

d8a7afd

torfmaster force-pushed the feature/shared-memory branch from c5c8267 to d8a7afd Compare May 23, 2020 15:30

alistair23 approved these changes May 26, 2020

View reviewed changes

Woyten suggested changes May 26, 2020

View reviewed changes

torfmaster added 3 commits May 27, 2020 20:25

Use reference to shared memory to prevent moving on stack

6928822

Improve operate on mut function

517673a

Unify operations on mut buffer

c1cc828

Woyten reviewed May 29, 2020

View reviewed changes

Woyten approved these changes Jun 6, 2020

View reviewed changes

Woyten suggested changes Jun 24, 2020

View reviewed changes

Woyten reviewed Jun 24, 2020

View reviewed changes

src/simple_ble.rs

}

pub fn initialize<'a>(

pub fn initialize<'a, 'b>(

Copy link

Contributor

Woyten Jun 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a second lifetime here?

Woyten reviewed Jun 24, 2020

View reviewed changes

jrvanwhy closed this Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put shared buffer into UnsafeCell #185

Put shared buffer into UnsafeCell #185

torfmaster commented May 23, 2020

Woyten May 26, 2020

torfmaster May 27, 2020

Woyten May 26, 2020

torfmaster May 27, 2020

Woyten May 26, 2020

torfmaster May 27, 2020

Woyten May 29, 2020

Woyten May 29, 2020

jrvanwhy commented May 29, 2020 •

edited

Loading

torfmaster commented Jun 5, 2020 •

edited

Loading

jrvanwhy commented Jun 5, 2020

torfmaster commented Jun 8, 2020

jrvanwhy commented Jun 9, 2020

torfmaster commented Jun 9, 2020

jrvanwhy commented Jun 9, 2020

torfmaster commented Jun 17, 2020

jrvanwhy commented Jun 17, 2020

torfmaster commented Jun 18, 2020

Woyten commented Jun 24, 2020

Woyten Jun 24, 2020

Woyten Jun 24, 2020

Woyten Jun 24, 2020

Woyten Jun 24, 2020

Woyten Jun 24, 2020

jrvanwhy commented Feb 9, 2022


		const EMPTY_SCAN_BUFFER: ScanBuffer = [0; BUFFER_SIZE_SCAN];
		const EMPTY_SCAN_BUFFER: ScanBuffer = ScanBuffer([0; BUFFER_SIZE_SCAN]);

Put shared buffer into UnsafeCell #185

Put shared buffer into UnsafeCell #185

Conversation

torfmaster commented May 23, 2020

Summary

Limitations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrvanwhy commented May 29, 2020 • edited Loading

torfmaster commented Jun 5, 2020 • edited Loading

jrvanwhy commented Jun 5, 2020

torfmaster commented Jun 8, 2020

jrvanwhy commented Jun 9, 2020

torfmaster commented Jun 9, 2020

jrvanwhy commented Jun 9, 2020

torfmaster commented Jun 17, 2020

jrvanwhy commented Jun 17, 2020

torfmaster commented Jun 18, 2020

Woyten commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrvanwhy commented Feb 9, 2022

jrvanwhy commented May 29, 2020 •

edited

Loading

torfmaster commented Jun 5, 2020 •

edited

Loading