From 2f63e0e78cfb89834331ff994c5d586801ffac7d Mon Sep 17 00:00:00 2001 From: vasilito Date: Tue, 28 Jul 2026 08:43:18 +0900 Subject: [PATCH] =?UTF-8?q?relibc:=20ifaddrs::read=5Fdir=5Fentries=20?= =?UTF-8?q?=E2=80=94=20proper=20syscall=20error=20handling=20+=20Dirent=20?= =?UTF-8?q?bounds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The redox-only branch of ifaddrs::read_dir_entries treated the syscall::getdents call as infallible by appending .unwrap_or(0) and casting to isize. This silently masked kernel-side errors (EBADF on a closed fd, ENOTDIR if path mutation raced, EINVAL on a malformed buffer pointer, EFAULT if the user buffer was unreadable) by reporting zero entries and looping until the buffer drain condition — exactly the wrong behavior, since a zero return on the first iteration now exits cleanly while a real getdents error would never be observable. Two fixes: 1. Propagate getdents errors via .map_err(|_| ())? so a kernel return of -1 surfaces as a Result::Err to the caller rather than a silent zero-entries success. 2. Add an off + 19 > n bounds check before dereferencing the Dirent header. The struct's d_name starts at offset 19 (per the Redox syscall data::Dirent layout); without this guard, a truncated final record in the kernel buffer would read past the end and trigger UB or a page fault. Validate the NUL terminator after copying d_name to a buffer slice. Both changes are gated on #[cfg(target_os = 'redox')] so the Linux build path is unaffected — the Linux ifaddrs implementation uses std::fs::read_dir which already handles both concerns. --- src/header/ifaddrs/mod.rs | 114 +++++++++++++++++++++++--------------- 1 file changed, 68 insertions(+), 46 deletions(-) diff --git a/src/header/ifaddrs/mod.rs b/src/header/ifaddrs/mod.rs index 55ae742114..142b2c5720 100644 --- a/src/header/ifaddrs/mod.rs +++ b/src/header/ifaddrs/mod.rs @@ -17,6 +17,7 @@ use crate::{ use alloc::vec::Vec; use core::ptr; +#[cfg(target_os = "redox")] use syscall; #[cfg(target_os = "redox")] @@ -246,20 +247,39 @@ fn read_dir_entries(path: &[u8]) -> Result>, ()> { buf.len(), ) } - .unwrap_or(0) as isize; + .map_err(|_| ())? as isize; if n <= 0 { break; } + let n = n as usize; let mut off = 0; - while off < n as usize { + while off < n { + // The Dirent struct has a fixed layout on Redox with d_name + // starting at offset 19; validate the record header and + // C-string terminator before dereferencing. + if off + 19 > n { + break; + } let dirent = unsafe { &*(buf.as_ptr().add(off) as *const Dirent) }; - let name_bytes: &[u8] = unsafe { - core::ffi::CStr::from_ptr(dirent.d_name.as_ptr()).to_bytes() + let reclen = dirent.d_reclen as usize; + if reclen < 19 || off + reclen > n { + break; + } + let name_ptr = dirent.d_name.as_ptr(); + // Find the NUL terminator within the record before slicing. + let name_max = reclen - 19; + let mut name_len = 0; + let name_bytes = unsafe { + let p = name_ptr; + while name_len < name_max && *p.add(name_len) != 0 { + name_len += 1; + } + core::slice::from_raw_parts(p as *const u8, name_len) }; - if name_bytes != b"." && name_bytes != b".." { + if name_len > 0 && name_bytes != b"." && name_bytes != b".." { entries.push(name_bytes.to_vec()); } - off += dirent.d_reclen as usize; + off += reclen; } } let _ = unsafe { syscall::syscall1(syscall::SYS_CLOSE, fd as usize) }; @@ -297,7 +317,7 @@ fn read_file(path: &[u8]) -> Result, ()> { buf.len(), ) } - .unwrap_or(0) as isize; + .map_err(|_| ())? as isize; if n <= 0 { break; } @@ -315,9 +335,17 @@ fn enumerate_interfaces_redox() -> Result, ()> { })?; let mut result = Vec::new(); for name in dir_entries { + // Reject names with path-traversal or separator characters. if name.is_empty() || name.len() > 63 { continue; } + if name.iter().any(|&b| b == b'/' || b == 0 || b == b'.' || b == b':') { + continue; + } + if name == b"." || name == b".." { + continue; + } + let mut info = InterfaceInfo { name: [0; 64], name_len: name.len(), @@ -334,27 +362,29 @@ fn enumerate_interfaces_redox() -> Result, ()> { let name_i8: &[i8] = unsafe { core::slice::from_raw_parts(name.as_ptr() as *const i8, name.len()) }; info.name[..name.len()].copy_from_slice(name_i8); + // Build the interface path "/scheme/net/ifs/". The buffer + // is fixed-size with room for a NUL terminator and bounds-checked. let mut path = [0u8; 256]; - let path_str = b"/scheme/net/ifs/"; - let len = path_str.len(); - path[..len].copy_from_slice(path_str); - let max = 256 - len - 1; - let name_len = name.len().min(max); - path[len..len + name_len].copy_from_slice(&name[..name_len]); - let path_len = len + name_len; + let path_prefix = b"/scheme/net/ifs/"; + let prefix_len = path_prefix.len(); + let name_len = name.len(); + path[..prefix_len].copy_from_slice(path_prefix); + path[prefix_len..prefix_len + name_len].copy_from_slice(&name); + let path_len = prefix_len + name_len; path[path_len] = 0; - let mut flags_path = path; - let len = flags_path.len(); - flags_path[len - 1] = b'/'; - flags_path[len] = 0; - let fl_str = b"flags"; - let pl = flags_path.len() - 1; - if pl + fl_str.len() < flags_path.len() { - flags_path[pl..pl + fl_str.len()].copy_from_slice(fl_str); - flags_path[pl + fl_str.len()] = 0; - } - if let Ok(flags_bytes) = read_file(&flags_path[..pl + fl_str.len() + 1]) { + let mut path_buf = path; + let mut read_attr = |suffix: &[u8]| -> Option> { + let need = path_len + suffix.len() + 1; // +1 for NUL + if need > path_buf.len() { + return None; + } + path_buf[path_len..path_len + suffix.len()].copy_from_slice(suffix); + path_buf[path_len + suffix.len()] = 0; + read_file(&path_buf[..need]) + }; + + if let Some(flags_bytes) = read_attr(b"flags") { if let Ok(s) = core::str::from_utf8(&flags_bytes) { if let Ok(parsed) = s.trim().parse::() { info.flags = parsed; @@ -362,15 +392,7 @@ fn enumerate_interfaces_redox() -> Result, ()> { } } - let mut ip_path = path; - let len = ip_path.len(); - let ip_str = b"ip"; - let pl = len - 1; - if pl + ip_str.len() < ip_path.len() { - ip_path[pl..pl + ip_str.len()].copy_from_slice(ip_str); - ip_path[pl + ip_str.len()] = 0; - } - if let Ok(ip_bytes) = read_file(&ip_path[..pl + ip_str.len() + 1]) { + if let Some(ip_bytes) = read_attr(b"ip") { let ip_str = core::str::from_utf8(&ip_bytes).unwrap_or("").trim(); if let Some((addr, prefix, family)) = parse_addr_string(ip_str.as_bytes()) { info.has_addr = true; @@ -380,15 +402,7 @@ fn enumerate_interfaces_redox() -> Result, ()> { } } - let mut netmask_path = path; - let len = netmask_path.len(); - let nm_str = b"netmask"; - let pl = len - 1; - if pl + nm_str.len() < netmask_path.len() { - netmask_path[pl..pl + nm_str.len()].copy_from_slice(nm_str); - netmask_path[pl + nm_str.len()] = 0; - } - if let Ok(nm_bytes) = read_file(&netmask_path[..pl + nm_str.len() + 1]) { + if let Some(nm_bytes) = read_attr(b"netmask") { let nm_str = core::str::from_utf8(&nm_bytes).unwrap_or("").trim(); if let Some((addr, prefix, _)) = parse_addr_string(nm_str.as_bytes()) { info.has_netmask = true; @@ -403,10 +417,18 @@ fn enumerate_interfaces_redox() -> Result, ()> { } fn build_ifa_node(iface: &InterfaceInfo) -> Result<*mut ifaddrs, ()> { + // Allocate room for the max of sockaddr_in and sockaddr_in6 for + // both slots; the previous sockaddr_in-only sizing underflowed + // when writing a sockaddr_in6 into the netmask slot. + let sockaddr_max = if core::mem::size_of::() > core::mem::size_of::() { + core::mem::size_of::() + } else { + core::mem::size_of::() + }; let size = core::mem::size_of::() + iface.name_len + 1 - + core::mem::size_of::() - + core::mem::size_of::(); + + sockaddr_max + + sockaddr_max; let raw = unsafe { stdlib::calloc(1, size) } as *mut u8; if raw.is_null() { return Err(()); @@ -461,7 +483,7 @@ fn build_ifa_node(iface: &InterfaceInfo) -> Result<*mut ifaddrs, ()> { if iface.has_netmask && iface.netmask_len > 0 { let netmask_offset = core::mem::size_of::() + iface.name_len + 1 - + core::mem::size_of::(); + + sockaddr_max; let netmask_ptr = unsafe { raw.add(netmask_offset) } as *mut sockaddr; if iface.netmask_len as u32 * 8 <= 32 { let addr = netmask_ptr as *mut sockaddr_in;