The unsafe invariant in `Node::add_death` relies only on implicit calling convention

[GeorgeAndrou]

Hello,

While studying the Android binder driver as part of our research into Rust kernel code safety, we came across what looks like a latent soundness issue in drivers/android/binder/node.rs. We are flagging it in case it is worth addressing.

Confirmed on: Linux kernel 7.1-rc5 mainline (8fde5d1d47f6)

The Problem

The // SAFETY: comment on NodeDeath::set_cleared says:

// SAFETY: A `NodeDeath` is never inserted into the death list of any node
// other than its owner, so it is either in this death list or in no death list.
unsafe { node_inner.death_list.remove(self) };

The function responsible for inserting NodeDeath objects is Node::add_death:

pub(crate) fn add_death(
    &self,
    death: ListArc<DTRWrap<NodeDeath>, 1>,
    guard: &mut Guard<'_, ProcessInner, SpinLockBackend>,
) {
    self.inner.access_mut(guard).death_list.push_back(death);
}

add_death is a safe function but has no check that death.node == self and access_mut does not check this either, it only checks that guard is the lock guard for the node owner's ProcessInner. The invariant holds today only because the existing caller Process::request_death always creates a NodeDeath and immediately inserts it into the same node's list, making death.node == self true by construction. This is convention, not enforcement: add_death imposes no such requirement, and the only hint is buried in the // SAFETY: comment of a completely different function.

The kernel Rust coding guidelines state that // SAFETY: comments should explain why the unsafe block "cannot trigger undefined behavior in any case". We believe this // SAFETY: comment may not fully satisfy that requirement, though we welcome the maintainers' perspective on this.

A Similar Pattern With Enforcement Already Exists in Binder

We noticed that Registration::new solves a structurally identical problem for ready_threads. It explicitly asserts the ownership invariant before inserting:

assert!(core::ptr::eq(&thread.process.inner, guard.lock_ref()));
// INVARIANT: We are pushing this thread to the right `ready_threads` list.
guard.ready_threads.push_front(list_arc);

Its drop has an unsafe remove with a // SAFETY: comment structurally identical to set_cleared:

// SAFETY: The thread has the invariant that we never push it to any other linked list
// than the `ready_threads` list of its parent process. Therefore, the thread is either
// in that list, or in no list.
unsafe { inner.ready_threads.remove(self.thread) };

For ready_threads the // SAFETY: comment holds in all cases because Registration::new enforces it at insertion time. For death_list the same enforcement is absent. We thought this inconsistency was worth pointing out.

PoC

The following calling sequence of only safe Rust functions triggers a null pointer dereference in List::remove_internal_inner (rust/kernel/list.rs):

// Setup: create a binder process and thread
let name = CStr::from_bytes_with_nul(b"q\0").map_err(|_| EINVAL)?;
let ctx = crate::Context::new(name)?;
let local_file = kernel::fs::LocalFile::fget(3)?;
let cred = local_file.cred();
let process = Process::new(ctx.into(), ARef::from(cred))?;
let thread = process.as_arc_borrow().get_current_thread()?;

// Create two distinct nodes in the same process
let node_a = Process::get_node(
    process.as_arc_borrow(), 0x660, 4, 2, false, &thread,
)?.node.clone();

let node_b = Process::get_node(
    process.as_arc_borrow(), 0, 8, 4, false, &thread,
)?.node.clone();

// Create NodeDeath_B owned by node_B
let death_b_uninit = kernel::sync::UniqueArc::new_uninit(GFP_KERNEL)?;
let init_b = NodeDeath::new(node_b, process.clone(), 8);
let death_b = death_b_uninit.pin_init_with(init_b).map_err(|_| ENOMEM)?;
let larc_b: ListArc<crate::DTRWrap<NodeDeath>, 1> = ListArc::from(death_b);
let darc_b: DArc<NodeDeath> = larc_b.clone_arc();

// THE VIOLATION: insert NodeDeath_B into node_A.death_list
// add_death is safe and has no ownership check
// node_A.death_list = [NodeDeath_B]  ← WRONG OWNER
// NodeDeath_B.node  = node_B ≠ node_A
{
    let mut guard = node_a.owner.inner.lock();
    node_a.add_death(larc_b, &mut guard);
}

NodeDeath::set_cleared(&darc_b, true);

// → NULL POINTER DEREFERENCE at rust/kernel/list.rs:672 (https://github.com/torvalds/linux/blob/8fde5d1d47f69db6082dfa34500c27f8485389a5/rust/kernel/list.rs)
Process::deferred_release(process);

Suggested Fix

Adding an ownership check to add_death — mirroring what Registration::new already does for ready_threads — would make the // SAFETY: comment in set_cleared hold in all cases:

pub(crate) fn add_death(
    &self,
    death: ListArc<DTRWrap<NodeDeath>, 1>,
    guard: &mut Guard<'_, ProcessInner, SpinLockBackend>,
) {
    debug_assert!(
        core::ptr::eq(&**death.node as *const Node, self as *const Node),
        "NodeDeath must belong to this node"
    );
    self.inner.access_mut(guard).death_list.push_back(death);
}

[ojeda]

Cc: @Darksonn @gregkh

@GeorgeAndrou How does a debug_assert! fix a soundness issue?

[GeorgeAndrou]

@ojeda Fair point. My suggested fix is an initial suggestion that in debug builds it would cause a panic at the point of the wrong insertion instead of a null pointer dereference later.