Skip to content

Commit 0073a17

Browse files
author
Danilo Krummrich
committed
gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors
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 and their relevant fields are temporarily widened to pub(super). 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: 75f6b1d ("gpu: nova-core: gsp: Add GSP command queue bindings and handling") Reviewed-by: Alexandre Courbot <acourbot@nvidia.com> Link: https://patch.msgid.link/20260309225408.27714-1-dakr@kernel.org [ Use pub(super) where possible; replace bitwise-and with modulo operator analogous to [1]. - Danilo ] Link: https://lore.kernel.org/all/20260129-nova-core-cmdq1-v3-1-2ede85493a27@nvidia.com/ [1] Signed-off-by: Danilo Krummrich <dakr@kernel.org>
1 parent c7940c8 commit 0073a17

2 files changed

Lines changed: 84 additions & 88 deletions

File tree

‎drivers/gpu/nova-core/gsp/cmdq.rs‎

Lines changed: 15 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,7 @@
22

33
use core::{
44
cmp,
5-
mem,
6-
sync::atomic::{
7-
fence,
8-
Ordering, //
9-
}, //
5+
mem, //
106
};
117

128
use kernel::{
@@ -146,30 +142,32 @@ static_assert!(align_of::<MsgqData>() == GSP_PAGE_SIZE);
146142
#[repr(C)]
147143
// There is no struct defined for this in the open-gpu-kernel-source headers.
148144
// Instead it is defined by code in `GspMsgQueuesInit()`.
149-
struct Msgq {
145+
// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
146+
pub(super) struct Msgq {
150147
/// Header for sending messages, including the write pointer.
151-
tx: MsgqTxHeader,
148+
pub(super) tx: MsgqTxHeader,
152149
/// Header for receiving messages, including the read pointer.
153-
rx: MsgqRxHeader,
150+
pub(super) rx: MsgqRxHeader,
154151
/// The message queue proper.
155152
msgq: MsgqData,
156153
}
157154

158155
/// Structure shared between the driver and the GSP and containing the command and message queues.
159156
#[repr(C)]
160-
struct GspMem {
157+
// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
158+
pub(super) struct GspMem {
161159
/// Self-mapping page table entries.
162160
ptes: PteArray<{ Self::PTE_ARRAY_SIZE }>,
163161
/// CPU queue: the driver writes commands here, and the GSP reads them. It also contains the
164162
/// write and read pointers that the CPU updates.
165163
///
166164
/// This member is read-only for the GSP.
167-
cpuq: Msgq,
165+
pub(super) cpuq: Msgq,
168166
/// GSP queue: the GSP writes messages here, and the driver reads them. It also contains the
169167
/// write and read pointers that the GSP updates.
170168
///
171169
/// This member is read-only for the driver.
172-
gspq: Msgq,
170+
pub(super) gspq: Msgq,
173171
}
174172

175173
impl GspMem {
@@ -331,12 +329,7 @@ impl DmaGspMem {
331329
//
332330
// - The returned value is between `0` and `MSGQ_NUM_PAGES`.
333331
fn gsp_write_ptr(&self) -> u32 {
334-
let gsp_mem = self.0.start_ptr();
335-
336-
// SAFETY:
337-
// - The 'CoherentAllocation' contains at least one object.
338-
// - By the invariants of `CoherentAllocation` the pointer is valid.
339-
(unsafe { (*gsp_mem).gspq.tx.write_ptr() } % MSGQ_NUM_PAGES)
332+
super::fw::gsp_mem::gsp_write_ptr(&self.0)
340333
}
341334

342335
// Returns the index of the memory page the GSP will read the next command from.
@@ -345,12 +338,7 @@ impl DmaGspMem {
345338
//
346339
// - The returned value is between `0` and `MSGQ_NUM_PAGES`.
347340
fn gsp_read_ptr(&self) -> u32 {
348-
let gsp_mem = self.0.start_ptr();
349-
350-
// SAFETY:
351-
// - The 'CoherentAllocation' contains at least one object.
352-
// - By the invariants of `CoherentAllocation` the pointer is valid.
353-
(unsafe { (*gsp_mem).gspq.rx.read_ptr() } % MSGQ_NUM_PAGES)
341+
super::fw::gsp_mem::gsp_read_ptr(&self.0)
354342
}
355343

356344
// Returns the index of the memory page the CPU can read the next message from.
@@ -359,27 +347,12 @@ impl DmaGspMem {
359347
//
360348
// - The returned value is between `0` and `MSGQ_NUM_PAGES`.
361349
fn cpu_read_ptr(&self) -> u32 {
362-
let gsp_mem = self.0.start_ptr();
363-
364-
// SAFETY:
365-
// - The ['CoherentAllocation'] contains at least one object.
366-
// - By the invariants of CoherentAllocation the pointer is valid.
367-
(unsafe { (*gsp_mem).cpuq.rx.read_ptr() } % MSGQ_NUM_PAGES)
350+
super::fw::gsp_mem::cpu_read_ptr(&self.0)
368351
}
369352

370353
// Informs the GSP that it can send `elem_count` new pages into the message queue.
371354
fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
372-
let rptr = self.cpu_read_ptr().wrapping_add(elem_count) % MSGQ_NUM_PAGES;
373-
374-
// Ensure read pointer is properly ordered.
375-
fence(Ordering::SeqCst);
376-
377-
let gsp_mem = self.0.start_ptr_mut();
378-
379-
// SAFETY:
380-
// - The 'CoherentAllocation' contains at least one object.
381-
// - By the invariants of `CoherentAllocation` the pointer is valid.
382-
unsafe { (*gsp_mem).cpuq.rx.set_read_ptr(rptr) };
355+
super::fw::gsp_mem::advance_cpu_read_ptr(&self.0, elem_count)
383356
}
384357

385358
// Returns the index of the memory page the CPU can write the next command to.
@@ -388,26 +361,12 @@ impl DmaGspMem {
388361
//
389362
// - The returned value is between `0` and `MSGQ_NUM_PAGES`.
390363
fn cpu_write_ptr(&self) -> u32 {
391-
let gsp_mem = self.0.start_ptr();
392-
393-
// SAFETY:
394-
// - The 'CoherentAllocation' contains at least one object.
395-
// - By the invariants of `CoherentAllocation` the pointer is valid.
396-
(unsafe { (*gsp_mem).cpuq.tx.write_ptr() } % MSGQ_NUM_PAGES)
364+
super::fw::gsp_mem::cpu_write_ptr(&self.0)
397365
}
398366

399367
// Informs the GSP that it can process `elem_count` new pages from the command queue.
400368
fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
401-
let wptr = self.cpu_write_ptr().wrapping_add(elem_count) & MSGQ_NUM_PAGES;
402-
let gsp_mem = self.0.start_ptr_mut();
403-
404-
// SAFETY:
405-
// - The 'CoherentAllocation' contains at least one object.
406-
// - By the invariants of `CoherentAllocation` the pointer is valid.
407-
unsafe { (*gsp_mem).cpuq.tx.set_write_ptr(wptr) };
408-
409-
// Ensure all command data is visible before triggering the GSP read.
410-
fence(Ordering::SeqCst);
369+
super::fw::gsp_mem::advance_cpu_write_ptr(&self.0, elem_count)
411370
}
412371
}
413372

‎drivers/gpu/nova-core/gsp/fw.rs‎

Lines changed: 69 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,75 @@ use crate::{
4040
},
4141
};
4242

43+
// TODO: Replace with `IoView` projections once available; the `unwrap()` calls go away once we
44+
// switch to the new `dma::Coherent` API.
45+
pub(super) mod gsp_mem {
46+
use core::sync::atomic::{
47+
fence,
48+
Ordering, //
49+
};
50+
51+
use kernel::{
52+
dma::CoherentAllocation,
53+
dma_read,
54+
dma_write,
55+
prelude::*, //
56+
};
57+
58+
use crate::gsp::cmdq::{
59+
GspMem,
60+
MSGQ_NUM_PAGES, //
61+
};
62+
63+
pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
64+
// PANIC: A `dma::CoherentAllocation` always contains at least one element.
65+
|| -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
66+
}
67+
68+
pub(in crate::gsp) fn gsp_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
69+
// PANIC: A `dma::CoherentAllocation` always contains at least one element.
70+
|| -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
71+
}
72+
73+
pub(in crate::gsp) fn cpu_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
74+
// PANIC: A `dma::CoherentAllocation` always contains at least one element.
75+
|| -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
76+
}
77+
78+
pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
79+
let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
80+
81+
// Ensure read pointer is properly ordered.
82+
fence(Ordering::SeqCst);
83+
84+
// PANIC: A `dma::CoherentAllocation` always contains at least one element.
85+
|| -> Result {
86+
dma_write!(qs, [0]?.cpuq.rx.0.readPtr, rptr);
87+
Ok(())
88+
}()
89+
.unwrap()
90+
}
91+
92+
pub(in crate::gsp) fn cpu_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
93+
// PANIC: A `dma::CoherentAllocation` always contains at least one element.
94+
|| -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
95+
}
96+
97+
pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
98+
let wptr = cpu_write_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
99+
100+
// PANIC: A `dma::CoherentAllocation` always contains at least one element.
101+
|| -> Result {
102+
dma_write!(qs, [0]?.cpuq.tx.0.writePtr, wptr);
103+
Ok(())
104+
}()
105+
.unwrap();
106+
107+
// Ensure all command data is visible before triggering the GSP read.
108+
fence(Ordering::SeqCst);
109+
}
110+
}
111+
43112
/// Empty type to group methods related to heap parameters for running the GSP firmware.
44113
enum GspFwHeapParams {}
45114

