From a604403ba37d7517e876f17e18ff9e84ea0993c3 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Fri, 28 Apr 2023 17:31:27 +0200 Subject: [PATCH] Write stacks correctly during stack overflows When encountering a stack overflow we often crash accessing the guard page. The logic assumed that wherever the stack pointer was so was the stack, but this lead the writer to dump the guard page in these cases. This patch changes the logic to inspect the properties of the mapping that appears to correspond to the stack and - if it looks like a guard page - look for the actual stack instead. This fixes #24 --- Cargo.toml | 4 +-- src/linux/android.rs | 2 +- src/linux/maps_reader.rs | 59 ++++++++++++++++++++++++++-------- src/linux/ptrace_dumper.rs | 26 ++++++++++++--- tests/linux_minidump_writer.rs | 4 +-- tests/ptrace_dumper.rs | 2 +- 6 files changed, 71 insertions(+), 26 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 141d852e..c7682a6a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ edition = "2021" license = "MIT" [dependencies] +bitflags = "2.0" byteorder = "1.3.2" cfg-if = "1.0" crash-context = "0.6" @@ -31,9 +32,6 @@ nix = { version = "0.26", default-features = false, features = [ "user", ] } -[target.'cfg(target_os = "windows")'.dependencies] -bitflags = "2.0" - [target.'cfg(target_os = "macos")'.dependencies] # Binds some additional mac specifics not in libc mach2 = "0.4" diff --git a/src/linux/android.rs b/src/linux/android.rs index 0c8195cc..a1bba078 100644 --- a/src/linux/android.rs +++ b/src/linux/android.rs @@ -118,7 +118,7 @@ pub fn late_process_mappings(pid: Pid, mappings: &mut [MappingInfo]) -> Result<( // where the ELF header indicates a mapped shared library. for mut map in mappings .iter_mut() - .filter(|m| m.executable && m.name.as_ref().map_or(false, |n| n.starts_with("/"))) + .filter(|m| m.is_executable() && m.name.as_ref().map_or(false, |n| n.starts_with("/"))) { let ehdr_opt = PtraceDumper::copy_from_process( pid, diff --git a/src/linux/maps_reader.rs b/src/linux/maps_reader.rs index 8050387a..a8395e3c 100644 --- a/src/linux/maps_reader.rs +++ b/src/linux/maps_reader.rs @@ -18,6 +18,16 @@ pub struct SystemMappingInfo { pub end_address: usize, } +bitflags::bitflags! { + #[derive(Copy, Clone, Debug, Default, Eq, PartialEq)] + #[repr(transparent)] + pub struct MappingPermission: u8 { + const READABLE = 0b0001; + const WRITABLE = 0b0010; + const EXECUTABLE = 0b0100; + } +} + // One of these is produced for each mapping in the process (i.e. line in // /proc/$x/maps). #[derive(Debug, PartialEq, Eq, Clone)] @@ -34,8 +44,8 @@ pub struct MappingInfo { // address range. The following structure holds the original mapping // address range as reported by the operating system. pub system_mapping_info: SystemMappingInfo, - pub offset: usize, // offset into the backed file. - pub executable: bool, // true if the mapping has the execute bit set. + pub offset: usize, // offset into the backed file. + pub permissions: MappingPermission, // read, write and execute permissions. pub name: Option, // pub elf_obj: Option, } @@ -120,7 +130,11 @@ impl MappingInfo { let start_address = usize::from_str_radix(addresses.next().unwrap(), 16)?; let end_address = usize::from_str_radix(addresses.next().unwrap(), 16)?; - let executable = perms.contains('x'); + let mut permissions = MappingPermission::default(); + permissions.set(MappingPermission::EXECUTABLE, perms.contains('x')); + permissions.set(MappingPermission::READABLE, perms.contains('r')); + permissions.set(MappingPermission::WRITABLE, perms.contains('w')); + let permissions = permissions; // Only copy name if the name is a valid path name, or if // it's the VDSO image. @@ -140,7 +154,7 @@ impl MappingInfo { { module.system_mapping_info.end_address = end_address; module.size = end_address - module.start_address; - module.executable |= executable; + module.permissions |= permissions; return Ok(MappingInfoParsingResult::SkipLine); } } @@ -151,7 +165,7 @@ impl MappingInfo { // and which directly follow an executable mapping. let module_end_address = module.start_address + module.size; if (start_address == module_end_address) - && module.executable + && module.is_executable() && is_mapping_a_path(module.name.as_deref()) && (offset == 0 || offset == module_end_address) && perms == RESERVED_FLAGS @@ -173,7 +187,7 @@ impl MappingInfo { end_address, }, offset, - executable, + permissions, name, // elf_obj, }; @@ -315,7 +329,7 @@ impl MappingInfo { return Ok((file_path, file_name)); }; - if self.executable && self.offset != 0 { + if self.is_executable() && self.offset != 0 { // If an executable is mapped from a non-zero offset, this is likely because // the executable was loaded directly from inside an archive file (e.g., an // apk on Android). @@ -356,7 +370,7 @@ impl MappingInfo { self.name.is_some() && // Only want to include one mapping per shared lib. // Avoid filtering executable mappings. - (self.offset == 0 || self.executable) && + (self.offset == 0 || self.is_executable()) && // big enough to get a signature for. self.size >= 4096 } @@ -365,6 +379,18 @@ impl MappingInfo { self.system_mapping_info.start_address <= address && address < self.system_mapping_info.end_address } + + pub fn is_executable(&self) -> bool { + self.permissions.contains(MappingPermission::EXECUTABLE) + } + + pub fn is_readable(&self) -> bool { + self.permissions.contains(MappingPermission::READABLE) + } + + pub fn is_writable(&self) -> bool { + self.permissions.contains(MappingPermission::WRITABLE) + } } #[cfg(test)] @@ -453,7 +479,9 @@ mod tests { end_address: 0x559748406000, }, offset: 0, - executable: true, + permissions: MappingPermission::READABLE + | MappingPermission::WRITABLE + | MappingPermission::EXECUTABLE, name: Some("/usr/bin/cat".to_string()), }; @@ -467,7 +495,7 @@ mod tests { end_address: 0x559749b2f000, }, offset: 0, - executable: false, + permissions: MappingPermission::READABLE | MappingPermission::WRITABLE, name: Some("[heap]".to_string()), }; @@ -481,7 +509,7 @@ mod tests { end_address: 0x7efd968f5000, }, offset: 0, - executable: false, + permissions: MappingPermission::READABLE | MappingPermission::WRITABLE, name: None, }; @@ -500,7 +528,8 @@ mod tests { end_address: 0x7ffc6e0f9000, }, offset: 0, - executable: true, + permissions: MappingPermission::READABLE | MappingPermission::EXECUTABLE, + name: Some("linux-gate.so".to_string()), }; @@ -560,7 +589,9 @@ mod tests { end_address: 0x7efd96d8c000, // ..but this is not visible here }, offset: 0, - executable: true, + permissions: MappingPermission::READABLE + | MappingPermission::WRITABLE + | MappingPermission::EXECUTABLE, name: Some("/lib64/libc-2.32.so".to_string()), }; @@ -621,7 +652,7 @@ mod tests { end_address: 0x7f0b97b73000, }, offset: 0, - executable: true, + permissions: MappingPermission::READABLE | MappingPermission::EXECUTABLE, name: Some("libmozgtk.so".to_string()), }; diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index 2380afb0..bf581bb7 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -332,15 +332,31 @@ impl PtraceDumper { // NOTE: original code uses getpagesize(), which a) isn't there in Rust and // b) shouldn't be used, as its not portable (see man getpagesize) let page_size = nix::unistd::sysconf(nix::unistd::SysconfVar::PAGE_SIZE)? - .expect("page size apparently unlimited: doesn't make sense."); - let stack_pointer = int_stack_pointer & !(page_size as usize - 1); + .expect("page size apparently unlimited: doesn't make sense.") + as usize; + let stack_pointer = int_stack_pointer & !(page_size - 1); // The number of bytes of stack which we try to capture. let stack_to_capture = 32 * 1024; - let mapping = self + let mut mapping = self .find_mapping(stack_pointer) .ok_or(DumperError::NoStackPointerMapping)?; + + // If the mapping we found has no permissions then it's likely the + // guard page, try using the mapping right before it. The stack grows + // towards lower addresses on the platforms we care about so the stack + // should appear after the guard page. + if mapping.permissions.is_empty() { + // This has been true since Linux 4.12 + let max_guard_page_size = 1024 * 1024; + + let next_mapping_address = mapping.start_address + mapping.size; + if (next_mapping_address - stack_pointer) < max_guard_page_size { + mapping = self.find_mapping(next_mapping_address).unwrap_or(mapping); + } + } + let offset = stack_pointer - mapping.start_address; let distance_to_end = mapping.size - offset; let stack_len = std::cmp::min(distance_to_end, stack_to_capture); @@ -394,7 +410,7 @@ impl PtraceDumper { // bit, modulo the bitfield size, is not set then there does not // exist a mapping in mappings that would contain that pointer. for mapping in &self.mappings { - if !mapping.executable { + if !mapping.is_executable() { continue; } // For each mapping, work out the (unmodulo'ed) range of bits to @@ -441,7 +457,7 @@ impl PtraceDumper { let test = addr >> shift; if could_hit_mapping[(test >> 3) & array_mask] & (1 << (test & 7)) != 0 { if let Some(hit_mapping) = self.find_mapping_no_bias(addr) { - if hit_mapping.executable { + if hit_mapping.is_executable() { last_hit_mapping = Some(hit_mapping); continue; } diff --git a/tests/linux_minidump_writer.rs b/tests/linux_minidump_writer.rs index cd2ca737..cf24e0e2 100644 --- a/tests/linux_minidump_writer.rs +++ b/tests/linux_minidump_writer.rs @@ -7,7 +7,7 @@ use minidump_writer::{ app_memory::AppMemory, crash_context::CrashContext, errors::*, - maps_reader::{MappingEntry, MappingInfo, SystemMappingInfo}, + maps_reader::{MappingEntry, MappingInfo, MappingPermission, SystemMappingInfo}, minidump_writer::MinidumpWriter, ptrace_dumper::PtraceDumper, thread_info::Pid, @@ -132,7 +132,7 @@ fn test_write_and_read_dump_from_parent_helper(context: Context) { start_address: mmap_addr, size: memory_size, offset: 0, - executable: false, + permissions: MappingPermission::READABLE | MappingPermission::WRITABLE, name: Some("a fake mapping".to_string()), system_mapping_info: SystemMappingInfo { start_address: mmap_addr, diff --git a/tests/ptrace_dumper.rs b/tests/ptrace_dumper.rs index 02ba836a..463fdc25 100644 --- a/tests/ptrace_dumper.rs +++ b/tests/ptrace_dumper.rs @@ -259,7 +259,7 @@ fn test_sanitize_stack_copy() { let mapping_info = dumper .find_mapping_no_bias(instr_ptr) .expect("Failed to find mapping info"); - assert!(mapping_info.executable); + assert!(mapping_info.is_executable()); // Pointers to code shouldn't be sanitized. simulated_stack = vec![0u8; 2 * size_of::()];