Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

Gary Guo
2026-03-10T02:01:29Z

On Mon Mar 9, 2026 at 10:53 PM GMT, Danilo Krummrich wrote:
> The DmaGspMem pointer accessor methods (gsp_write_ptr, gsp_read_ptr,
> cpu_read_ptr, cpu_write_ptr, advance_cpu_read_ptr,
> advance_cpu_write_ptr) dereference a raw pointer to DMA memory, creating
> an intermediate reference before calling volatile read/write methods.
>
> This is undefined behavior since DMA memory can be concurrently modified
> by the device.
>
> Fix this by moving the implementations into a gsp_mem module in fw.rs
> that uses the dma_read!() / dma_write!() macros, making the original
> methods on DmaGspMem thin forwarding wrappers.
>
> An alternative approach would have been to wrap the shared memory in
> Opaque, but that would have required even more unsafe code.
>
> Since the gsp_mem module lives in fw.rs (to access firmware-specific
> binding field names), GspMem, Msgq, DmaGspMem and their relevant fields
> are temporarily widened to pub(in crate::gsp). This will be reverted
> once IoView projections are available.
>
> Cc: Gary Guo <gary@garyguo.net>
> Closes: https://lore.kernel.org/nouveau/DGUT14ILG35P.1UMNRKU93JUM1@kernel.org/
> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> drivers/gpu/nova-core/gsp/cmdq.rs | 71 +++++----------------
> drivers/gpu/nova-core/gsp/fw.rs | 101 ++++++++++++++++++++----------
> 2 files changed, 84 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index ae54708c38eb..00a4062a47dc 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -2,11 +2,7 @@
>
> use core::{
> cmp,
> - mem,
> - sync::atomic::{
> - fence,
> - Ordering, //
> - }, //
> + mem, //
> };
>
> use kernel::{
> @@ -146,30 +142,32 @@ struct MsgqData {
> #[repr(C)]
> // There is no struct defined for this in the open-gpu-kernel-source headers.
> // Instead it is defined by code in `GspMsgQueuesInit()`.
> -struct Msgq {
> +// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
> +pub(in crate::gsp) struct Msgq {

These could all be `(in super)`?

> /// Header for sending messages, including the write pointer.
> - tx: MsgqTxHeader,
> + pub(in crate::gsp) tx: MsgqTxHeader,
> /// Header for receiving messages, including the read pointer.
> - rx: MsgqRxHeader,
> + pub(in crate::gsp) rx: MsgqRxHeader,
> /// The message queue proper.
> msgq: MsgqData,
> }
>
> /// Structure shared between the driver and the GSP and containing the command and message queues.
> #[repr(C)]
> -struct GspMem {
> +// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
> +pub(in crate::gsp) struct GspMem {
> /// Self-mapping page table entries.
> ptes: PteArray<{ GSP_PAGE_SIZE / size_of::<u64>() }>,
> /// CPU queue: the driver writes commands here, and the GSP reads them. It also contains the
> /// write and read pointers that the CPU updates.
> ///
> /// This member is read-only for the GSP.
> - cpuq: Msgq,
> + pub(in crate::gsp) cpuq: Msgq,
> /// GSP queue: the GSP writes messages here, and the driver reads them. It also contains the
> /// write and read pointers that the GSP updates.
> ///
> /// This member is read-only for the driver.
> - gspq: Msgq,
> + pub(in crate::gsp) gspq: Msgq,
> }
>
> // SAFETY: These structs don't meet the no-padding requirements of AsBytes but
> @@ -321,12 +319,7 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
> //
> // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
> fn gsp_write_ptr(&self) -> u32 {
> - let gsp_mem = self.0.start_ptr();
> -
> - // SAFETY:
> - // - The 'CoherentAllocation' contains at least one object.
> - // - By the invariants of `CoherentAllocation` the pointer is valid.
> - (unsafe { (*gsp_mem).gspq.tx.write_ptr() } % MSGQ_NUM_PAGES)
> + super::fw::gsp_mem::gsp_write_ptr(&self.0)
> }
>
> // Returns the index of the memory page the GSP will read the next command from.
> @@ -335,12 +328,7 @@ fn gsp_write_ptr(&self) -> u32 {
> //
> // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
> fn gsp_read_ptr(&self) -> u32 {
> - let gsp_mem = self.0.start_ptr();
> -
> - // SAFETY:
> - // - The 'CoherentAllocation' contains at least one object.
> - // - By the invariants of `CoherentAllocation` the pointer is valid.
> - (unsafe { (*gsp_mem).gspq.rx.read_ptr() } % MSGQ_NUM_PAGES)
> + super::fw::gsp_mem::gsp_read_ptr(&self.0)
> }
>
> // Returns the index of the memory page the CPU can read the next message from.
> @@ -349,27 +337,12 @@ fn gsp_read_ptr(&self) -> u32 {
> //
> // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
> fn cpu_read_ptr(&self) -> u32 {
> - let gsp_mem = self.0.start_ptr();
> -
> - // SAFETY:
> - // - The ['CoherentAllocation'] contains at least one object.
> - // - By the invariants of CoherentAllocation the pointer is valid.
> - (unsafe { (*gsp_mem).cpuq.rx.read_ptr() } % MSGQ_NUM_PAGES)
> + super::fw::gsp_mem::cpu_read_ptr(&self.0)
> }
>
> // Informs the GSP that it can send `elem_count` new pages into the message queue.
> fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
> - let rptr = self.cpu_read_ptr().wrapping_add(elem_count) % MSGQ_NUM_PAGES;
> -
> - // Ensure read pointer is properly ordered.
> - fence(Ordering::SeqCst);
> -
> - let gsp_mem = self.0.start_ptr_mut();
> -
> - // SAFETY:
> - // - The 'CoherentAllocation' contains at least one object.
> - // - By the invariants of `CoherentAllocation` the pointer is valid.
> - unsafe { (*gsp_mem).cpuq.rx.set_read_ptr(rptr) };
> + super::fw::gsp_mem::advance_cpu_read_ptr(&self.0, elem_count)
> }
>
> // Returns the index of the memory page the CPU can write the next command to.
> @@ -378,26 +351,12 @@ fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
> //
> // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
> fn cpu_write_ptr(&self) -> u32 {
> - let gsp_mem = self.0.start_ptr();
> -
> - // SAFETY:
> - // - The 'CoherentAllocation' contains at least one object.
> - // - By the invariants of `CoherentAllocation` the pointer is valid.
> - (unsafe { (*gsp_mem).cpuq.tx.write_ptr() } % MSGQ_NUM_PAGES)
> + super::fw::gsp_mem::cpu_write_ptr(&self.0)
> }
>
> // Informs the GSP that it can process `elem_count` new pages from the command queue.
> fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
> - let wptr = self.cpu_write_ptr().wrapping_add(elem_count) & MSGQ_NUM_PAGES;
> - let gsp_mem = self.0.start_ptr_mut();
> -
> - // SAFETY:
> - // - The 'CoherentAllocation' contains at least one object.
> - // - By the invariants of `CoherentAllocation` the pointer is valid.
> - unsafe { (*gsp_mem).cpuq.tx.set_write_ptr(wptr) };
> -
> - // Ensure all command data is visible before triggering the GSP read.
> - fence(Ordering::SeqCst);
> + super::fw::gsp_mem::advance_cpu_write_ptr(&self.0, elem_count)
> }
> }
>
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 83ff91614e36..ca03d497246d 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -40,6 +40,75 @@
> },
> };
>
> +// TODO: Replace with `IoView` projections once available; the `unwrap()` calls go away once we
> +// switch to the new `dma::Coherent` API.
> +pub(in crate::gsp) mod gsp_mem {
> + use core::sync::atomic::{
> + fence,
> + Ordering, //
> + };
> +
> + use kernel::{
> + dma::CoherentAllocation,
> + dma_read,
> + dma_write,
> + prelude::*, //
> + };
> +
> + use crate::gsp::cmdq::{
> + GspMem,
> + MSGQ_NUM_PAGES, //
> + };
> +
> + pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> + // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> + || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()

I wonder if I should add a panicking variant of index projection for this case.
Perhaps of syntax `[index]!`.

We could also make the existing `[index]` becoming a panicking one instead of
`build_error!` one. It is more consistent with Rust index operator that way.

> + }
> +
> + pub(in crate::gsp) fn gsp_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> + // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> + || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
> + }
> +
> + pub(in crate::gsp) fn cpu_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> + // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> + || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
> + }
> +
> + pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
> + let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
> +
> + // Ensure read pointer is properly ordered.
> + fence(Ordering::SeqCst);
> +
> + // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> + || -> Result {
> + dma_write!(qs, [0]?.cpuq.rx.0.readPtr, rptr);
> + Ok(())
> + }()
> + .unwrap()
> + }
> +
> + pub(in crate::gsp) fn cpu_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> + // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> + || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
> + }
> +
> + pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
> + let wptr = cpu_write_ptr(qs).wrapping_add(count) & MSGQ_NUM_PAGES;