@@ -708,22 +777,6 @@ impl MsgqTxHeader {
708777
entryOff: num::usize_into_u32::<GSP_PAGE_SIZE>(),
709778
})
710779
}
711-
712-
/// Returns the value of the write pointer for this queue.
713-
pub(crate) fn write_ptr(&self) -> u32 {
714-
let ptr = core::ptr::from_ref(&self.0.writePtr);
715-
716-
// SAFETY: `ptr` is a valid pointer to a `u32`.
717-
unsafe { ptr.read_volatile() }
718-
}
719-
720-
/// Sets the value of the write pointer for this queue.
721-
pub(crate) fn set_write_ptr(&mut self, val: u32) {
722-
let ptr = core::ptr::from_mut(&mut self.0.writePtr);
723-
724-
// SAFETY: `ptr` is a valid pointer to a `u32`.
725-
unsafe { ptr.write_volatile(val) }
726-
}
727780
}
728781

729782
// SAFETY: Padding is explicit and does not contain uninitialized data.
@@ -739,22 +792,6 @@ impl MsgqRxHeader {
739792
pub(crate) fn new() -> Self {
740793
Self(Default::default())
741794
}
742-
743-
/// Returns the value of the read pointer for this queue.
744-
pub(crate) fn read_ptr(&self) -> u32 {
745-
let ptr = core::ptr::from_ref(&self.0.readPtr);
746-
747-
// SAFETY: `ptr` is a valid pointer to a `u32`.
748-
unsafe { ptr.read_volatile() }
749-
}
750-
751-
/// Sets the value of the read pointer for this queue.
752-
pub(crate) fn set_read_ptr(&mut self, val: u32) {
753-
let ptr = core::ptr::from_mut(&mut self.0.readPtr);
754-
755-
// SAFETY: `ptr` is a valid pointer to a `u32`.
756-
unsafe { ptr.write_volatile(val) }
757-
}
758795
}
759796

760797
// SAFETY: Padding is explicit and does not contain uninitialized data.

0 commit comments

Comments
 (0)