Commit 8f589c9c authored by Alice Ryhl's avatar Alice Ryhl Committed by Greg Kroah-Hartman
Browse files

rust_binder: correctly handle FDA objects of length zero 



Fix a bug where an empty FDA (fd array) object with 0 fds would cause an
out-of-bounds error. The previous implementation used `skip == 0` to
mean "this is a pointer fixup", but 0 is also the correct skip length
for an empty FDA. If the FDA is at the end of the buffer, then this
results in an attempt to write 8-bytes out of bounds. This is caught and
results in an EINVAL error being returned to userspace.

The pattern of using `skip == 0` as a special value originates from the
C-implementation of Binder. As part of fixing this bug, this pattern is
replaced with a Rust enum.

I considered the alternate option of not pushing a fixup when the length
is zero, but I think it's cleaner to just get rid of the zero-is-special
stuff.

The root cause of this bug was diagnosed by Gemini CLI on first try. I
used the following prompt:

> There appears to be a bug in @drivers/android/binder/thread.rs where
> the Fixups oob bug is triggered with 316 304 316 324. This implies
> that we somehow ended up with a fixup where buffer A has a pointer to
> buffer B, but the pointer is located at an index in buffer A that is
> out of bounds. Please investigate the code to find the bug. You may
> compare with @drivers/android/binder.c that implements this correctly.

Cc: stable@vger.kernel.org
Reported-by: default avatarDeepChirp <DeepChirp@outlook.com>
Closes: https://github.com/waydroid/waydroid/issues/2157


Fixes: eafedbc7 ("rust_binder: add Rust Binder driver")
Tested-by: default avatarDeepChirp <DeepChirp@outlook.com>
Signed-off-by: default avatarAlice Ryhl <aliceryhl@google.com>
Acked-by: default avatarCarlos Llamas <cmllamas@google.com>
Link: https://patch.msgid.link/20251229-fda-zero-v1-1-58a41cb0e7ec@google.com


Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 63804fed

drivers/android/binder/thread.rs

+34 −25
Original line number Diff line number Diff line
@@ -69,17 +69,24 @@ struct ScatterGatherEntry { 
}



/// This entry specifies that a fixup should happen at `target_offset` of the

/// buffer. If `skip` is nonzero, then the fixup is a `binder_fd_array_object`

/// and is applied later. Otherwise if `skip` is zero, then the size of the

/// fixup is `sizeof::<u64>()` and `pointer_value` is written to the buffer.

struct PointerFixupEntry {

    /// The number of bytes to skip, or zero for a `binder_buffer_object` fixup.

    skip: usize,

    /// The translated pointer to write when `skip` is zero.

/// buffer.

enum PointerFixupEntry {

    /// A fixup for a `binder_buffer_object`.

    Fixup {

        /// The translated pointer to write.

        pointer_value: u64,

        /// The offset at which the value should be written. The offset is relative

        /// to the original buffer.

        target_offset: usize,

    },

    /// A skip for a `binder_fd_array_object`.

    Skip {

        /// The number of bytes to skip.

        skip: usize,

        /// The offset at which the skip should happen. The offset is relative

        /// to the original buffer.

        target_offset: usize,

    },

}



/// Return type of `apply_and_validate_fixup_in_parent`.
@@ -762,8 +769,7 @@ fn translate_object( 


                    parent_entry.fixup_min_offset = info.new_min_offset;

                    parent_entry.pointer_fixups.push(

                        PointerFixupEntry {

                            skip: 0,

                        PointerFixupEntry::Fixup {

                            pointer_value: buffer_ptr_in_user_space,

                            target_offset: info.target_offset,

                        },
@@ -807,9 +813,8 @@ fn translate_object( 
                parent_entry

                    .pointer_fixups

                    .push(

                        PointerFixupEntry {

                        PointerFixupEntry::Skip {

                            skip: fds_len,

                            pointer_value: 0,

                            target_offset: info.target_offset,

                        },

                        GFP_KERNEL,
@@ -871,17 +876,21 @@ fn apply_sg(&self, alloc: &mut Allocation, sg_state: &mut ScatterGatherState) -> 
            let mut reader =

                UserSlice::new(UserPtr::from_addr(sg_entry.sender_uaddr), sg_entry.length).reader();

            for fixup in &mut sg_entry.pointer_fixups {

                let fixup_len = if fixup.skip == 0 {

                    size_of::<u64>()

                } else {

                    fixup.skip

                let (fixup_len, fixup_offset) = match fixup {

                    PointerFixupEntry::Fixup { target_offset, .. } => {

                        (size_of::<u64>(), *target_offset)

                    }

                    PointerFixupEntry::Skip {

                        skip,

                        target_offset,

                    } => (*skip, *target_offset),

                };



                let target_offset_end = fixup.target_offset.checked_add(fixup_len).ok_or(EINVAL)?;

                if fixup.target_offset < end_of_previous_fixup || offset_end < target_offset_end {

                let target_offset_end = fixup_offset.checked_add(fixup_len).ok_or(EINVAL)?;

                if fixup_offset < end_of_previous_fixup || offset_end < target_offset_end {

                    pr_warn!(

                        "Fixups oob {} {} {} {}",

                        fixup.target_offset,

                        fixup_offset,

                        end_of_previous_fixup,

                        offset_end,

                        target_offset_end
@@ -890,13 +899,13 @@ fn apply_sg(&self, alloc: &mut Allocation, sg_state: &mut ScatterGatherState) -> 
                }



                let copy_off = end_of_previous_fixup;

                let copy_len = fixup.target_offset - end_of_previous_fixup;

                let copy_len = fixup_offset - end_of_previous_fixup;

                if let Err(err) = alloc.copy_into(&mut reader, copy_off, copy_len) {

                    pr_warn!("Failed copying into alloc: {:?}", err);

                    return Err(err.into());

                }

                if fixup.skip == 0 {

                    let res = alloc.write::<u64>(fixup.target_offset, &fixup.pointer_value);

                if let PointerFixupEntry::Fixup { pointer_value, .. } = fixup {

                    let res = alloc.write::<u64>(fixup_offset, pointer_value);

                    if let Err(err) = res {

                        pr_warn!("Failed copying ptr into alloc: {:?}", err);

                        return Err(err.into());