Skip to content

fix(pci): BAR sizing and MMIO bounds from security audit #949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MichalTarnacki wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(pci): BAR sizing and MMIO bounds from security audit #949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 46 additions & 23 deletions src/devices/pci/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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<u64> {
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)
})
}

Expand Down
14 changes: 10 additions & 4 deletions src/devices/pci/src/mmio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
}
Expand Down Expand Up @@ -80,6 +82,10 @@ pub fn alloc_mmio64(size: u64) -> Result<u64> {
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)
}
Expand Down
Loading