base: fix CRITICAL F001 + F1.6 + rtl8139d/rtl8168d panics + e1000d bounds check

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.
This commit is contained in:
Red Bear OS
2026-07-27 15:29:59 +09:00
parent a4a7d1a87c
commit 3a3af8253e
5 changed files with 66 additions and 14 deletions
+13
View File
@@ -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)
}
+16 -4
View File
@@ -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");
+13 -4
View File
@@ -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");
+4 -1
View File
@@ -86,7 +86,10 @@ impl Ring {
let index = offset / mem::size_of::<Trb>();
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;
}
+20 -5
View File
@@ -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
}
};