[PATCH 0/3] rust_binder: fix unsoundness due to combining List::remove with mem:take

[PATCH 0/3] rust_binder: fix unsoundness due to combining List::remove with mem:take

[Alice Ryhl]

See the first patch for details on the bug.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Alice Ryhl (3):
      rust_binder: fix race condition on death_list
      rust_binder: avoid mem::take on delivered_deaths
      rust: list: add warning to List::remove docs about mem::take

 drivers/android/binder/node.rs    | 6 +++---
 drivers/android/binder/process.rs | 8 ++++++--
 rust/kernel/list.rs               | 3 +++
 3 files changed, 12 insertions(+), 5 deletions(-)
---
base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
change-id: 20251111-binder-fix-list-remove-41c29c75ffc9

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH 1/3] rust_binder: fix race condition on death_list

[Alice Ryhl]

Rust Binder contains the following unsafe operation:

	// 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) };

This operation is unsafe because when touching the prev/next pointers of
a list element, we have to ensure that no other thread is also touching
them in parallel. If the node is present in the list that `remove` is
called on, then that is fine because we have exclusive access to that
list. If the node is not in any list, then it's also ok. But if it's
present in a different list that may be accessed in parallel, then that
may be a data race on the prev/next pointers.

And unfortunately that is exactly what is happening here. In
Node::release, we:

 1. Take the lock.
 2. Move all items to a local list on the stack.
 3. Drop the lock.
 4. Iterate the local list on the stack.

