Re: [PATCH 16/16] gpu: nova-core: mm: Add BAR1 memory management self-tests

[sashiko-bot@kernel.org]

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 <joelagnelf@nvidia.com>

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/20260909-mmrebase-v1-0-8dd5d4225d2e@nvidia.com?part=16