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,
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(); } }
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();
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
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::takeGreg 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
linux-stable-mirror@lists.linaro.org
-
Alice Ryhl -
Greg Kroah-Hartman -
Miguel Ojeda