rust_binder: check context manager before creating node

PATCH Superseded Keshav Verma 4 participants 8 messages Jun 17 – Jul 22 1 message not in the archive

Subsystems (2)

Rust Android drivers

lore

Share & tools

lore

Keshav Verma

author

2026-06-17 22:21 UTC #0 lore raw

PATCH 1/1 rust_binder: check context manager before creating node

Rust Binder currently creates the Binder node before checking whether a context manager is already registered. If a context manager already exists, set_manager_node() returns -EBUSY after node state has already been created.

Add a check before creating the node to match the C Binder ordering for the common already registered case. Keep the final checks in set_manager_node() so races with another caller are still handled after node creation.

Signed-off-by Keshav Verma <[email protected]>

---
 drivers/android/binder/context.rs | 20 ++++++++++++++++++++
 drivers/android/binder/process.rs |  1 +
 2 files changed, 21 insertions(+)

drivers/android/binder/context.rs +20 -0

index ddddb66b3557..562fb339b31f 100644
@@ -4,6 +4,7 @@
 
 use kernel::{
     alloc::kvec::KVVec,
+    cred::Credential,
     error::code::*,
     prelude::*,
     security,

@@ -107,6 +108,25 @@ pub(crate) fn deregister_process(self: &Arc<Self>, proc: &Arc<Process>) {
         }
     }
 
