From 70fffa20b21f8c62e52473117f38c301ee8d8d7d Mon Sep 17 00:00:00 2001 From: Michal Tarnacki Date: Tue, 7 Jul 2026 09:51:14 +0200 Subject: [PATCH] fix(pci): BAR sizing and MMIO bounds from security audit - reject size==0/IoSpace BARs, never store raw host BAR addr - get_bar_size mask-first zero-check - alloc_mmio64 upper-bound and checked_add overlap test Signed-off-by: Michal Tarnacki Co-authored-by: GitHub Copilot --- src/devices/pci/src/config.rs | 69 +++++++++++++++++++++++------------ src/devices/pci/src/mmio.rs | 14 +++++-- 2 files changed, 56 insertions(+), 27 deletions(-) diff --git a/src/devices/pci/src/config.rs b/src/devices/pci/src/config.rs index f751f8915..693cb0603 100644 --- a/src/devices/pci/src/config.rs +++ b/src/devices/pci/src/config.rs @@ -454,10 +454,7 @@ impl PciDevice { 2 => { self.bars[current_bar].bar_type = PciBarType::MemorySpace64; - let mut size = self.get_bar_size(current_bar_offset)? as u64; - if size == 0 { - size = (self.get_bar_size(current_bar_offset + 4)? as u64) << 32; - } + let size = self.get_bar_size64(current_bar_offset)?; let addr = if size > 0 { let addr = alloc_mmio64(size)?; @@ -515,34 +512,31 @@ impl PciDevice { // bits 2-1 are the type 0 is 32-but, 2 is 64 bit match bar >> 1 & 3 { 0 => { - let size = self.read_u32(current_bar_offset)?; + let size = self.get_bar_size(current_bar_offset)?; - let addr = if size > 0 { + if size > 0 { let addr = alloc_mmio32(size)?; self.set_bar_addr(current_bar_offset, addr)?; - addr + self.bars[current_bar].bar_type = PciBarType::MemorySpace32; + self.bars[current_bar].address = + u64::from(addr & PCI_MEM32_BASE_ADDRESS_MASK); } else { - bar - }; - - self.bars[current_bar].bar_type = PciBarType::MemorySpace32; - self.bars[current_bar].address = - u64::from(addr & PCI_MEM32_BASE_ADDRESS_MASK); + self.bars[current_bar].bar_type = PciBarType::MemorySpace32; + self.bars[current_bar].address = 0; + } } 2 => { self.bars[current_bar].bar_type = PciBarType::MemorySpace64; - let mut size = self.read_u64(current_bar_offset)?; - let addr = if size > 0 { + let size = self.get_bar_size64(current_bar_offset)?; + if size > 0 { let addr = alloc_mmio64(size)?; self.set_bar_addr(current_bar_offset, addr as u32)?; self.set_bar_addr(current_bar_offset + 4, (addr >> 32) as u32)?; - addr + self.bars[current_bar].address = addr & PCI_MEM64_BASE_ADDRESS_MASK; } else { - bar as u64 - }; - - self.bars[current_bar].address = addr & PCI_MEM64_BASE_ADDRESS_MASK; + self.bars[current_bar].address = 0; + } current_bar_offset += 4; } _ => return Err(PciError::InvalidBarType), @@ -572,10 +566,39 @@ impl PciDevice { let size = self.read_u32(offset)?; self.write_u32(offset, restore)?; - Ok(if size == 0 { - size + let masked = size & 0xFFFF_FFF0; + Ok(if masked == 0 { + 0 + } else { + (!masked).wrapping_add(1) + }) + } + + /// Probe the size of a 64-bit memory BAR. + /// + /// A 64-bit BAR spans two consecutive 32-bit config registers (`offset` and + /// `offset + 4`). The size is determined by writing all ones to both halves, + /// reading them back, restoring the original values, and computing the size + /// from the combined 64-bit mask. The low 4 bits of the low register encode + /// the BAR type, not address bits, so they are masked off; the high register + /// is all address bits. + fn get_bar_size64(&self, offset: u8) -> Result { + let restore_low = self.read_u32(offset)?; + let restore_high = self.read_u32(offset + 4)?; + + self.write_u32(offset, u32::MAX)?; + self.write_u32(offset + 4, u32::MAX)?; + let low = self.read_u32(offset)?; + let high = self.read_u32(offset + 4)?; + + self.write_u32(offset, restore_low)?; + self.write_u32(offset + 4, restore_high)?; + + let masked = ((high as u64) << 32) | u64::from(low & 0xFFFF_FFF0); + Ok(if masked == 0 { + 0 } else { - !(size & 0xFFFF_FFF0) + 1 + (!masked).wrapping_add(1) }) } diff --git a/src/devices/pci/src/mmio.rs b/src/devices/pci/src/mmio.rs index cee28ef39..2f80d25c5 100644 --- a/src/devices/pci/src/mmio.rs +++ b/src/devices/pci/src/mmio.rs @@ -25,10 +25,12 @@ pub fn init_mmio() { // If an overlap is detected, panic with an error message indicating an invalid MMIO configuration. // This ensures that the MMIO space does not conflict with the RAM space. for region in memory_map.iter() { - if (region.addr < (MMIO32_START + MMIO32_SIZE) as u64 - && region.addr + region.size > MMIO32_START as u64) - || (region.addr < MMIO64_START + MMIO64_SIZE - && region.addr + region.size > MMIO64_START) + let region_end = region.addr.saturating_add(region.size); + let mmio32_end = (MMIO32_START as u64).saturating_add(MMIO32_SIZE as u64); + let mmio64_end = MMIO64_START.saturating_add(MMIO64_SIZE); + + if (region.addr < mmio32_end && region_end > MMIO32_START as u64) + || (region.addr < mmio64_end && region_end > MMIO64_START) { panic!("Invalid MMIO configuration: MMIO space overlaps with the RAM space."); } @@ -80,6 +82,10 @@ pub fn alloc_mmio64(size: u64) -> Result { let cur = *MMIO64.lock(); let addr = align_up(cur, size).ok_or(PciError::InvalidParameter)? as u64; + if size > MMIO64_SIZE || addr > MMIO64_START + MMIO64_SIZE - size { + return Err(PciError::MmioOutofResource); + } + *MMIO64.lock() = addr.checked_add(size).ok_or(PciError::InvalidParameter)?; Ok(addr) }