Combined with threads using the unsafe remove method on the original
list, this leads to memory corruption of the prev/next pointers. This
leads to crashes like this one:

	Unable to handle kernel paging request at virtual address 000bb9841bcac70e
	Mem abort info:
	  ESR = 0x0000000096000044
	  EC = 0x25: DABT (current EL), IL = 32 bits
	  SET = 0, FnV = 0
	  EA = 0, S1PTW = 0
	  FSC = 0x04: level 0 translation fault
	Data abort info:
	  ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
	  CM = 0, WnR = 1, TnD = 0, TagAccess = 0
	  GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
	[000bb9841bcac70e] address between user and kernel address ranges
	Internal error: Oops: 0000000096000044 [#1] PREEMPT SMP
	google-cdd 538c004.gcdd: context saved(CPU:1)
	item - log_kevents is disabled
	Modules linked in: ... rust_binder
	CPU: 1 UID: 0 PID: 2092 Comm: kworker/1:178 Tainted: G S      W  OE      6.12.52-android16-5-g98debd5df505-4k #1 f94a6367396c5488d635708e43ee0c888d230b0b
	Tainted: [S]=CPU_OUT_OF_SPEC, [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
	Hardware name: MUSTANG PVT 1.0 based on LGA (DT)
	Workqueue: events _RNvXs6_NtCsdfZWD8DztAw_6kernel9workqueueINtNtNtB7_4sync3arc3ArcNtNtCs8QPsHWIn21X_16rust_binder_main7process7ProcessEINtB5_15WorkItemPointerKy0_E3runB13_ [rust_binder]
	pstate: 23400005 (nzCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
	pc : _RNvXs3_NtCs8QPsHWIn21X_16rust_binder_main7processNtB5_7ProcessNtNtCsdfZWD8DztAw_6kernel9workqueue8WorkItem3run+0x450/0x11f8 [rust_binder]
	lr : _RNvXs3_NtCs8QPsHWIn21X_16rust_binder_main7processNtB5_7ProcessNtNtCsdfZWD8DztAw_6kernel9workqueue8WorkItem3run+0x464/0x11f8 [rust_binder]
	sp : ffffffc09b433ac0
	x29: ffffffc09b433d30 x28: ffffff8821690000 x27: ffffffd40cbaa448
	x26: ffffff8821690000 x25: 00000000ffffffff x24: ffffff88d0376578
	x23: 0000000000000001 x22: ffffffc09b433c78 x21: ffffff88e8f9bf40
	x20: ffffff88e8f9bf40 x19: ffffff882692b000 x18: ffffffd40f10bf00
	x17: 00000000c006287d x16: 00000000c006287d x15: 00000000000003b0
	x14: 0000000000000100 x13: 000000201cb79ae0 x12: fffffffffffffff0
	x11: 0000000000000000 x10: 0000000000000001 x9 : 0000000000000000
	x8 : b80bb9841bcac706 x7 : 0000000000000001 x6 : fffffffebee63f30
	x5 : 0000000000000000 x4 : 0000000000000001 x3 : 0000000000000000
	x2 : 0000000000004c31 x1 : ffffff88216900c0 x0 : ffffff88e8f9bf00
	Call trace:
	 _RNvXs3_NtCs8QPsHWIn21X_16rust_binder_main7processNtB5_7ProcessNtNtCsdfZWD8DztAw_6kernel9workqueue8WorkItem3run+0x450/0x11f8 [rust_binder bbc172b53665bbc815363b22e97e3f7e3fe971fc]
	 process_scheduled_works+0x1c4/0x45c
	 worker_thread+0x32c/0x3e8
	 kthread+0x11c/0x1c8
	 ret_from_fork+0x10/0x20
	Code: 94218d85 b4000155 a94026a8 d10102a0 (f9000509)
	---[ end trace 0000000000000000 ]---

Thus, modify Node::release to pop items directly off the original list.

Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/node.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs
index ade895ef791ec5746f9f5c1bfc15f47d59829455..107e08a3ba782225c0f8e03add247ec667a970d6 100644
--- a/drivers/android/binder/node.rs
+++ b/drivers/android/binder/node.rs
@@ -541,10 +541,10 @@ pub(crate) fn release(&self) {
             guard = self.owner.inner.lock();
         }
 
-        let death_list = core::mem::take(&mut self.inner.access_mut(&mut guard).death_list);
-        drop(guard);
-        for death in death_list {
+        while let Some(death) = self.inner.access_mut(&mut guard).death_list.pop_front() {
+            drop(guard);
             death.into_arc().set_dead();
+            guard = self.owner.inner.lock();
         }
     }
 

-- 
2.51.2.1041.gc1ab5b90ca-goog

[PATCH 2/3] rust_binder: avoid mem::take on delivered_deaths

[Alice Ryhl]

Similar to the previous commit, List::remove is used on
delivered_deaths, so do not use mem::take on it as that may result in
violations of the List::remove safety requirements.

I don't think this particular case can be triggered because it requires
fd close to run in parallel with an ioctl on the same fd. But let's not
tempt fate.

Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/process.rs | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index f13a747e784c84a0fb09cbf47442712106eba07c..022f554bb049280126fdaf636dc7a41dd02c535e 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -1335,8 +1335,12 @@ fn deferred_release(self: Arc<Self>) {
             work.into_arc().cancel();
         }
 
-        let delivered_deaths = take(&mut self.inner.lock().delivered_deaths);
-        drop(delivered_deaths);
+        // Clear delivered_deaths list.
+        //
+        // Scope ensures that MutexGuard is dropped while executing the body.
+        while let Some(delivered_death) = { self.inner.lock().delivered_deaths.pop_front() } {
+            drop(delivered_death);
+        }
 
         // Free any resources kept alive by allocated buffers.
         let omapping = self.inner.lock().mapping.take();

-- 
2.51.2.1041.gc1ab5b90ca-goog

[PATCH 3/3] rust: list: add warning to List::remove docs about mem::take

[Alice Ryhl]

The previous patches in this series illustrate why the List::remove
method is really dangerous. I think the real takeaway here is to replace
the linked lists with a different data structure without this unsafe
footgun, but for now we fix the bugs and add a warning to the docs.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/list.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
index 7355bbac16a7fe7feeb8bc6408671817b186b21d..8349ff32fc37ff7141fb7c62d26653bda6507f91 100644
--- a/rust/kernel/list.rs
+++ b/rust/kernel/list.rs
@@ -576,6 +576,9 @@ pub fn pop_front(&mut self) -> Option<ListArc<T, ID>> {
     /// This returns `None` if the item is not in the list. (Note that by the safety requirements,
     /// this means that the item is not in any list.)
     ///
+    /// When using this method, be careful with using `mem::take` on the same list as that may
+    /// result in violating the safety requirements of this method.
+    ///
     /// # Safety
     ///
     /// `item` must not be in a different linked list (with the same id).

-- 
2.51.2.1041.gc1ab5b90ca-goog

Re: [PATCH 3/3] rust: list: add warning to List::remove docs about mem::take

[Miguel Ojeda]

On Tue, Nov 11, 2025 at 3:23 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> +    /// When using this method, be careful with using `mem::take` on the same list as that may
> +    /// result in violating the safety requirements of this method.

I suggested adding an example or similar to hopefully help prevent
this in the future, and this note is "to the point", so that is great.
Thanks for adding it!

Nit: intra-doc link [`core::mem::take`]

Cheers,
Miguel

Re: [PATCH 0/3] rust_binder: fix unsoundness due to combining List::remove with mem:take

[Miguel Ojeda]

On Tue, Nov 11, 2025 at 3:23 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
>       rust_binder: fix race condition on death_list
>       rust_binder: avoid mem::take on delivered_deaths
>       rust: list: add warning to List::remove docs about mem::take

Greg et al.: please let me know if you are not taking the last one
together with the fixes (so that I pick it up). Otherwise, if you do:

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Thanks!

Cheers,
Miguel

Re: [PATCH 0/3] rust_binder: fix unsoundness due to combining List::remove with mem:take

[Greg Kroah-Hartman]

On Wed, Nov 12, 2025 at 11:22:25AM +0100, Miguel Ojeda wrote:
> On Tue, Nov 11, 2025 at 3:23 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> >       rust_binder: fix race condition on death_list
> >       rust_binder: avoid mem::take on delivered_deaths
> >       rust: list: add warning to List::remove docs about mem::take
> 
> Greg et al.: please let me know if you are not taking the last one
> together with the fixes (so that I pick it up). Otherwise, if you do:
> 
> Acked-by: Miguel Ojeda <ojeda@kernel.org>

I'll take all of these, thanks for the ack!

greg k-h