[PATCH v2 0/1] rust: sync: Make Guard::lock_ref unavailable if Lock 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.rs.

Guard::lock_ref() returns a reference to the Lock stored in the Guard. 
Guard is Sync if T is Sync. However, Lock is Sync only if T is Send. 
This means that a reference to Guard can be sent across threads and 
multiple threads can acquire a reference to the Lock, 
even if Lock is not Sync. 

The following scenario is possible:
If T is sync but not send, 
Thread A: Creates a Lock<T>. Because T is not Send, Lock<T> is not Sync. 
It cannot be shared with other threads. 
Thread A: Locks the mutex and gets a Guard<T>. 
Since T is Sync, code says Guard<T> is Sync. 
Thread A can now safely give a &Guard<T> to Thread B.
Thread B: calls guard.lock_ref(). This returns a &'a Lock<T>.
Thread B now has a reference to the Lock. In this case, 
even after Thread A drops the guard, Thread B still has that &Lock. 
Thread B can now call lock.lock() and get a &mut T.
This is unsound.

Here is a poc kernel module which demonstrates this problem:

use core::sync::atomic::{AtomicBool, Ordering};
use kernel::{
    bindings, c_str,
    error::from_err_ptr,
    prelude::*,
    sync::{
        lock::{mutex::MutexBackend, Backend, Guard, Lock},
        new_mutex, Arc,
    },
    types::NotThreadSafe,
};
use pin_init::stack_pin_init;

module! {
    type: LockRefPoc,
    name: "lock_ref_poc",
    authors: ["Priya Govindasamy"],
    description: "Sample module demonstrating cross-thread Guard::lock_ref access",
    license: "GPL",
}

struct LockRefPoc;

struct SyncButNotSend {
    value: u32,
    _not_send: NotThreadSafe,
}

// SAFETY: Access to `value` is synchronized by the lock in this sample, and the marker field is
// only used to prevent `Send`.
unsafe impl Sync for SyncButNotSend {}

struct HandoffState {
    lock_ref_ready: AtomicBool,
    guard_dropped: AtomicBool,
    relock_done: AtomicBool,
}

struct ThreadBContext {
    guard_addr: usize,
    handoff: Arc<HandoffState>,
    thread_id: u32,
}

fn guard_matches_lock<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) -> bool {
    core::ptr::eq(guard.lock_ref(), lock)
}

unsafe extern "C" fn thread_b_main(data: *mut kernel::ffi::c_void) -> i32 {
    // SAFETY: `data` comes from `Arc::into_raw` in `spawn_thread_b`, and the thread takes back
    // ownership of exactly one reference.
    let ctx = unsafe { Arc::from_raw(data.cast::<ThreadBContext>()) };

    let guard: &Guard<'_, SyncButNotSend, MutexBackend> =
        // SAFETY: Thread A passes the address of a live guard to thread B.
        unsafe { &*(ctx.guard_addr as *const Guard<'_, SyncButNotSend, MutexBackend>) };

    let leaked_lock = guard.lock_ref();
    pr_info!(
        "thread B obtained &Lock<SyncButNotSend, _> through &Guard\n",
    );
    ctx.handoff.lock_ref_ready.store(true, Ordering::Release);

    while !ctx.handoff.guard_dropped.load(Ordering::Acquire) {
        unsafe { bindings::msleep(20) };
    }

    let mut relocked = leaked_lock.lock();
    relocked.value += 1;
    pr_info!(
        "thread B locked after thread A dropped the original guard; value={}\n",
        relocked.value
    );
    ctx.handoff.relock_done.store(true, Ordering::Release);
    0
}

fn spawn_thread_b(guard_addr: usize, handoff: Arc<HandoffState>, thread_id: u32) -> Result {
    let ctx = Arc::new(
        ThreadBContext {
            guard_addr,
            handoff,
            thread_id,
        },
        GFP_KERNEL,
    )?;
    let data = Arc::into_raw(ctx) as *mut kernel::ffi::c_void;

    let task = match from_err_ptr(unsafe {
        bindings::kthread_create_on_node(
            Some(thread_b_main),
            data,
            bindings::NUMA_NO_NODE,
            c_str!("lock_ref_thread_b/%u").as_char_ptr(),
            thread_id,
        )
    }) {
        Ok(task) => task,
        Err(err) => {
            // SAFETY: Thread creation failed, so the raw reference was not handed off.
            unsafe { drop(Arc::from_raw(data.cast::<ThreadBContext>())) };
            return Err(err);
        }
    };

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

unsafe extern "C" fn thread_a_main(_data: *mut kernel::ffi::c_void) -> i32 {
    let handoff = match Arc::new(
        HandoffState {
            lock_ref_ready: AtomicBool::new(false),
            guard_dropped: AtomicBool::new(false),
            relock_done: AtomicBool::new(false),
        },
        GFP_KERNEL,
    ) {
        Ok(h) => h,
        Err(_) => return ENOMEM.to_errno(),
    };

    stack_pin_init!(let lock = new_mutex!(
        SyncButNotSend {
            value: 100,
            _not_send: NotThreadSafe,
        },
        "lock_ref_poc.thread_a_lock"
    ));

    let guard = lock.lock();
    if !guard_matches_lock(&guard, &lock) {
        pr_err!("thread A: guard.lock_ref() did not return the original lock\n");
        return EINVAL.to_errno();
    }

    if let Err(err) = spawn_thread_b(
        (&guard as *const Guard<'_, SyncButNotSend, MutexBackend>) as usize,
        handoff.clone(),
        2,
    ) {
        return err.to_errno();
    }

    while !handoff.lock_ref_ready.load(Ordering::Acquire) {
        unsafe { bindings::msleep(20) };
    }

    drop(guard);
    pr_info!("thread A dropped the original guard after thread B captured &Lock\n");
    handoff.guard_dropped.store(true, Ordering::Release);

    while !handoff.relock_done.load(Ordering::Acquire) {
        unsafe { bindings::msleep(20) };
    }

    0
}

fn spawn_thread_a() -> Result {
    let task = match from_err_ptr(unsafe {
        bindings::kthread_create_on_node(
            Some(thread_a_main),
            core::ptr::null_mut(),
            bindings::NUMA_NO_NODE,
            c_str!("lock_ref_thread_a").as_char_ptr(),
        )
    }) {
        Ok(task) => task,
        Err(err) => return Err(err),
    };

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

impl kernel::Module for LockRefPoc {
    fn init(_module: &'static ThisModule) -> Result<Self> {
        
        spawn_thread_a()?;

        Ok(Self)
    }
}

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


Makefile:

obj-m += lock_ref_poc.o

KDIR := /lib/modules/$(shell uname -r)/build

PWD := $(shell pwd)

all:
	$(MAKE) -C $(KDIR) M=$(PWD) LLVM=1 modules

clean:
	$(MAKE) -C $(KDIR) M=$(PWD) clean


Output:
[  360.591802] lock_ref_poc: thread B obtained &Lock<SyncButNotSend, _> through &Guard
[  360.617600] lock_ref_poc: thread A dropped the original guard after thread B captured &Lock
[  360.641550] lock_ref_poc: thread B locked after thread A dropped the original guard; value=101
[  362.580557] lock_ref_poc: lock_ref_poc unloaded


Priya Bala Govindasamy (1):
  Make Guard::lock_ref unavailable if Lock is not Sync

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

-- 
2.34.1