pci/vmm: Harden BAR reprogramming against out-of-range MMIO addresses#7950
Conversation
| for bar in self.bar_regions.iter_mut() { | ||
| if bar.addr() == params.new_base { | ||
| *bar = bar.set_address(params.old_base); | ||
| } |
There was a problem hiding this comment.
I don't think this handles the 64-bit BARs correctly as they span two BARs
phip1611
left a comment
There was a problem hiding this comment.
My domain knowledge for PCI/Bar is limited. Using claude code, I've got the following remarks. They seem plausible, but I'm not 100% sure. WDYT?
-
restore_bar_addr64-bit BAR matching is wrong (pci/src/configuration.rs): The arm
(u64::from(bar.addr) << 32) == params.new_baseshifts the low-half value rather than reading the adjacent high-half BAR slot. Mirrordetect_bar_reprogramming's index-based logic and write back bothbars[i]/bars[i+1]with a proper high/low split instead of a bareold_base as u32. -
bar_regionsrollback loop is likely dead code (virtio-devices/src/transport/pci_device.rs):
Ifmove_barfailed before touchingbar_regionsthe loop silently no-ops, making the comment "Undo move_bar's bar_regions update" misleading. Confirm whethermove_barcan partially updatebar_regions; if not, drop the loop. -
Patch ordering breaks bisectability: Patch 2 relies on
endbeing inclusive, but that convention isn't established until patch 3. Swap the two so each commit is individually
correct. -
Document
endas inclusive (create_mmio_allocators): Add a doc comment to the function signature so future callers don't repeat the same off-by-one.
Indeed! That's what I flagged up too. |
1e9583f to
6fb6283
Compare
|
@CMGS Please can you rebase to get rid of the merge commit |
1b823e8 to
a73b2ea
Compare
|
@rbradford done with rebase, thanks |
rbradford
left a comment
There was a problem hiding this comment.
Just need a VFIO implementation.
| /// Restore BAR address in config space after a failed move_bar. | ||
| /// This rolls back the address update made by detect_bar_reprogramming() | ||
| /// so that the config register stays consistent with the MMIO bus mapping. | ||
| fn restore_bar_addr(&mut self, _params: &BarReprogrammingParams) {} |
There was a problem hiding this comment.
Is it possible for you to implement this for VFIO too? Currently it's only implemented here for the virtio devices:
diff --git a/pci/src/vfio.rs b/pci/src/vfio.rs
index 51e3aa927..xxxxx 100644
--- a/pci/src/vfio.rs
+++ b/pci/src/vfio.rs
@@ -2003,6 +2003,10 @@ impl PciDevice for VfioPciDevice {
Ok(())
}
+ fn restore_bar_addr(&mut self, params: &BarReprogrammingParams) {
+ self.common.configuration.restore_bar_addr(params);
+ }
+
fn as_any_mut(&mut self) -> &mut dyn Any {
self
}
There was a problem hiding this comment.
I looked a bit further and there are in total 6 implementations of this trait and so they all need updating in the same way.
There was a problem hiding this comment.
added in latest commit, thanks
There was a problem hiding this comment.
Thanks but I think it makes most sense to squash it into the commit that added it for virtio or put the virtio one into that last commit (my preference). Having it being enabled in the same way spread across two commits is a bit weird.
efc0af0 to
3d55297
Compare
When BAR reprogramming is detected, detect_bar_reprogramming() eagerly updates the BAR address in config space before the actual MMIO remapping occurs. If the subsequent move_bar() fails (e.g. the new address falls outside the allocator range), the config register retains the new address while the MMIO bus still uses the old one, leaving the device broken. Add restore_bar_addr() to undo the config space update when move_bar() fails, so the device remains functional at its original address. For 64-bit BARs, restore both the low and high BAR slots as well as the corresponding config registers, mirroring the two-slot update logic in detect_bar_reprogramming(). Implement restore_bar_addr() for all PciDevice implementations (VirtioPciDevice, VfioPciDevice, VfioUserPciDevice, IvshmemDevice, PvPanicDevice, and PvmemcontrolPciDevice) by delegating to their respective PciConfiguration::restore_bar_addr(). Signed-off-by: CMGS <ilskdw@gmail.com>
The MMIO64 allocator size is computed with alignment truncation: size = (range / alignment) * alignment This loses up to one alignment unit (4 GiB) at the top of the address space. When a guest (Windows with virtio-win 0.1.285) programs a BAR near the top of the physical address space, the allocation fails because the address falls in the truncated gap. Give the last PCI segment allocator all remaining space up to the end of the device area, so no addresses are lost. The `end` parameter of create_mmio_allocators() is an inclusive address (the last valid byte). Fix the 32-bit caller and tests to pass inclusive values, consistent with the 64-bit caller which already uses the inclusive end_of_device_area(). Signed-off-by: CMGS <ilskdw@gmail.com>
3d55297 to
c69c23f
Compare
Summary
When a guest programs a PCI BAR to an address outside the allocatable MMIO range,
move_barfails but the config register has already been updated.This inconsistency between config space and the MMIO bus mapping causes virtio device activation failure, crashing the VMM.
This PR adds two defensive fixes:
bar_regionsto old values whenmove_barfails, keeping the device functional at its original address.Fixes #7938