Not really related to your change, but this `&` probably require a comment, as
it has different behaviour compared to `%` given the `MSGQ_NUM_PAGES` is not
power of two. I suppose this is actually intended so there's a way to
distinguish between empty and full ring buffer?

Best,
Gary

> +
> + // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> + || -> Result {
> + dma_write!(qs, [0]?.cpuq.tx.0.writePtr, wptr);
> + Ok(())
> + }()
> + .unwrap();
> +
> + // Ensure all command data is visible before triggering the GSP read.
> + fence(Ordering::SeqCst);
> + }
> +}
> +
> /// Empty type to group methods related to heap parameters for running the GSP firmware.
> enum GspFwHeapParams {}
>
> @@ -708,22 +777,6 @@ pub(crate) fn new(msgq_size: u32, rx_hdr_offset: u32, msg_count: u32) -> Self {
> entryOff: num::usize_into_u32::<GSP_PAGE_SIZE>(),
> })
> }
> -
> - /// Returns the value of the write pointer for this queue.
> - pub(crate) fn write_ptr(&self) -> u32 {
> - let ptr = core::ptr::from_ref(&self.0.writePtr);
> -
> - // SAFETY: `ptr` is a valid pointer to a `u32`.
> - unsafe { ptr.read_volatile() }
> - }
> -
> - /// Sets the value of the write pointer for this queue.
> - pub(crate) fn set_write_ptr(&mut self, val: u32) {
> - let ptr = core::ptr::from_mut(&mut self.0.writePtr);
> -
> - // SAFETY: `ptr` is a valid pointer to a `u32`.
> - unsafe { ptr.write_volatile(val) }
> - }
> }
>
> // SAFETY: Padding is explicit and does not contain uninitialized data.
> @@ -739,22 +792,6 @@ impl MsgqRxHeader {
> pub(crate) fn new() -> Self {
> Self(Default::default())
> }
> -
> - /// Returns the value of the read pointer for this queue.
> - pub(crate) fn read_ptr(&self) -> u32 {
> - let ptr = core::ptr::from_ref(&self.0.readPtr);
> -
> - // SAFETY: `ptr` is a valid pointer to a `u32`.
> - unsafe { ptr.read_volatile() }
> - }
> -
> - /// Sets the value of the read pointer for this queue.
> - pub(crate) fn set_read_ptr(&mut self, val: u32) {
> - let ptr = core::ptr::from_mut(&mut self.0.readPtr);
> -
> - // SAFETY: `ptr` is a valid pointer to a `u32`.
> - unsafe { ptr.write_volatile(val) }
> - }
> }
>
> // SAFETY: Padding is explicit and does not contain uninitialized data.
>
> base-commit: 4da879a0d3fd170a70994b73baa554c6913918b5