[PATCH 0/1] rust: sync: Make `GlobalLockedBy::as_ref` unavailable if T is not Sync

[Priya Bala Govindasamy <pgovind2@uci.edu>]

Dear Linux kernel maintainers,

We are developing a tool called FerroLens to detect potential
unsound behavior in Rust code in the Linux kernel. The tool
internally uses an LLM to detect bugs.
We then perform manual analysis to verify these reports.
FerroLens reported the following bug in rust/kernel/sync/lock/global.rs:

The `GlobalLockedBy::as_ref` method allows `&T` to be obtained from 
mutliple threads even if `T` is not Sync. 

This happens because `GlobalLockedBy<T, B>` is Sync if 
`LockedBy<T, B::item>` is Sync. `LockedBy<T, B>` is Sync if `T` is Send. 
Since there is no requirement that `T` also implements Sync, it is 
possible for multiple threads to simultaneously obtain references to a 
`GlobalLockedBy<T, B>` and then acquire `&T` through 
`GlobalLockedBy::as_ref`. This can lead to undefined behavior if `T` does 
not implement Sync.

Note that a reference to `GlobalGuard<B>` is required as an argument to 
`GlobalLockedBy::as_ref`. Therefore, this issue requires that 
`GlobalGuard<B>` is `Sync` which happens if `B::item` is `Sync` 
(for example, ()).

Here is a PoC kernel module demonstrating the issue.

    use core::cell::Cell;
    use kernel::{
        bindings, c_str,
        error::from_err_ptr,
        prelude::*,
        sync::{Completion, GlobalGuard, GlobalLockedBy},
    };
    use pin_init::stack_pin_init;

    module! {
        type: LockedByPoc,
        name: "locked_by_poc",
        authors: ["Priya Govindasamy"],
        description: "PoC for GlobalLockedBy::as_ref on a non-Sync value",
        license: "GPL",
    }

    kernel::sync::global_lock! {
        // SAFETY: Initialized exactly once in `LockedByPoc::init`, before use.
        unsafe(uninit) static POC_LOCK: Mutex<()> = ();
    }

    struct LockedByPoc;

    // `Cell<u32>` is `Send` but not `Sync`, so this type is also `Send` but not `Sync`.
    struct SendButNotSync {
        value: Cell<u32>,
    }

    fn assert_sync<T: Sync>() {}

    #[pin_data]
    struct ThreadContext<'a> {
        protected: &'a GlobalLockedBy<SendButNotSync, POC_LOCK>,
        guard: &'a GlobalGuard<POC_LOCK>,
        thread_id: u32,
        #[pin]
        done: Completion,
    }

    unsafe extern "C" fn thread_main(data: *mut kernel::ffi::c_void) -> i32 {
        // SAFETY: `data` points to a pinned `ThreadContext` in `init`. `init` waits for this thread
        // to run and then joins it before dropping the context or either borrowed reference.
        let ctx = unsafe { &*data.cast::<ThreadContext<'_>>() };

        // This is accepted despite `SendButNotSync: !Sync`. 
        let value = ctx.protected.as_ref(ctx.guard);
        pr_info!(
            "thread {} acquired &SendButNotSync through GlobalLockedBy::as_ref (value={})\n",
            ctx.thread_id,
            value.value.get()
        );
        ctx.done.complete_all();
        0
    }

    fn spawn_thread(ctx: &ThreadContext<'_>) -> Result<*mut bindings::task_struct> {
        let task = match from_err_ptr(unsafe {
            bindings::kthread_create_on_node(
                Some(thread_main),
                (ctx as *const ThreadContext<'_>).cast_mut().cast(),
                bindings::NUMA_NO_NODE,
                c_str!("locked_by_thread/%u").as_char_ptr(),
                ctx.thread_id,
            )
        }) {
            Ok(task) => task,
            Err(err) => return Err(err),
        };

        // SAFETY: `task` is the sleeping kthread returned by `kthread_create_on_node`.
        unsafe { bindings::wake_up_process(task) };
        Ok(task)
    }

    impl kernel::Module for LockedByPoc {
        fn init(_module: &'static ThisModule) -> Result<Self> {
            // SAFETY: Module initialization executes once for this static global lock.
            unsafe { POC_LOCK.init() };

            // `SendButNotSync` is not `Sync`, but this assertion currently compiles because
            // `GlobalLockedBy` only requires its protected value to be `Send` to implement `Sync`.
            assert_sync::<GlobalLockedBy<SendButNotSync, POC_LOCK>>();
            pr_info!("GlobalLockedBy<SendButNotSync, POC_LOCK> is Sync\n");
            assert_sync::<GlobalGuard<POC_LOCK>>();
            pr_info!("GlobalGuard<POC_LOCK> is Sync\n");

            let protected = GlobalLockedBy::<SendButNotSync, POC_LOCK>::new(SendButNotSync {
                value: Cell::new(0),
            });
            let guard = POC_LOCK.lock();
            let protected_ref = &protected;
            let guard_ref = &guard;

            stack_pin_init!(let thread_a = pin_init!(ThreadContext {
                protected: protected_ref,
                guard: guard_ref,
                thread_id: 1,
                done <- Completion::new(),
            }));
            stack_pin_init!(let thread_b = pin_init!(ThreadContext {
                protected: protected_ref,
                guard: guard_ref,
                thread_id: 2,
                done <- Completion::new(),
            }));

            let task_a = spawn_thread(thread_a.as_ref().get_ref())?;
            let task_b = match spawn_thread(thread_b.as_ref().get_ref()) {
                Ok(task) => task,
                Err(err) => {
                    thread_a.done.wait_for_completion();
                    // SAFETY: The callback has already run; this joins it before its context dies.
                    unsafe { bindings::kthread_stop(task_a) };
                    return Err(err);
                }
            };

            // Each callback prints before signalling. Wait for both callbacks to run, then join
            // their kthreads before the stack contexts or their borrowed values are dropped.
            thread_a.done.wait_for_completion();
            thread_b.done.wait_for_completion();
            unsafe {
                bindings::kthread_stop(task_a);
                bindings::kthread_stop(task_b);
            }

            Ok(Self)
        }
    }

    impl Drop for LockedByPoc {
        fn drop(&mut self) {
            pr_info!("locked_by_poc unloaded\n");
        }
    }


Output:
[91306.702534] locked_by_poc: GlobalLockedBy<SendButNotSync, POC_LOCK> is Sync
[91306.702563] locked_by_poc: GlobalGuard<POC_LOCK> is Sync
[91306.706793] locked_by_poc: thread 1 acquired &SendButNotSync through GlobalLockedBy::as_ref (value=0)
[91306.707231] locked_by_poc: thread 2 acquired &SendButNotSync through GlobalLockedBy::as_ref (value=0)


Priya Bala Govindasamy (1):
  rust: sync: Make `GlobalLockedBy::as_ref` unavailable if T is not Sync

 rust/kernel/sync/lock/global.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.34.1