tokio::io::ReadHalf<T>::unsplit can violate Pin contract when T: !Unpin

[zachs18]

Version
From what I gather on docs.rs, tokio::io::ReadHalf::unsplit was introduced in tokio 0.1.12, so all versions since 0.1.12 are probably affected, but the example is tested with tokio 1.24.1.
Requires the io-util feature.

Platform
Not platform-specific.

Background
tokio::io::split allows splitting a T: AsyncRead + AsyncWrite into separate ReadHalf<T>: AsyncRead and WriteHalf<T>: AsyncWrite halves, each of which can be used separately. This is done by having ReadHalf<T>'s poll_read method call T's poll_read method, and WriteHalf<T>'s poll_write call T's poll_write method (with appropriate mutual exclusion). Note that both of these methods take self: Pin<&mut Self>, which is a guarantee that the T will not be moved before it is dropped (unless T: Unpin) (https://doc.rust-lang.org/std/pin/index.html#drop-guarantee). This is fine in absence of unsplit, since as a safety comment says (https://github.com/tokio-rs/tokio/blob/master/tokio/src/io/split.rs#L154), the T is stored in an Arc that is shared between the Readhalf<T> and WriteHalf<T>, and which is not deallocated or moved from until both the ReadHalf and WriteHalf are dropped (and thus the T is dropped).

Description
However, in the presence of ReadHalf::unsplit, this safety comment is incorrect: the T is not pinned, since the T may be moved out of the shared Arc by calling ReadHalf::unsplit (https://github.com/tokio-rs/tokio/blob/master/tokio/src/io/split.rs#L82), possibly after a Pin<&mut T> has been created by calling ReadHalf::poll_read or WriteHalf::poll_write, violating the Pin contract (if T: !Unpin).

A simple (but breaking-change) fix would be to add a where T: Unpin bound to ReadHalf::unsplit. (and perhaps to hold a Pin<Arc<Inner<T>>> instead of an Arc<Inner<T>> in ReadHalf and WriteHalf to communicate the requirement).

Note that there are not (to my knowledge) any concrete types in tokio by itself which are AsyncRead + AsyncWrite + !Unpin (File, TcpStream, and UnixStream are all Unpin, and the adapters all are Unpin if their underlying type is Unpin).

---

Following is (what I think is) an (admittedly somewhat dubious) minimal example of undefined behaviour (use-after-free) resulting from this violation of the Pin contract. Here is a playground link to the same example.

Example code

Cargo.toml:

[package]
name = "tokio-mwe"
version = "0.1.0"
edition = "2021"
[dependencies]
tokio = { version = "1.24.1", features = ["io-util", "rt", "macros"] }

src/main.rs

use std::{
    cell::Cell,
    marker::{PhantomData, PhantomPinned},
    pin::Pin,
    ptr::NonNull,
    sync::Arc,
    task::{Context, Poll},
};

use tokio::io::AsyncReadExt;

// align(2048) to make allocator collsions happen more easily
#[repr(align(2048))]
pub struct BadRW {
    value: Cell<usize>,
    // To ensure that `BadRw` cannot be moved after `poll_read` (via `Pin` contract).
    _pinned: PhantomPinned,
    // To ensure that `BadRw` will be dropped on the thread that created it (if it is dropped).
    _unsend: PhantomData<*const ()>,
}

thread_local! {
    // Points to the most recently polled `BadRW` on this thread, unless it has been dropped.
    static BAD_PTR: Cell<Option<NonNull<BadRW>>> = Cell::new(None);
}

impl Drop for BadRW {
    fn drop(&mut self) {
        // SAFETY: BAD_PTR, if not `None`, must point to a pinned `BadRW` on this thread,
        // so set it to None when dropping a `BadRW`.
        BAD_PTR.with(|tl| tl.set(None));
        eprintln!("{:p} (dropped)", self);
    }
}

impl tokio::io::AsyncRead for BadRW {
    fn poll_read(
        self: Pin<&mut Self>,
        _: &mut Context<'_>,
        _: &mut tokio::io::ReadBuf<'_>,
    ) -> Poll<std::io::Result<()>> {
        // SAFETY: We have a Pin<&mut Self>, so `*self` will not be moved until it is dropeed
        // (by the contract of `Pin`), at which point BadRW::drop will remove this pointer from BAD_PTR.
        // (BadRW is not `Send`, so `*self` will be dropped on this thread, if it is dropped).
        BAD_PTR.with(|tl| tl.set(Some(NonNull::from(&*self))));
        eprintln!("{:p} (pinned)", self);
        Poll::Ready(Ok(()))
    }
}

// Does nothing, only here so `tokio::io::split` can be called.
impl tokio::io::AsyncWrite for BadRW {
    fn poll_write(
        self: Pin<&mut Self>,
        _: &mut Context<'_>,
        _: &[u8],
    ) -> Poll<Result<usize, std::io::Error>> {
        std::task::Poll::Ready(Ok(0))
    }

    fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<Result<(), std::io::Error>> {
        std::task::Poll::Ready(Ok(()))
    }

    fn poll_shutdown(
        self: Pin<&mut Self>,
        _: &mut Context<'_>,
    ) -> Poll<Result<(), std::io::Error>> {
        std::task::Poll::Ready(Ok(()))
    }
}

async fn example_1() {
    let bad = BadRW {
        value: Cell::new(0),
        _pinned: PhantomPinned,
        _unsend: PhantomData,
    };
    let (mut rd, wr) = tokio::io::split(bad);

    let _ = rd.read_u8().await;
    let bad = rd.unsplit(wr);
    let ptr = BAD_PTR.with(|tl| tl.get().unwrap());

    // If `BAD_PTR` is not `None`, then it points to a `BadRW` which was pinned,
    // was polled, and has not yet been dropped, so this pointer should not point
    // to deallocated memory, but it does, because `unsplit` moved a `BadRW` after
    // it was pinned.
    eprintln!("Setting *BAD_PTR to 42");
    unsafe { ptr.as_ref() }.value.set(42);
    dbg!(bad.value.get());
}

async fn example_2() {
    let bad = BadRW {
        value: Cell::new(0),
        _pinned: PhantomPinned,
        _unsend: PhantomData,
    };
    let (mut rd, wr) = tokio::io::split(bad);

    let _ = rd.read_u8().await;
    let bad = rd.unsplit(wr);
    let ptr = BAD_PTR.with(|tl| tl.get().unwrap());

    let unrelated = Arc::new(BadRW {
        value: Cell::new(0),
        _pinned: PhantomPinned,
        _unsend: PhantomData,
    });
    eprintln!("{:p} (unrelated, non-pinned)", unrelated);

    // If `BAD_PTR` is not `None`, then it points to a `BadRW` which was pinned,
    // was polled, and has not yet been dropped, so this pointer should point
    // to that specific `BadRW`, but it doesn't, because `unsplit` moved a `BadRW`
    // after it was pinned, and another `BadRW` was (perhaps) allocated in its place.
    eprintln!("Setting *BAD_PTR to 42");
    unsafe { ptr.as_ref() }.value.set(42);
    dbg!(bad.value.get());
    dbg!(unrelated.value.get());
}

#[tokio::main(flavor = "current_thread")]
async fn main() {
    eprintln!("Example 1");
    example_1().await;
    eprintln!("\nExample 2");
    example_2().await;
}

In async fn example_1, BadRW allows writing to deallocated memory. In async fn example_2, BadRW allows writing to a different object than the one that was pinned (if the allocator re-uses the address of a particular previous allocation).

Following are the output of cargo run and MIRIFLAGS=-Zmiri-backtrace=full cargo +nightly miri run on my computer (only the first example gets run under miri, because it aborts. The second example isn't interesting under miri, since miri doesn't re-use the previous allocation anyway).

`cargo run` output

$ cargo run
   Compiling tokio-mwe v0.1.0 (/home/zachary/Programming/rusttesting/tokio-crimes)
    Finished dev [unoptimized + debuginfo] target(s) in 0.72s
     Running `target/debug/tokio-mwe`
Example 1
0x556871452000 (pinned)
Setting *BAD_PTR to 42
[src/main.rs:91] bad.value.get() = 0
0x7ffd1d452000 (dropped)

Example 2
0x556871454000 (pinned)
0x556871454000 (unrelated, non-pinned)
Setting *BAD_PTR to 42
[src/main.rs:119] bad.value.get() = 0
[src/main.rs:120] unrelated.value.get() = 42
0x556871454000 (dropped)
0x7ffd1d451000 (dropped)

miri output

$ MIRIFLAGS=-Zmiri-backtrace=full cargo +nightly miri run
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `/home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/tokio-mwe`
Example 1
0x4e800 (pinned)
Setting *BAD_PTR to 42
error: Undefined Behavior: pointer to alloc5497 was dereferenced after this allocation got freed
   --> /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:385:18
    |
385 |         unsafe { &*self.as_ptr() }
    |                  ^^^^^^^^^^^^^^^ pointer to alloc5497 was dereferenced after this allocation got freed
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `std::ptr::NonNull::<BadRW>::as_ref::<'_>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:385:18: 385:33
note: inside closure
   --> src/main.rs:90:14
    |
90  |     unsafe { ptr.as_ref() }.value.set(42);
    |              ^^^^^^^^^^^^
note: inside closure
   --> src/main.rs:126:16
    |
126 |     example_1().await;
    |                ^^^^^^
    = note: inside `<std::pin::Pin<&mut [async block@src/main.rs:123:1: 123:42]> as std::future::Future>::poll` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/future/future.rs:125:9: 125:61
    = note: inside closure at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/current_thread.rs:541:57: 541:86
    = note: inside `tokio::runtime::coop::with_budget::<std::task::Poll<()>, [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut [async block@src/main.rs:123:1: 123:42]>>::{closure#0}::{closure#0}::{closure#0}]>` at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/coop.rs:102:5: 102:8
    = note: inside `tokio::runtime::coop::budget::<std::task::Poll<()>, [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut [async block@src/main.rs:123:1: 123:42]>>::{closure#0}::{closure#0}::{closure#0}]>` at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/coop.rs:68:5: 68:38
    = note: inside closure at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/current_thread.rs:541:25: 541:87
    = note: inside `tokio::runtime::scheduler::current_thread::Context::enter::<std::task::Poll<()>, [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut [async block@src/main.rs:123:1: 123:42]>>::{closure#0}::{closure#0}]>` at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/current_thread.rs:350:19: 350:22
    = note: inside closure at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/current_thread.rs:540:36: 542:23
    = note: inside closure at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/current_thread.rs:615:57: 615:79
    = note: inside `tokio::macros::scoped_tls::ScopedKey::<tokio::runtime::scheduler::current_thread::Context>::set::<[closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::enter<[closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut [async block@src/main.rs:123:1: 123:42]>>::{closure#0}], std::option::Option<()>>::{closure#0}], (std::boxed::Box<tokio::runtime::scheduler::current_thread::Core>, std::option::Option<()>)>` at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/macros/scoped_tls.rs:61:9: 61:12
    = note: inside `tokio::runtime::scheduler::current_thread::CoreGuard::<'_>::enter::<[closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut [async block@src/main.rs:123:1: 123:42]>>::{closure#0}], std::option::Option<()>>` at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/current_thread.rs:615:27: 615:80
    = note: inside `tokio::runtime::scheduler::current_thread::CoreGuard::<'_>::block_on::<std::pin::Pin<&mut [async block@src/main.rs:123:1: 123:42]>>` at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/current_thread.rs:530:19: 594:11
    = note: inside `tokio::runtime::scheduler::current_thread::CurrentThread::block_on::<[async block@src/main.rs:123:1: 123:42]>` at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/scheduler/current_thread.rs:154:24: 154:45
    = note: inside `tokio::runtime::Runtime::block_on::<[async block@src/main.rs:123:1: 123:42]>` at /home/zachary/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.24.1/src/runtime/runtime.rs:282:47: 282:88
note: inside `main`
   --> src/main.rs:128:5
    |
128 |     example_2().await;
    |     ^^^^^^^^^^^^^^^^^^
    = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
    = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:121:18: 121:21
    = note: inside closure at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:166:18: 166:82
    = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:287:13: 287:31
    = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:483:40: 483:43
    = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:447:19: 447:81
    = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:140:14: 140:33
    = note: inside closure at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:148:48: 148:73
    = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:483:40: 483:43
    = note: inside `std::panicking::r#try::<isize, [closure@std::rt::lang_start_internal::{closure#2}]>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:447:19: 447:81
    = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:140:14: 140:33
    = note: inside `std::rt::lang_start_internal` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:148:20: 148:98
    = note: inside `std::rt::lang_start::<()>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:165:17: 170:6

error: aborting due to previous error

[carllerche]

Thanks for the report.

For context, @zachs18 initially reported this via our security process. We decided that it was not a security issue and asked for the report to be filed as an issue. The reasoning is that the specific set of conditions needed to trigger an issue (a !Unpin type in ReadHalf) is unusual, combined with the difficulty of making any arbitrary use-after-free exploitable in Rust without doing a lot of careful alignment of data types in the surrounding code.

We will address the issue here and backport fixes to the LTS branches.

[taiki-e]

Fixed in 1.18.6 (LTS), 1.20.4 (LTS), and 1.24.2 (latest).