Skip to content

Commit d047248

Browse files
Darksonngregkh
authored andcommitted
rust_binder: add additional alignment checks
This adds some alignment checks to match C Binder more closely. This causes the driver to reject more transactions. I don't think any of the transactions in question are harmful, but it's still a bug because it's the wrong uapi to accept them. The cases where usize is changed for u64, it will affect only 32-bit kernels. Cc: stable@vger.kernel.org Fixes: eafedbc ("rust_binder: add Rust Binder driver") Signed-off-by: Alice Ryhl <aliceryhl@google.com> Acked-by: Carlos Llamas <cmllamas@google.com> Link: https://patch.msgid.link/20260123-binder-alignment-more-checks-v1-1-7e1cea77411d@google.com Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent 5e8a3d0 commit d047248

1 file changed

Lines changed: 36 additions & 14 deletions

File tree

drivers/android/binder/thread.rs

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ use core::{
3939
sync::atomic::{AtomicU32, Ordering},
4040
};
4141

42+
fn is_aligned(value: usize, to: usize) -> bool {
43+
value % to == 0
44+
}
45+
4246
/// Stores the layout of the scatter-gather entries. This is used during the `translate_objects`
4347
/// call and is discarded when it returns.
4448
struct ScatterGatherState {
@@ -795,6 +799,10 @@ impl Thread {
795799
let num_fds = usize::try_from(obj.num_fds).map_err(|_| EINVAL)?;
796800
let fds_len = num_fds.checked_mul(size_of::<u32>()).ok_or(EINVAL)?;
797801

802+
if !is_aligned(parent_offset, size_of::<u32>()) {
803+
return Err(EINVAL.into());
804+
}
805+
798806
let info = sg_state.validate_parent_fixup(parent_index, parent_offset, fds_len)?;
799807
view.alloc.info_add_fd_reserve(num_fds)?;
800808

@@ -809,6 +817,10 @@ impl Thread {
809817
}
810818
};
811819

820+
if !is_aligned(parent_entry.sender_uaddr, size_of::<u32>()) {
821+
return Err(EINVAL.into());
822+
}
823+
812824
parent_entry.fixup_min_offset = info.new_min_offset;
813825
parent_entry
814826
.pointer_fixups
@@ -825,6 +837,7 @@ impl Thread {
825837
.sender_uaddr
826838
.checked_add(parent_offset)
827839
.ok_or(EINVAL)?;
840+
828841
let mut fda_bytes = KVec::new();
829842
UserSlice::new(UserPtr::from_addr(fda_uaddr as _), fds_len)
830843
.read_all(&mut fda_bytes, GFP_KERNEL)?;
@@ -958,25 +971,30 @@ impl Thread {
958971

959972
let data_size = trd.data_size.try_into().map_err(|_| EINVAL)?;
960973
let aligned_data_size = ptr_align(data_size).ok_or(EINVAL)?;
961-
let offsets_size = trd.offsets_size.try_into().map_err(|_| EINVAL)?;
962-
let aligned_offsets_size = ptr_align(offsets_size).ok_or(EINVAL)?;
963-
let buffers_size = tr.buffers_size.try_into().map_err(|_| EINVAL)?;
964-
let aligned_buffers_size = ptr_align(buffers_size).ok_or(EINVAL)?;
974+
let offsets_size: usize = trd.offsets_size.try_into().map_err(|_| EINVAL)?;
975+
let buffers_size: usize = tr.buffers_size.try_into().map_err(|_| EINVAL)?;
965976
let aligned_secctx_size = match secctx.as_ref() {
966977
Some((_offset, ctx)) => ptr_align(ctx.len()).ok_or(EINVAL)?,
967978
None => 0,
968979
};
969980

981+
if !is_aligned(offsets_size, size_of::<u64>()) {
982+
return Err(EINVAL.into());
983+
}
984+
if !is_aligned(buffers_size, size_of::<u64>()) {
985+
return Err(EINVAL.into());
986+
}
987+
970988
// This guarantees that at least `sizeof(usize)` bytes will be allocated.
971989
let len = usize::max(
972990
aligned_data_size
973-
.checked_add(aligned_offsets_size)
974-
.and_then(|sum| sum.checked_add(aligned_buffers_size))
991+
.checked_add(offsets_size)
992+
.and_then(|sum| sum.checked_add(buffers_size))
975993
.and_then(|sum| sum.checked_add(aligned_secctx_size))
976994
.ok_or(ENOMEM)?,
977-
size_of::<usize>(),
995+
size_of::<u64>(),
978996
);
979-
let secctx_off = aligned_data_size + aligned_offsets_size + aligned_buffers_size;
997+
let secctx_off = aligned_data_size + offsets_size + buffers_size;
980998
let mut alloc =
981999
match to_process.buffer_alloc(debug_id, len, is_oneway, self.process.task.pid()) {
9821000
Ok(alloc) => alloc,
@@ -1008,13 +1026,13 @@ impl Thread {
10081026
}
10091027

10101028
let offsets_start = aligned_data_size;
1011-
let offsets_end = aligned_data_size + aligned_offsets_size;
1029+
let offsets_end = aligned_data_size + offsets_size;
10121030

10131031
// This state is used for BINDER_TYPE_PTR objects.
10141032
let sg_state = sg_state.insert(ScatterGatherState {
10151033
unused_buffer_space: UnusedBufferSpace {
10161034
offset: offsets_end,
1017-
limit: len,
1035+
limit: offsets_end + buffers_size,
10181036
},
10191037
sg_entries: KVec::new(),
10201038
ancestors: KVec::new(),
@@ -1023,12 +1041,16 @@ impl Thread {
10231041
// Traverse the objects specified.
10241042
let mut view = AllocationView::new(&mut alloc, data_size);
10251043
for (index, index_offset) in (offsets_start..offsets_end)
1026-
.step_by(size_of::<usize>())
1044+
.step_by(size_of::<u64>())
10271045
.enumerate()
10281046
{
1029-
let offset = view.alloc.read(index_offset)?;
1047+
let offset: usize = view
1048+
.alloc
1049+
.read::<u64>(index_offset)?
1050+
.try_into()
1051+
.map_err(|_| EINVAL)?;
10301052

1031-
if offset < end_of_previous_object {
1053+
if offset < end_of_previous_object || !is_aligned(offset, size_of::<u32>()) {
10321054
pr_warn!("Got transaction with invalid offset.");
10331055
return Err(EINVAL.into());
10341056
}
@@ -1060,7 +1082,7 @@ impl Thread {
10601082
}
10611083

10621084
// Update the indexes containing objects to clean up.
1063-
let offset_after_object = index_offset + size_of::<usize>();
1085+
let offset_after_object = index_offset + size_of::<u64>();
10641086
view.alloc
10651087
.set_info_offsets(offsets_start..offset_after_object);
10661088
}

0 commit comments

Comments
 (0)