Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Unnecessary memory and DMA mapping leak on early failure paths in `hal.boot()`. --
--- Patch [3]: [PATCH 3/3] gpu: nova-core: gsp: map the WPR meta for streaming DMA --- commit 3c4ae11167b244d18dae6296df30ee5e35af6eb1 Author: Maurice Hieronymus <[email protected]> gpu: nova-core: gsp: map the WPR meta for streaming DMA This commit changes the WPR metadata mapping from a coherent allocation to a streaming DMA transfer. It intentionally drops the mapping in flight without completing it if the boot sequence fails, to prevent a device-side use-after-free. > diff --git a/drivers/gpu/nova-core/gsp/boot.rs > b/drivers/gpu/nova-core/gsp/boot.rs > index 8afb62d689cb4..300ebf4e843d6 100644 > --- a/drivers/gpu/nova-core/gsp/boot.rs > +++ b/drivers/gpu/nova-core/gsp/boot.rs [ ... ] > @@ -131,6 +139,14 @@ pub(crate) fn boot( > sec2_falcon, > )?; > > + // The chipset-specific boot sequence only succeeds once the device > is done reading the > + // WPR metadata: on Tu102 the Booter-load falcon has halted, on > GH100 GSP-FMC has released > + // the lockdown. If it fails instead, `wpr_meta` drops in flight and > leaks, as the falcon > + // may still be reading the buffer. > + // > + // SAFETY: Per the above, the device has finished accessing the > buffer. > + let _ = unsafe { wpr_meta.complete() }; [Severity: Medium] The patch intentionally drops wpr_meta in flight if hal.boot() returns an error to avoid a device-side use-after-free. However, does this unconditionally leak the memory and DMA mapping even on early failures where hardware access is structurally impossible? For example, looking at Gh100::boot() in drivers/gpu/nova-core/gsp/hal/gh100.rs, an error can occur early: Gh100::boot() { ... let fsp_fw = FspFirmware::new(dev, chipset, FIRMWARE_VERSION)?; ... } This happens before the device is given the DMA handle. Similarly in drivers/gpu/nova-core/gsp/hal/tu102.rs: Tu102::boot() { ... let bios = Vbios::new(dev, bar)?; ... } This also happens before the hardware is given the DMA handle via BooterFirmware::run(). Because hal.boot() only borrows wpr_meta by reference, Gsp::boot() cannot distinguish whether the error occurred before or after the DMA address was handed to the hardware. Consequently, it leaks the buffer on any error. Could the design be adjusted to only leak the buffer if the error happens after the hardware actually receives the DMA handle? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
