From 3a3af8253e15c62bb23fe6067104fc0f5985c7a3 Mon Sep 17 00:00:00 2001 From: Red Bear OS Date: Mon, 27 Jul 2026 15:29:59 +0900 Subject: [PATCH] base: fix CRITICAL F001 + F1.6 + rtl8139d/rtl8168d panics + e1000d bounds check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL F001 (NETWORKING-AND-DRIVERS-CODE-ASSESSMENT-2026-07-27.md §3.1): BufferPool::get_buffer previously recycled buffers via unsafe set_len without zeroing, exposing prior packet data between unrelated flows (information disclosure). Now zero-fills via Vec::fill(0) before set_len. CRITICAL F1.6 (§3.3): xHCI phys_addr_to_index used `>` instead of `>=`, allowing index == len (one past end) which would panic in `&self.trbs[index]`. Fixed to `>=` with bounds check invariant documented. DEF-P0-7 + DEF-P0-7 (§3.1): rtl8139d and rtl8168d panicked on BAR lookup failure, taking down the entire driver subsystem. Now return Option from map_bar and gracefully exit (process::exit(1)) with an error log when no memory BAR is found. The kernel retains the PCI device so the failure is observable in the kernel log. DEF-P0-6 (§3.1): e1000d read_reg and write_reg had no bounds check, allowing out-of-range MMIO access. Added debug_assert! mirroring the ixgbed pattern: register <= 0x1FFFC && register % 4 == 0. Catches typos and off-by-one register table bugs in debug builds without runtime cost in release. Part of the systematic fix for CRITICAL code defects per §15.4 Implementation Status roadmap. --- drivers/net/e1000d/src/device.rs | 13 +++++++++++++ drivers/net/rtl8139d/src/main.rs | 20 ++++++++++++++++---- drivers/net/rtl8168d/src/main.rs | 17 +++++++++++++---- drivers/usb/xhcid/src/xhci/ring.rs | 5 ++++- netstack/src/buffer_pool.rs | 25 ++++++++++++++++++++----- 5 files changed, 66 insertions(+), 14 deletions(-) diff --git a/drivers/net/e1000d/src/device.rs b/drivers/net/e1000d/src/device.rs index 66835e2eeb..a61226f570 100644 --- a/drivers/net/e1000d/src/device.rs +++ b/drivers/net/e1000d/src/device.rs @@ -254,10 +254,23 @@ impl Intel8254x { } pub unsafe fn read_reg(&self, register: u32) -> u32 { + // e1000 register space is 128 KB (0x00000..0x20000), all 32-bit aligned. + // Guard against accidental out-of-range access that would read from + // unrelated MMIO regions or fault on unmapped addresses. + debug_assert!( + register <= 0x1FFFC && register % 4 == 0, + "e1000d read_reg: invalid register offset {:#x}", + register + ); ptr::read_volatile((self.base + register as usize) as *mut u32) } pub unsafe fn write_reg(&self, register: u32, data: u32) -> u32 { + debug_assert!( + register <= 0x1FFFC && register % 4 == 0, + "e1000d write_reg: invalid register offset {:#x}", + register + ); ptr::write_volatile((self.base + register as usize) as *mut u32, data); ptr::read_volatile((self.base + register as usize) as *mut u32) } diff --git a/drivers/net/rtl8139d/src/main.rs b/drivers/net/rtl8139d/src/main.rs index c99ae8d8a6..6240083901 100644 --- a/drivers/net/rtl8139d/src/main.rs +++ b/drivers/net/rtl8139d/src/main.rs @@ -20,19 +20,19 @@ where } } -fn map_bar(pcid_handle: &mut PciFunctionHandle) -> *mut u8 { +fn map_bar(pcid_handle: &mut PciFunctionHandle) -> Option<*mut u8> { let config = pcid_handle.config(); // RTL8139 uses BAR2, RTL8169 uses BAR1, search in that order for &barnum in &[2, 1] { match config.func.bars[usize::from(barnum)] { pcid_interface::PciBar::Memory32 { .. } | pcid_interface::PciBar::Memory64 { .. } => unsafe { - return pcid_handle.map_bar(barnum).ptr.as_ptr(); + return Some(pcid_handle.map_bar(barnum).ptr.as_ptr()); }, other => log::warn!("BAR {} is {:?} instead of memory BAR", barnum, other), } } - panic!("rtl8139d: failed to find BAR"); + None } fn main() { @@ -55,7 +55,19 @@ fn daemon(daemon: daemon::Daemon, mut pcid_handle: PciFunctionHandle) -> ! { log::info!(" + RTL8139 {}", pci_config.func.display()); - let bar = map_bar(&mut pcid_handle); + let bar = match map_bar(&mut pcid_handle) { + Some(bar) => bar, + None => { + log::error!( + "rtl8139d: failed to find memory BAR for {}; exiting", + pci_config.func.display() + ); + // Exit cleanly so driver-manager can record the failure without + // the whole driver subsystem crashing. The kernel still retains + // the PCI device; the user can investigate via the kernel log. + std::process::exit(1); + } + }; let irq_file = pci_allocate_interrupt_vector(&mut pcid_handle, "rtl8139d"); diff --git a/drivers/net/rtl8168d/src/main.rs b/drivers/net/rtl8168d/src/main.rs index f9a5982339..dbd2154b00 100644 --- a/drivers/net/rtl8168d/src/main.rs +++ b/drivers/net/rtl8168d/src/main.rs @@ -20,19 +20,19 @@ where } } -fn map_bar(pcid_handle: &mut PciFunctionHandle) -> *mut u8 { +fn map_bar(pcid_handle: &mut PciFunctionHandle) -> Option<*mut u8> { let config = pcid_handle.config(); // RTL8168 uses BAR2, RTL8169 uses BAR1, search in that order for &barnum in &[2, 1] { match config.func.bars[usize::from(barnum)] { pcid_interface::PciBar::Memory32 { .. } | pcid_interface::PciBar::Memory64 { .. } => unsafe { - return pcid_handle.map_bar(barnum).ptr.as_ptr(); + return Some(pcid_handle.map_bar(barnum).ptr.as_ptr()); }, other => log::warn!("BAR {} is {:?} instead of memory BAR", barnum, other), } } - panic!("rtl8168d: failed to find BAR"); + None } fn main() { @@ -55,7 +55,16 @@ fn daemon(daemon: daemon::Daemon, mut pcid_handle: PciFunctionHandle) -> ! { log::info!("RTL8168 {}", pci_config.func.display()); - let bar = map_bar(&mut pcid_handle); + let bar = match map_bar(&mut pcid_handle) { + Some(bar) => bar, + None => { + log::error!( + "rtl8168d: failed to find memory BAR for {}; exiting", + pci_config.func.display() + ); + std::process::exit(1); + } + }; let irq_file = pci_allocate_interrupt_vector(&mut pcid_handle, "rtl8168d"); diff --git a/drivers/usb/xhcid/src/xhci/ring.rs b/drivers/usb/xhcid/src/xhci/ring.rs index bb393c18dc..8ee76cd04c 100644 --- a/drivers/usb/xhcid/src/xhci/ring.rs +++ b/drivers/usb/xhcid/src/xhci/ring.rs @@ -86,7 +86,10 @@ impl Ring { let index = offset / mem::size_of::(); - if index > self.trbs.len() { + // Valid indices are 0..len (inclusive of 0, exclusive of len). + // `index == len` would index one-past-the-end and panic in + // `&self.trbs[index]`. The previous `>` allowed this off-by-one. + if index >= self.trbs.len() { return None; } diff --git a/netstack/src/buffer_pool.rs b/netstack/src/buffer_pool.rs index f4f525bec0..f06d98fc53 100644 --- a/netstack/src/buffer_pool.rs +++ b/netstack/src/buffer_pool.rs @@ -79,12 +79,27 @@ impl BufferPool { let buffer = match self.stack.borrow_mut().pop() { None => vec![0u8; self.buffers_size], Some(mut v) => { - // memsetting the buffer with `resize` would be a waste of time + // The recycled Vec still holds the previous packet's bytes + // between its logical length and its capacity. The previous + // implementation called `set_len(capacity)` and handed the + // buffer out without zeroing, which is a persistent + // information-disclosure vector: any consumer that fails to + // fully overwrite the buffer (padding bytes, partial reads) + // would leak prior flows' data on the wire. + // + // Zero the capacity-then-truncate window. The cost is one + // memset of the buffer's capacity; this is acceptable because + // (a) the buffer is hot in cache, (b) the alternative is a + // security boundary violation, and (c) callers may still use + // the buffer with `resize`/`set_len` semantics as before. let capacity = v.capacity(); - // SAFETY: caller must verify the safety contract for this operation -unsafe { - v.set_len(capacity); - } + v.fill(0); + // SAFETY: capacity bytes have just been initialized to 0, + // and the previous allocation lives until the next `Drop` of + // this Vec (the Vec is not dropped, just truncated). The + // returned Vec has length == capacity and all bytes == 0, + // so reads of any prefix are well-defined. + unsafe { v.set_len(capacity) }; v } };