+    pub(crate) fn check_manager(&self, cred: &Credential) -> Result {
+        let manager = self.manager.lock();
+        if manager.node.is_some() {
+            pr_warn!("BINDER_SET_CONTEXT_MGR already set");
+            return Err(EBUSY);
+        }
+        security::binder_set_context_mgr(cred)?;
+
+        // If the context manager has been set before, ensure that we use the same euid.
+        let caller_uid = Kuid::current_euid();
+        if let Some(ref uid) = manager.uid {
+            if *uid != caller_uid {
+                return Err(EPERM);
+            }
+        }
+
+        Ok(())
+    }
+
     pub(crate) fn set_manager_node(&self, node_ref: NodeRef) -> Result {
         let mut manager = self.manager.lock();
         if manager.node.is_some() {

drivers/android/binder/process.rs +1 -0

index 96b8440ceac6..d09facebddf6 100644
@@ -741,6 +741,7 @@ fn set_as_manager(
         } else {
             (0, 0, 0)
         };
+        self.ctx.check_manager(&self.cred)?;
         let node_ref = self.get_node(ptr, cookie, flags as _, true, thread)?;
         let node = node_ref.node.clone();
         self.ctx.set_manager_node(node_ref)?;

Signature · 1 line

2.39.5

Greg Kroah-Hartman

maintainer

2026-06-18 10:27 UTC #1 lore raw

Quoted 4 lines · Keshav Verma

Rust Binder currently creates the Binder node before checking whether a context manager is already registered. If a context manager already exists, set_manager_node() returns -EBUSY after node state has already been created.

Odd line-wrapping :(

Quoted 6 lines

Add a check before creating the node to match the C Binder ordering for the common already registered case. Keep the final checks in set_manager_node() so races with another caller are still handled after node creation.
Signed-off-by Keshav Verma <[email protected]>

What commit id does this "fix"?

Quoted 18 lines

---
 drivers/android/binder/context.rs | 20 ++++++++++++++++++++
 drivers/android/binder/process.rs |  1 +
 2 files changed, 21 insertions(+)

drivers/android/binder/context.rs +1 -0

index ddddb66b3557..562fb339b31f 100644
@@ -4,6 +4,7 @@
 
 use kernel::{
     alloc::kvec::KVVec,
+    cred::Credential,
     error::code::*,
     prelude::*,
     security,

drivers/android/binder/context.rs pub(crate) fn deregister_process(self: &Arc<Self>, proc: &Arc<Process>) { · line 114

Quoted 7 lines

drivers/android/binder/context.rs +4 -0

@@ -107,6 +108,25 @@ pub(crate) fn deregister_process(self: &Arc<Self>, proc: &Arc<Process>) {
         }
     }
 
+    pub(crate) fn check_manager(&self, cred: &Credential) -> Result {
+        let manager = self.manager.lock();
+        if manager.node.is_some() {
+            pr_warn!("BINDER_SET_CONTEXT_MGR already set");

How can this be triggered?

thanks,

greg k-h

A message that is not in the archive (sent off-list or not yet received): CAPE_3zLCOwqa8Yb21oAb8i9AHW-UPqW=+9848fUtuTKK6wAZDA@mail.gmail.com

Greg Kroah-Hartman

maintainer

2026-06-18 11:29 UTC #3 lore raw

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

Quoted 2 lines · Keshav Verma

Thanks, I will fix the line wrapping and add the appropriate Fixes tag in v2.

Also, you sent html email which the lists reject :(

Quoted 9 lines

This can be triggered after the normal context manager has already been registered. A second process can open /dev/binder and call BINDER_SET_CONTEXT_MGR_EXT with a local BINDER_TYPE_BINDER object. The ioctl returns -EBUSY because the context manager already exists.
Repeating this with unique ptr/cookie values triggers the warning each time.
Since this is easy to trigger from userspace, should I drop the pr_warn!() from the check in v2 and just return -EBUSY there?

If userspace can trigger this, then please remove it so that it doesn't cause a denial-of-service on the kernel log.

thanks,

greg k-h

Miguel Ojeda

2026-06-18 11:33 UTC #4 lore raw

In reply to a message not in the archive

Quoted 2 lines · Keshav Verma

Thanks, I will fix the line wrapping and add the appropriate Fixes tag in v2.

Please consider (depending on the hash that this fixes) whether it needs Cc: stable@ too.

Quoted 3 lines

Repeating this with unique ptr/cookie values triggers the warning each time.
Since this is easy to trigger from userspace, should I drop the pr_warn!() from the check in v2 and just return -EBUSY there?

If it is something that is expected to be able to be triggered from userspace, then printing is a bad idea, and at the very least it may need to be pr_warn_once! if you somehow think it can be useful for users.

Now, for things that are truly logic errors, i.e. things that in principle should not be possible to trigger from userspace unless there is a bug in the kernel, you may consider pr_warn_once! + debug_assert!(false); in that path. But only if you are truly sure that it not meant to be hit.

I hope that helps.

Cheers,
Miguel

Alice Ryhl

maintainer

2026-06-18 11:52 UTC #5 lore raw

In reply to Greg Kroah-Hartman

Quoted 9 lines · Greg Kroah-Hartman

Quoted 4 lines · Keshav Verma
Rust Binder currently creates the Binder node before checking whether a context manager is already registered. If a context manager already exists, set_manager_node() returns -EBUSY after node state has already been created.
Odd line-wrapping :(
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
Add a check before creating the node to match the C Binder ordering for the common already registered case. Keep the final checks in set_manager_node() so races with another caller are still handled after node creation.
Signed-off-by Keshav Verma <[email protected]>
What commit id does this "fix"?

There's no need to mark this as a fix. It's nothing more than a fast-path that does less work when exiting on error. As the commit message notes, the existing error check after get_node() is still needed because otherwise two threads could invoke this in parallel leading to multiple context managers.

Quoted 6 lines

drivers/android/binder/context.rs pub(crate) fn deregister_process(self: &Arc<Self>, proc: &Arc<Process>) { · line 114
Quoted 4 lines
drivers/android/binder/context.rs +4 -0
+    pub(crate) fn check_manager(&self, cred: &Credential) -> Result {
+        let manager = self.manager.lock();
+        if manager.node.is_some() {
+            pr_warn!("BINDER_SET_CONTEXT_MGR already set");
How can this be triggered?

If two processes try to register themselves as the context manager of this context, this occurs. The warning should probably be using rate-limited printing.

Alice

Keshav Verma

author

2026-06-18 12:12 UTC #6 lore raw

PATCH v2 1/1 rust_binder: check context manager before creating node

In reply to Keshav Verma

Signed-off-by Keshav Verma <[email protected]>

---
Changes in v2:
- Fix commit message line wrapping.
- Drop pr_warn!() from the pre-check since userspace can trigger it.

 drivers/android/binder/context.rs | 19 +++++++++++++++++++
 drivers/android/binder/process.rs |  1 +
 2 files changed, 20 insertions(+)

drivers/android/binder/context.rs +19 -0

index ddddb66b3557..f7ae84074f96 100644
@@ -4,6 +4,7 @@
 
 use kernel::{
     alloc::kvec::KVVec,
+    cred::Credential,
     error::code::*,
     prelude::*,
     security,

@@ -107,6 +108,24 @@ pub(crate) fn deregister_process(self: &Arc<Self>, proc: &Arc<Process>) {
         }
     }
 
+    pub(crate) fn check_manager(&self, cred: &Credential) -> Result {
+        let manager = self.manager.lock();
+        if manager.node.is_some() {
+            return Err(EBUSY);
+        }
+        security::binder_set_context_mgr(cred)?;
+
+        // If the context manager has been set before, ensure that we use the same euid.
+        let caller_uid = Kuid::current_euid();
+        if let Some(ref uid) = manager.uid {
+            if *uid != caller_uid {
+                return Err(EPERM);
+            }
+        }
+
+        Ok(())
+    }
+
     pub(crate) fn set_manager_node(&self, node_ref: NodeRef) -> Result {
         let mut manager = self.manager.lock();
         if manager.node.is_some() {

drivers/android/binder/process.rs +1 -0

index 96b8440ceac6..d09facebddf6 100644
@@ -741,6 +741,7 @@ fn set_as_manager(
         } else {
             (0, 0, 0)
         };
+        self.ctx.check_manager(&self.cred)?;
         let node_ref = self.get_node(ptr, cookie, flags as _, true, thread)?;
         let node = node_ref.node.clone();
         self.ctx.set_manager_node(node_ref)?;

Signature · 1 line

2.39.5

Alice Ryhl

maintainer

2026-07-21 07:34 UTC #7 lore raw

Quoted 10 lines · Keshav Verma

Rust Binder currently creates the Binder node before checking whether a context manager is already registered. If a context manager already exists, set_manager_node() returns -EBUSY after node state has already been created.
Add a check before creating the node to match the C Binder ordering for the common already registered case. Keep the final checks in set_manager_node() so races with another caller are still handled after node creation.
Signed-off-by Keshav Verma <[email protected]>

Honestly, I don't think we should make this change. It duplicates logic to optimize for a case that doesn't exist in practice. I'd prefer to avoid the code duplication this causes, than I want to optimize for a codepath nobody ever hits in practice.

Alice

Keshav Verma

author

2026-07-22 02:25 UTC #8 lore raw

Quoted 4 lines · Alice Ryhl

Honestly, I don't think we should make this change. It duplicates logic to optimize for a case that doesn't exist in practice. I'd prefer to avoid the code duplication this causes, than I want to optimize for a codepath nobody ever hits in practice.

Makes sense. Thanks again for the review.

Thank you,
Keshav Verma