Skip to content

Commit 0fdf41d

Browse files
Darksonnheftig
authored andcommitted
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: DeepChirp <DeepChirp@outlook.com> Closes: waydroid/waydroid#2157 Fixes: eafedbc ("rust_binder: add Rust Binder driver") Tested-by: DeepChirp <DeepChirp@outlook.com> Signed-off-by: Alice Ryhl <aliceryhl@google.com> Cherry-picked-for: https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/issues/173
1 parent ed4bfee commit 0fdf41d

File tree

1 file changed

+34
-25
lines changed

1 file changed

+34
-25
lines changed

drivers/android/binder/thread.rs

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -69,17 +69,24 @@ struct ScatterGatherEntry {
6969
}
7070

7171
/// This entry specifies that a fixup should happen at `target_offset` of the
72-
/// buffer. If `skip` is nonzero, then the fixup is a `binder_fd_array_object`
73-
/// and is applied later. Otherwise if `skip` is zero, then the size of the
74-
/// fixup is `sizeof::<u64>()` and `pointer_value` is written to the buffer.
75-
struct PointerFixupEntry {
76-
/// The number of bytes to skip, or zero for a `binder_buffer_object` fixup.
77-
skip: usize,
78-
/// The translated pointer to write when `skip` is zero.
79-
pointer_value: u64,
80-
/// The offset at which the value should be written. The offset is relative
81-
/// to the original buffer.
82-
target_offset: usize,
72+
/// buffer.
73+
enum PointerFixupEntry {
74+
/// A fixup for a `binder_buffer_object`.
75+
Fixup {
76+
/// The translated pointer to write.
77+
pointer_value: u64,
78+
/// The offset at which the value should be written. The offset is relative
79+
/// to the original buffer.
80+
target_offset: usize,
81+
},
82+
/// A skip for a `binder_fd_array_object`.
83+
Skip {
84+
/// The number of bytes to skip.
85+
skip: usize,
86+
/// The offset at which the skip should happen. The offset is relative
87+
/// to the original buffer.
88+
target_offset: usize,
89+
},
8390
}
8491

8592
/// Return type of `apply_and_validate_fixup_in_parent`.
@@ -762,8 +769,7 @@ impl Thread {
762769

763770
parent_entry.fixup_min_offset = info.new_min_offset;
764771
parent_entry.pointer_fixups.push(
765-
PointerFixupEntry {
766-
skip: 0,
772+
PointerFixupEntry::Fixup {
767773
pointer_value: buffer_ptr_in_user_space,
768774
target_offset: info.target_offset,
769775
},
@@ -807,9 +813,8 @@ impl Thread {
807813
parent_entry
808814
.pointer_fixups
809815
.push(
810-
PointerFixupEntry {
816+
PointerFixupEntry::Skip {
811817
skip: fds_len,
812-
pointer_value: 0,
813818
target_offset: info.target_offset,
814819
},
815820
GFP_KERNEL,
@@ -871,17 +876,21 @@ impl Thread {
871876
let mut reader =
872877
UserSlice::new(UserPtr::from_addr(sg_entry.sender_uaddr), sg_entry.length).reader();
873878
for fixup in &mut sg_entry.pointer_fixups {
874-
let fixup_len = if fixup.skip == 0 {
875-
size_of::<u64>()
876-
} else {
877-
fixup.skip
879+
let (fixup_len, fixup_offset) = match fixup {
880+
PointerFixupEntry::Fixup { target_offset, .. } => {
881+
(size_of::<u64>(), *target_offset)
882+
}
883+
PointerFixupEntry::Skip {
884+
skip,
885+
target_offset,
886+
} => (*skip, *target_offset),
878887
};
879888

880-
let target_offset_end = fixup.target_offset.checked_add(fixup_len).ok_or(EINVAL)?;
881-
if fixup.target_offset < end_of_previous_fixup || offset_end < target_offset_end {
889+
let target_offset_end = fixup_offset.checked_add(fixup_len).ok_or(EINVAL)?;
890+
if fixup_offset < end_of_previous_fixup || offset_end < target_offset_end {
882891
pr_warn!(
883892
"Fixups oob {} {} {} {}",
884-
fixup.target_offset,
893+
fixup_offset,
885894
end_of_previous_fixup,
886895
offset_end,
887896
target_offset_end
@@ -890,13 +899,13 @@ impl Thread {
890899
}
891900

892901
let copy_off = end_of_previous_fixup;
893-
let copy_len = fixup.target_offset - end_of_previous_fixup;
902+
let copy_len = fixup_offset - end_of_previous_fixup;
894903
if let Err(err) = alloc.copy_into(&mut reader, copy_off, copy_len) {
895904
pr_warn!("Failed copying into alloc: {:?}", err);
896905
return Err(err.into());
897906
}
898-
if fixup.skip == 0 {
899-
let res = alloc.write::<u64>(fixup.target_offset, &fixup.pointer_value);
907+
if let PointerFixupEntry::Fixup { pointer_value, .. } = fixup {
908+
let res = alloc.write::<u64>(fixup_offset, pointer_value);
900909
if let Err(err) = res {
901910
pr_warn!("Failed copying ptr into alloc: {:?}", err);
902911
return Err(err.into());

0 commit comments

Comments
 (0)