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: DeepChirp DeepChirp@outlook.com Closes: https://github.com/waydroid/waydroid/issues/2157 Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver") Tested-by: DeepChirp DeepChirp@outlook.com Signed-off-by: Alice Ryhl aliceryhl@google.com --- drivers/android/binder/thread.rs | 59 +++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 25 deletions(-)
diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs index 1a8e6fdc0dc42369ee078e720aa02b2554fb7332..dcd47e10aeb8c748d04320fbbe15ad35201684b9 100644 --- a/drivers/android/binder/thread.rs +++ b/drivers/android/binder/thread.rs @@ -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. - pointer_value: u64, - /// The offset at which the value should be written. The offset is relative - /// to the original buffer. - target_offset: usize, +/// 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());
--- base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8 change-id: 20251229-fda-zero-4e46e56be58d
Best regards,
On Mon, 29 Dec 2025 15:38:14 +0000 Alice Ryhl aliceryhl@google.com wrote:
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 was curious and checked the C binder implementation. Apparently the C binder implementation returns early when translating a FD array with length 0.
Would it still make sense to do something similar in the Rust binder? The enum change is still good to make, though.
Best, Gary
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: DeepChirp DeepChirp@outlook.com Closes: https://github.com/waydroid/waydroid/issues/2157 Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver") Tested-by: DeepChirp DeepChirp@outlook.com Signed-off-by: Alice Ryhl aliceryhl@google.com
On Mon, Dec 29, 2025 at 04:45:44PM +0000, Gary Guo wrote:
On Mon, 29 Dec 2025 15:38:14 +0000 Alice Ryhl aliceryhl@google.com wrote:
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 was curious and checked the C binder implementation. Apparently the C binder implementation returns early when translating a FD array with length 0.
Would it still make sense to do something similar in the Rust binder? The enum change is still good to make, though.
Based on where the early return is, that'd be equivalent in wrapping this:
parent_entry .pointer_fixups .push( PointerFixupEntry::Skip { skip: fds_len, target_offset: info.target_offset, }, GFP_KERNEL, ) .map_err(|_| ENOMEM)?;
in an `if fds_len > 0 {}` block. I don't believe it makes any difference, but not having a special case may be cleaner?
Alice
linux-stable-mirror@lists.linaro.org