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

Update rust-toolchain to nightly of 2023-07-30 #481

Closed

Conversation

lschuermann
Copy link
Member

This version and the time to update is chosen somewhat arbitrarily, as libtock-rs fails to build elf2tab on its current Rust toolchain (2022-06-10).

This version and the time to update is chosen somewhat arbitrarily, as
libtock-rs fails to build elf2tab on its current Rust toolchain
(2022-06-10).
alistair23
alistair23 previously approved these changes Jul 31, 2023
@lschuermann
Copy link
Member Author

lschuermann commented Jul 31, 2023

Okay, I think I hit a roadblock. After the update, Miri fails with the following:

error: unsupported operation: integer-to-pointer casts and `ptr::from_exposed_addr` are not supported with `-Zmiri-strict-provenance`
   --> /home/runner/work/libtock-rs/libtock-rs/platform/src/register.rs:25:18
    |
25  |         Register(value as *mut ())
    |                  ^^^^^^^^^^^^^^^^ integer-to-pointer casts and `ptr::from_exposed_addr` are not supported with `-Zmiri-strict-provenance`
    |
    = help: use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead
    = note: BACKTRACE:
    = note: inside `<libtock_platform::Register as core::convert::From<u32>>::from` at /home/runner/work/libtock-rs/libtock-rs/platform/src/register.rs:25:18: 25:34
    = note: inside `<u32 as core::convert::Into<libtock_platform::Register>>::into` at /home/runner/.rustup/toolchains/nightly-2023-07-30-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/convert/mod.rs:716:9: 716:22
note: inside `libtock_platform::syscalls_impl::<impl libtock_platform::Syscalls for libtock_unittest::fake::Syscalls>::command`

Having read the linked strict provenance APIs documentation, I can understand why this is an error. However, I don't really know how to work around it. As far as I see it, we're using the Register in raw_syscalls as a means to provide Miri with the ability to track pointers as they are passed across the fake system call interface boundaries. By using raw pointers instead of usize integers, those pointers can carry an additional tag which Miri uses for its provenance checks. This is not actually used at runtime in a compiled Rust program, as carrying provenance across the asynchronous system call boundary (such as in the case of Tock's upcalls) wouldn't work anyways -- we're fundamentally limited to a usize-sized type here. So far, so good.

The root cause of the issues we're facing here is that we can't create a raw pointer type from a "tagless" usize integer. Semantically, accessing such a pointer does not make sense with provenance analysis. Miri can't know that we're using this pointer solely as a usize-sized (which is actually not necessarily true on all platforms, such as Miri) type to hold an integer in its address field, only to extract this integer back later. We never actually dereference this pointer.

From what I understand, Miri doesn't leave us with too many options here, assuming we want to stick to using strict provenance checks:

  • We can change the definition of our Register type to contain either an integer or a pointer:

     pub enum Register {
         Pointer(*mut ()),
         Integer(usize),
     }

    Unfortunately, this uses another word of memory to store the enum's tag, which we really only need to keep around for Miri. On platforms where core::mem::size_of::<*mut ()>() == core::mem::size_of::<usize>() (which platforms are all that Tock targets, and Tock further requires by means of its system call ABI), we should not have to incur this cost. It is further impossible to reconstruct the enum tag from a raw register value as passed across the system call ABI, e.g. for an upcall.

  • We can construct a raw pointer from an integer by taking the "provenance" (Miri's tag) from another invented pointer, e.g. core::ptr::null(). While this seems like a hack, it might actually give us some useful properties: by using the provenance of some invented, zero-sized raw pointer for the "integer pseudo-pointers", Miri should complain about any dereferences or pointer-arithmetic on such pointers. This would effectively enforce that we use these pointers only for retaining their addresses, and the only allowed operation would be extracting their address.

I'll prototype the second approach for now, and will see whether I can work around the Miri issues this way.

@lschuermann
Copy link
Member Author

My "fix" seems to work, but it does prevent compilation on stable Rust. As far as I know, the only way to have this work on both stable (for compiling) and unstable (for testing with Miri) is to use conditional compilation. If we need to use conditional compilation anyways, it might make sense to go with the enum-route instead for Miri builds, and use the previous raw pointer-based approach for everything else.

lschuermann pushed a commit to lschuermann/libtock-rs that referenced this pull request Aug 1, 2023
This updates `libtock-rs` to a relatively old Rust toolchain, which
happens to ship a rustc v1.64.0-nightly such that the latest `elf2tab`
release can build with it (v0.11.0). However, it's not too recent for
Miri to complain about unsupported integer-to-pointer casts in
strict-provenance mode:

    running 5 tests
    test tests::driver_check ... error: unsupported operation: integer-to-pointer casts and `ptr::from_exposed_addr` are not supported with `-Zmiri-strict-provenance`
       --> /home/leons/dev/libtock-rs/platform/src/register.rs:25:18
        |
    25  |         Register(value as *mut ())
        |                  ^^^^^^^^^^^^^^^^ integer-to-pointer casts and `ptr::from_exposed_addr` are not supported with `-Zmiri-strict-provenance`
        |
        = help: use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead
        = note: backtrace:
        = note: inside `<libtock_platform::Register as core::convert::From<u32>>::from` at /home/leons/dev/libtock-rs/platform/src/register.rs:25:18
        = note: inside `<u32 as core::convert::Into<libtock_platform::Register>>::into` at /home/leons/.rustup/toolchains/nightly-2022-08-02-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/convert/mod.rs:550:9

This serves as a bridge-update to get CI running again, until a proper
fix for the above Miri error is integrated (see [1]).

[1]: tock#481
@jrvanwhy
Copy link
Collaborator

jrvanwhy commented Aug 8, 2023

I wouldn't rule out the possibility of this code running on a platform where size_of::<*mut ()> != size_of::<usize>().

I think the most standard/idiomatic fix here is to use sptr::invalid. sptr exists primarily to allow Rust code to work on both stable and under strict provenance rules, which is the problem we're facing.

If you really want to avoid adding another dependency, you could try copying the implementation from sptr, which is just a transmute:

    // FIXME(strict_provenance_magic): I am magic and should be a compiler intrinsic.
    // We use transmute rather than a cast so tools like Miri can tell that this
    // is *not* the same as from_exposed_addr.
    // SAFETY: every valid integer is also a valid pointer (as long as you don't dereference that
    // pointer).
    #[cfg(miri)]
    return unsafe { core::mem::transmute(addr) };

Note: we don't need conditional compilation here because we don't need const.

However, I'm not 100% sure that'll work, because it's possible that Miri special-cases the sptr crate.

github-merge-queue bot pushed a commit that referenced this pull request Aug 22, 2023
I'm preparing a toolchain update PR that will supercede #481. I'd prefer to avoid sending a large PR that mixes these style updates with important `unsafe` code, so I'm splitting the update into two PRs.
@lschuermann
Copy link
Member Author

Closing in favor of #511, thanks @jrvanwhy!

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 this pull request may close these issues.

3 participants