Message-ID: <20231101-rust-binder-v1-14-08ba9197f637@google.com>
Date:   Wed, 01 Nov 2023 18:01:44 +0000
From:   Alice Ryhl <aliceryhl@...gle.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Arve Hjønnevåg" <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <brauner@...nel.org>,
        Carlos Llamas <cmllamas@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>
Cc:     linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        "Björn Roy Baron" <bjorn3_gh@...tonmail.com>,
        Benno Lossin <benno.lossin@...ton.me>,
        Andreas Hindborg <a.hindborg@...sung.com>,
        Matt Gilbride <mattgilbride@...gle.com>,
        Jeffrey Vander Stoep <jeffv@...gle.com>,
        Matthew Maurer <mmaurer@...gle.com>,
        Alice Ryhl <aliceryhl@...gle.com>
Subject: [PATCH RFC 14/20] rust_binder: add BINDER_TYPE_FDA support

In the previous patch, we introduced support for `BINDER_TYPE_FD`
objects that let you send a single fd, and in this patch, we add support
for FD arrays. One important difference between `BINDER_TYPE_FD` and
`BINDER_TYPE_FDA` is that FD arrays will close the file descriptors when
the transaction allocation is freed, whereas FDs sent using
`BINDER_TYPE_FD` are not closed.

Note that `BINDER_TYPE_FDA` is used only with hwbinder.

Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
---
 drivers/android/allocation.rs  | 74 ++++++++++++++++++++++++++++++++---
 drivers/android/defs.rs        |  1 +
 drivers/android/thread.rs      | 87 +++++++++++++++++++++++++++++++++++++++---
 drivers/android/transaction.rs | 13 ++++---
 4 files changed, 159 insertions(+), 16 deletions(-)

