drivers/android/binder/thread.rs | 59 +++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 25 deletions(-)
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,
--
Alice Ryhl <aliceryhl@google.com>On Mon, Dec 29, 2025 at 03:38:14PM +0000, Alice Ryhl 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 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>
> ---
Acked-by: Carlos Llamas <cmllamas@google.com>
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
© 2016 - 2026 Red Hat, Inc.