Skip to content

Commit

Permalink
Perform a checked add in the ptrace dumper ModuleMemory impl.
Browse files Browse the repository at this point in the history
This also bumps the version of `goblin` to at least 0.8.2, which allows
us to clean up some code.
  • Loading branch information
afranchuk committed Aug 8, 2024
1 parent 6f93cb2 commit 3db68c6
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 23 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ thiserror = "1.0"

[target.'cfg(unix)'.dependencies]
libc = "0.2"
goblin = "0.8"
goblin = "0.8.2"
memmap2 = "0.9"

[target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies]
Expand Down
47 changes: 31 additions & 16 deletions src/linux/module_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,20 @@ impl<'a> ModuleMemory for &'a [u8] {
type Memory = Self;

fn read_module_memory(&self, offset: u64, length: u64) -> std::io::Result<Self::Memory> {
self.get(offset as usize..(offset + length) as usize)
.ok_or_else(|| {
std::io::Error::new(
std::io::ErrorKind::UnexpectedEof,
format!("{} out of bounds", offset + length),
)
})
let end = offset.checked_add(length).ok_or_else(|| {
std::io::Error::new(
std::io::ErrorKind::Other,
format!(
"integer overflow calculating end address (offset={offset}, length={length})"
),
)
})?;
self.get(offset as usize..end as usize).ok_or_else(|| {
std::io::Error::new(
std::io::ErrorKind::UnexpectedEof,
format!("{offset}..{end} out of bounds (length={})", self.len()),
)
})
}
}

Expand Down Expand Up @@ -266,7 +273,12 @@ impl<T: ModuleMemory> ModuleReader<T> {
}
}
}
self.read_name_from_strtab(addr, size, offset)
if offset < size {
self.read_name_from_strtab(addr, size, offset)
} else {
log::warn!("soname strtab offset ({offset}) exceeds strtab size ({size})");
Err(Error::NoSoNameEntry)
}
}
}
}
Expand Down Expand Up @@ -308,6 +320,11 @@ impl<T: ModuleMemory> ModuleReader<T> {
dynstr_section_header.sh_size,
name_offset,
);
} else {
log::warn!(
"soname offset ({name_offset}) exceeds dynstr section size ({})",
dynstr_section_header.sh_size
);
}
}
}
Expand Down Expand Up @@ -382,6 +399,7 @@ impl<T: ModuleMemory> ModuleReader<T> {
strtab_size: u64,
name_offset: u64,
) -> Result<String, Error> {
assert!(name_offset < strtab_size);
let name = read(
&self.module_memory,
strtab_offset + name_offset,
Expand Down Expand Up @@ -423,18 +441,15 @@ impl<T: ModuleMemory> ModuleReader<T> {
return Err(Error::NoSections);
}

// FIXME Until a version following goblin 0.8.0 is published (with
// `SectionHeader::parse_from`), we read one extra byte preceding the sections so that
// `SectionHeader::parse` doesn't return immediately due to a 0 offset.

let section_headers_data = read(
&self.module_memory,
self.header.e_shoff - 1,
self.header.e_shentsize as u64 * self.header.e_shnum as u64 + 1,
self.header.e_shoff,
self.header.e_shentsize as u64 * self.header.e_shnum as u64,
)?;
let section_headers = elf::SectionHeader::parse(
// Use `parse_from` rather than `parse`, which allows a 0 offset.
let section_headers = elf::SectionHeader::parse_from(
&section_headers_data,
1,
0,
self.header.e_shnum as usize,
self.context,
)?;
Expand Down
14 changes: 8 additions & 6 deletions src/linux/ptrace_dumper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -570,12 +570,14 @@ impl PtraceDumper {
length: u64,
) -> std::io::Result<Self::Memory> {
// Leave bounds checks to `copy_from_process`
PtraceDumper::copy_from_process(
self.pid,
(self.start_address + offset) as _,
length as usize,
)
.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))
let addr = self.start_address.checked_add(offset).ok_or_else(|| {
std::io::Error::new(
std::io::ErrorKind::Other,
format!("integer overflow calculating address: (start_address={}, offset={offset})", self.start_address),
)
})?;
PtraceDumper::copy_from_process(self.pid, addr as _, length as usize)
.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))
}

fn base_address(&self) -> Option<u64> {
Expand Down

0 comments on commit 3db68c6

Please sign in to comment.