diff --git a/drivers/android/allocation.rs b/drivers/android/allocation.rs
index 9d777ffb7176..c7f44a54b79b 100644
--- a/drivers/android/allocation.rs
+++ b/drivers/android/allocation.rs
@@ -4,7 +4,7 @@
 
 use kernel::{
     bindings,
-    file::{File, FileDescriptorReservation},
+    file::{DeferredFdCloser, File, FileDescriptorReservation},
     io_buffer::{IoBufferReader, ReadableFromBytes, WritableToBytes},
     pages::Pages,
     prelude::*,
@@ -165,18 +165,38 @@ pub(crate) fn set_info_target_node(&mut self, target_node: NodeRef) {
         self.get_or_init_info().target_node = Some(target_node);
     }
 
-    pub(crate) fn info_add_fd(&mut self, file: ARef<File>, buffer_offset: usize) -> Result {
+    /// Reserve enough space to push at least `num_fds` fds.
+    pub(crate) fn info_add_fd_reserve(&mut self, num_fds: usize) -> Result {
+        self.get_or_init_info()
+            .file_list
+            .files_to_translate
+            .try_reserve(num_fds)?;
+
+        Ok(())
+    }
+
+    pub(crate) fn info_add_fd(
+        &mut self,
+        file: ARef<File>,
+        buffer_offset: usize,
+        close_on_free: bool,
+    ) -> Result {
         self.get_or_init_info()
             .file_list
             .files_to_translate
             .try_push(FileEntry {
                 file,
                 buffer_offset,
+                close_on_free,
             })?;
 
         Ok(())
     }
 
+    pub(crate) fn set_info_close_on_free(&mut self, cof: FdsCloseOnFree) {
+        self.get_or_init_info().file_list.close_on_free = cof.0;
+    }
+
     pub(crate) fn translate_fds(&mut self) -> Result<TranslatedFds> {
         let file_list = match self.allocation_info.as_mut() {
             Some(info) => &mut info.file_list,
@@ -184,17 +204,38 @@ pub(crate) fn translate_fds(&mut self) -> Result<TranslatedFds> {
         };
 
         let files = core::mem::take(&mut file_list.files_to_translate);
+
+        let num_close_on_free = files.iter().filter(|entry| entry.close_on_free).count();
+        let mut close_on_free = Vec::try_with_capacity(num_close_on_free)?;
+
         let mut reservations = Vec::try_with_capacity(files.len())?;
         for file_info in files {
             let res = FileDescriptorReservation::new(bindings::O_CLOEXEC)?;
-            self.write::<u32>(file_info.buffer_offset, &res.reserved_fd())?;
+            let fd = res.reserved_fd();
+            self.write::<u32>(file_info.buffer_offset, &fd)?;
             reservations.try_push(Reservation {
                 res,
                 file: file_info.file,
             })?;
+            if file_info.close_on_free {
+                close_on_free.try_push(fd)?;
+            }
         }
 
-        Ok(TranslatedFds { reservations })
+        Ok(TranslatedFds {
+            reservations,
+            close_on_free: FdsCloseOnFree(close_on_free),
+        })
+    }
+
+    /// Should the looper return to userspace when freeing this allocation?
+    pub(crate) fn looper_need_return_on_free(&self) -> bool {
+        // Closing fds involves pushing task_work for execution when we return to userspace. Hence,
+        // we should return to userspace asap if we are closing fds.
+        match self.allocation_info {
+            Some(ref info) => !info.file_list.close_on_free.is_empty(),
+            None => false,
+        }
     }
 }
 
@@ -220,6 +261,18 @@ fn drop(&mut self) {
                 }
             }
 
+            for &fd in &info.file_list.close_on_free {
+                let closer = match DeferredFdCloser::new() {
+                    Ok(closer) => closer,
+                    Err(core::alloc::AllocError) => {
+                        // Ignore allocation failures.
+                        break;
+                    }
+                };
+
+                closer.close_fd(fd);
+            }
+
             if info.clear_on_free {
                 if let Err(e) = self.fill_zero() {
                     pr_warn!("Failed to clear data on free: {:?}", e);
@@ -457,6 +510,7 @@ fn type_to_size(type_: u32) -> Option<usize> {
 #[derive(Default)]
 struct FileList {
     files_to_translate: Vec<FileEntry>,
+    close_on_free: Vec<u32>,
 }
 
 struct FileEntry {
@@ -464,10 +518,15 @@ struct FileEntry {
     file: ARef<File>,
     /// The offset in the buffer where the file descriptor is stored.
     buffer_offset: usize,
+    /// Whether this fd should be closed when the allocation is freed.
+    close_on_free: bool,
 }
 
 pub(crate) struct TranslatedFds {
     reservations: Vec<Reservation>,
+    /// If commit is called, then these fds should be closed. (If commit is not called, then they
+    /// shouldn't be closed.)
+    close_on_free: FdsCloseOnFree,
 }
 
 struct Reservation {
@@ -479,12 +538,17 @@ impl TranslatedFds {
     pub(crate) fn new() -> Self {
         Self {
             reservations: Vec::new(),
+            close_on_free: FdsCloseOnFree(Vec::new()),
         }
     }
 
-    pub(crate) fn commit(self) {
+    pub(crate) fn commit(self) -> FdsCloseOnFree {
         for entry in self.reservations {
             entry.res.commit(entry.file);
         }
+
+        self.close_on_free
     }
 }
+
+pub(crate) struct FdsCloseOnFree(Vec<u32>);
diff --git a/drivers/android/defs.rs b/drivers/android/defs.rs
index fa4ec3eff424..8f9419d474de 100644
--- a/drivers/android/defs.rs
+++ b/drivers/android/defs.rs
@@ -106,6 +106,7 @@ fn default() -> Self {
 decl_wrapper!(BinderNodeInfoForRef, bindings::binder_node_info_for_ref);
 decl_wrapper!(FlatBinderObject, bindings::flat_binder_object);
 decl_wrapper!(BinderFdObject, bindings::binder_fd_object);
+decl_wrapper!(BinderFdArrayObject, bindings::binder_fd_array_object);
 decl_wrapper!(BinderObjectHeader, bindings::binder_object_header);
 decl_wrapper!(BinderBufferObject, bindings::binder_buffer_object);
 decl_wrapper!(BinderTransactionData, bindings::binder_transaction_data);
diff --git a/drivers/android/thread.rs b/drivers/android/thread.rs
index 56b36dc43bcc..2e86592fb61f 100644
--- a/drivers/android/thread.rs
+++ b/drivers/android/thread.rs
@@ -654,7 +654,8 @@ fn translate_object(
                 const FD_FIELD_OFFSET: usize =
                     ::core::mem::offset_of!(bindings::binder_fd_object, __bindgen_anon_1.fd)
                         as usize;
-                view.alloc.info_add_fd(file, offset + FD_FIELD_OFFSET)?;
+                view.alloc
+                    .info_add_fd(file, offset + FD_FIELD_OFFSET, false)?;
             }
             BinderObjectRef::Ptr(obj) => {
                 let obj_length = obj.length.try_into().map_err(|_| EINVAL)?;
@@ -729,9 +730,77 @@ fn translate_object(
                 obj_write.parent_offset = obj.parent_offset;
                 view.write::<BinderBufferObject>(offset, &obj_write)?;
             }
-            BinderObjectRef::Fda(_obj) => {
-                pr_warn!("Using unsupported binder object type fda.");
-                return Err(EINVAL.into());
+            BinderObjectRef::Fda(obj) => {
+                if !allow_fds {
+                    return Err(EPERM.into());
+                }
+                let parent_index = usize::try_from(obj.parent).map_err(|_| EINVAL)?;
+                let parent_offset = usize::try_from(obj.parent_offset).map_err(|_| EINVAL)?;
+                let num_fds = usize::try_from(obj.num_fds).map_err(|_| EINVAL)?;
+                let fds_len = num_fds.checked_mul(size_of::<u32>()).ok_or(EINVAL)?;
+
+                view.alloc.info_add_fd_reserve(num_fds)?;
+
+                let info = sg_state.validate_parent_fixup(parent_index, parent_offset, fds_len)?;
+
+                sg_state.ancestors.truncate(info.num_ancestors);
+                let parent_entry = match sg_state.sg_entries.get_mut(info.parent_sg_index) {
+                    Some(parent_entry) => parent_entry,
+                    None => {
+                        pr_err!(
+                            "validate_parent_fixup returned index out of bounds for sg.entries"
+                        );
+                        return Err(EINVAL.into());
+                    }
+                };
+
+                parent_entry.fixup_min_offset = info.new_min_offset;
+                parent_entry
+                    .pointer_fixups
+                    .try_push(PointerFixupEntry {
+                        skip: fds_len,
+                        pointer_value: 0,
+                        target_offset: info.target_offset,
+                    })
+                    .map_err(|_| ENOMEM)?;
+
+                let fda_uaddr = parent_entry
+                    .sender_uaddr
+                    .checked_add(parent_offset)
+                    .ok_or(EINVAL)?;
+                let fda_bytes = UserSlicePtr::new(fda_uaddr as _, fds_len).read_all()?;
+
+                if fds_len != fda_bytes.len() {
+                    pr_err!("UserSlicePtr::read_all returned wrong length in BINDER_TYPE_FDA");
+                    return Err(EINVAL.into());
+                }
+
+                for i in (0..fds_len).step_by(size_of::<u32>()) {
+                    let fd = {
+                        let mut fd_bytes = [0u8; size_of::<u32>()];
+                        fd_bytes.copy_from_slice(&fda_bytes[i..i + size_of::<u32>()]);
+                        u32::from_ne_bytes(fd_bytes)
+                    };
+
+                    let file = File::from_fd(fd)?;
+                    security::binder_transfer_file(
+                        &self.process.cred,
+                        &view.alloc.process.cred,
+                        &file,
+                    )?;
+
+                    // The `validate_parent_fixup` call ensuers that this addition will not
+                    // overflow.
+                    view.alloc.info_add_fd(file, info.target_offset + i, true)?;
+                }
+                drop(fda_bytes);
+
+                let mut obj_write = BinderFdArrayObject::default();
+                obj_write.hdr.type_ = BINDER_TYPE_FDA;
+                obj_write.num_fds = obj.num_fds;
+                obj_write.parent = obj.parent;
+                obj_write.parent_offset = obj.parent_offset;
+                view.write::<BinderFdArrayObject>(offset, &obj_write)?;
             }
         }
         Ok(())
@@ -1160,7 +1229,15 @@ fn write(self: &Arc<Self>, req: &mut BinderWriteRead) -> Result {
                     let tr = reader.read::<BinderTransactionDataSg>()?;
                     self.transaction(&tr, Self::reply_inner)
                 }
-                BC_FREE_BUFFER => drop(self.process.buffer_get(reader.read()?)),
+                BC_FREE_BUFFER => {
+                    let buffer = self.process.buffer_get(reader.read()?);
+                    if let Some(buffer) = &buffer {
+                        if buffer.looper_need_return_on_free() {
+                            self.inner.lock().looper_need_return = true;
+                        }
+                    }
+                    drop(buffer);
+                }
                 BC_INCREFS => self.process.update_ref(reader.read()?, true, false)?,
                 BC_ACQUIRE => self.process.update_ref(reader.read()?, true, true)?,
                 BC_RELEASE => self.process.update_ref(reader.read()?, false, true)?,
diff --git a/drivers/android/transaction.rs b/drivers/android/transaction.rs
index 3230ea490a5b..ec32a9fd0ff1 100644
--- a/drivers/android/transaction.rs
+++ b/drivers/android/transaction.rs
@@ -288,17 +288,18 @@ fn do_work(self: DArc<Self>, thread: &Thread, writer: &mut UserSlicePtrWriter) -
             writer.write(&*tr)?;
         }
 
+        let mut alloc = self.allocation.lock().take().ok_or(ESRCH)?;
+
         // Dismiss the completion of transaction with a failure. No failure paths are allowed from
         // here on out.
         send_failed_reply.dismiss();
 
-        // It is now the user's responsibility to clear the allocation.
-        let alloc = self.allocation.lock().take();
-        if let Some(alloc) = alloc {
-            alloc.keep_alive();
-        }
+        // Commit files, and set FDs in FDA to be closed on buffer free.
+        let close_on_free = files.commit();
+        alloc.set_info_close_on_free(close_on_free);
 
-        files.commit();
+        // It is now the user's responsibility to clear the allocation.
+        alloc.keep_alive();
 
         // When this is not a reply and not a oneway transaction, update `current_transaction`. If
         // it's a reply, `current_transaction` has already been updated appropriately.

-- 
2.42.0.820.g83a721a137-goog

Powered by blists - more mailing lists