Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Hardware Use-After-Free due to dangling Page Directory Entries (PDEs) when temporary Vmm instances are dropped. - [High] Hardware Use-After-Free: Early error returns bypass explicit unmapping, leaking hardware PTEs pointing to freed VRAM. - [Medium] Logic Error: Overlapping Virtual Frame Number (VFN) allocations corrupt global state. - [Medium] Hardware Synchronization: Missing dummy reads to flush posted MMIO writes across different PCI BARs. - [Low] Unconditional `#[expect(dead_code)]` causes a build error when selftests are enabled. --
commit 8fda415a03ac7ee1e1b918cb7cfe5d0bb8315e6c Author: Joel Fernandes <[email protected]> gpu: nova-core: mm: Add BAR1 memory management self-tests Add self-tests for BAR1 access during driver probe when CONFIG_NOVA_CORE_SELFTESTS is enabled (default disabled). This results in testing the Vmm, GPU buddy allocator and BAR1 region all of which should function correctly for the tests to pass. > diff --git a/drivers/gpu/nova-core/mm/bar_user.rs > b/drivers/gpu/nova-core/mm/bar_user.rs > index ef1d8e6f8c9c3..1bef01d147ada 100644 > --- a/drivers/gpu/nova-core/mm/bar_user.rs > +++ b/drivers/gpu/nova-core/mm/bar_user.rs [ ... ] > @@ -84,6 +87,7 @@ pub(crate) struct BarUserAccess<'gpu> { > mapped: Option<MappedRange>, > } > > +#[expect(dead_code)] > impl BarUserAccess<'_> { > /// Tear down the BAR1 mapping. > pub(crate) fn release(mut self, mm: &mut GpuMm<'_>) -> Result { [Severity: Low] When CONFIG_NOVA_CORE_SELFTESTS is enabled, try_read32 and release are used by the self-test, so they are no longer dead code. Since kernel Rust builds treat warnings as errors, does this unconditional lint expectation cause a build failure? Should this use cfg_attr to be conditionally applied? [ ... ] > + // Create a VMM of size 64K to track virtual memory mappings. > + let mut vmm = Vmm::new(pdb_addr, chipset.mmu_version(), > SZ_64K.into_safe_cast())?; [Severity: High] Does dropping this temporary Vmm later in the function leave dangling PDEs in the live hardware pointing to freed VRAM? The Vmm allocates PT pages from the buddy allocator and writes PDEs into the live hardware page directory, but its Drop implementation does not clear them, and the PT VRAM is freed when the temporary Vmm is dropped. If this VRAM is reallocated for user buffers, could the GPU MMU misinterpret user data as page tables? [Severity: Medium] Will this temporary Vmm (and the BarUser created later) allocate VFN 0 for their first mappings and blindly overwrite mappings placed in the global page directory by the live driver's bar_user? > + > + // Create a test mapping. > + let mapped = vmm.map_pages(mm, &[test_pfn], None, true)?; > + let test_vfn = mapped.vfn_start; > + > + // Pre-compute test addresses for the PRAMIN to BAR1 read test. > + let vfn_offset: usize = test_vfn.raw().into_safe_cast(); > + let bar1_base_offset = > vfn_offset.checked_mul(PAGE_SIZE).ok_or(EOVERFLOW)?; > + let bar1_read_offset: usize = bar1_base_offset + 0x100; > + let vram_read_addr = test_vram + 0x100; > + > + // Test 1: Write via PRAMIN, read via BAR1. > + mm.pramin_mut() > + .window_at::<u32>(vram_read_addr)? > + .view() > + .write_val(PATTERN_PRAMIN); > + > + // Read back via BAR1 aperture. > + let bar1_value = bar1.try_read32(bar1_read_offset)?; [Severity: Medium] Since PCI memory space writes are posted operations, could the read from the BAR1 aperture arrive at VRAM before the write via PRAMIN completes? Is a dummy read from PRAMIN required to explicitly flush the write before reading via BAR1 to avoid spurious test failures? [Severity: High] If try_read32() returns an error, the function will return early via the `?` operator. Since the early return bypasses explicit cleanup calls like vmm.unmap_pages(), does this leak live PTEs in hardware pointing to freed VRAM? If the selftest fails early, the driver probe continues normally since the error is caught in run_selftests() in gpu.rs, which would cement this UAF condition in the running system. [ ... ] > + for j in 0..num_pages { > + let page_bar1_off = bar1_base + j * PAGE_SIZE; > + let j_u64: u64 = j.into_safe_cast(); > + let page_phys = block.offset() > + + j_u64 > + .checked_mul(PAGE_SIZE.into_safe_cast()) > + .ok_or(EOVERFLOW)?; > + > + bar1.try_write32(PATTERN_BAR1, page_bar1_off)?; > + > + let pramin_val = mm > + .pramin_mut() > + .window_at::<u32>(VramAddress::from_raw(page_phys))? > + .view() > + .read_val(); [Severity: Medium] Similarly, does writing to BAR1 and immediately reading from PRAMIN require a dummy read from BAR1 to flush the posted write? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
