[PATCH v2] gpu: nova-core: gsp: fix undefined behavior in command queue code

[Alexandre Courbot <acourbot@nvidia.com>]

`driver_read_area` and `driver_write_area` are internal methods that
return slices containing the area of the command queue buffer that the
driver has exclusive read or write access, respectively.

While their returned value is correct and safe to use, internally they
temporarily create a reference to the whole command-buffer slice,
including GSP-owned regions. These regions can change without notice,
and thus creating a slice to them is undefined behavior.

Fix this by replacing the slice logic with pointer arithmetic and
creating slices to valid regions only. It adds unsafe code, but should
be mostly replaced by `IoView` and `IoSlice` once they land.

Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
Reported-by: Danilo Krummrich <dakr@kernel.org>
Closes: https://lore.kernel.org/all/DH47AVPEKN06.3BERUSJIB4M1R@kernel.org/
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
I didn't apply Eliot's Reviewed-by because the code has changed
drastically. The logic should remain identical though.
---
Changes in v2:
- Use `u32_as_usize` consistently.
- Reduce the number of `unsafe` blocks by computing the end offset of
  the returned slices and creating them at the end, in one step.
- Take advantage of the fact that both slices have the same start index
  regardless of the branch chosen.
- Improve safety comments.
- Link to v1: https://patch.msgid.link/20260319-cmdq-ub-fix-v1-1-0f9f6e8f3ce3@nvidia.com
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 133 ++++++++++++++++++++++++--------------
 1 file changed, 84 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index d36a62ba1c60..4d73a299f3da 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -251,38 +251,53 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
     /// As the message queue is a circular buffer, the region may be discontiguous in memory. In
     /// that case the second slice will have a non-zero length.
     fn driver_write_area(&mut self) -> (&mut [[u8; GSP_PAGE_SIZE]], &mut [[u8; GSP_PAGE_SIZE]]) {
-        let tx = self.cpu_write_ptr() as usize;
-        let rx = self.gsp_read_ptr() as usize;
+        let tx = num::u32_as_usize(self.cpu_write_ptr());
+        let rx = num::u32_as_usize(self.gsp_read_ptr());
+
+        // Pointer to the start of the CPU message queue.
+        //
+        // SAFETY:
+        // - `self.0` contains exactly one element.
+        // - `cpuq.msgq.data[0]` is within the bounds of that element.
+        let data = unsafe { &raw mut (*self.0.start_ptr_mut()).cpuq.msgq.data[0] };
+
+        // The two slices we return:
+        // - The tail slice starts at `tx` and ends either at `rx - 1` (if the area doesn't
+        //   wrap) or the end of the queue.
+        // - The wrap slice starts at `0` and is either empty (if the area doesn't wrap) or
+        //   ends at `rx - 1`.
+        // One empty slot is always left before `rx` as a sentinel.
+        //
+        // INVARIANTS:
+        // Per the invariants of `gsp_read_ptr` and `cpu_write_ptr`, `tx` and `rx` are in the range
+        // `0..MSGQ_NUM_PAGES`, so:
+        // - `tx <= tail_end <= MSGQ_NUM_PAGES`.
+        // - `wrap_end < MSGQ_NUM_PAGES`.
+        let (tail_end, wrap_end) = if rx == 0 {
+            // Leave an empty slot at end of the buffer.
+            (num::u32_as_usize(MSGQ_NUM_PAGES) - 1, 0)
+        } else if rx <= tx {
+            // Discontiguous, leave an empty slot before `rx`.
+            (num::u32_as_usize(MSGQ_NUM_PAGES), rx - 1)
+        } else {
+            // Contiguous, leave an empty slot before `rx`.
+            (rx - 1, 0)
+        };
 
         // SAFETY:
-        // - The `CoherentAllocation` contains exactly one object.
-        // - We will only access the driver-owned part of the shared memory.
-        // - Per the safety statement of the function, no concurrent access will be performed.
-        let gsp_mem = &mut unsafe { self.0.as_slice_mut(0, 1) }.unwrap()[0];
-        // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.
-        let (before_tx, after_tx) = gsp_mem.cpuq.msgq.data.split_at_mut(tx);
-
-        // The area starting at `tx` and ending at `rx - 2` modulo MSGQ_NUM_PAGES, inclusive,
-        // belongs to the driver for writing.
-
-        if rx == 0 {
-            // Since `rx` is zero, leave an empty slot at end of the buffer.
-            let last = after_tx.len() - 1;
-            (&mut after_tx[..last], &mut [])
-        } else if rx <= tx {
-            // The area is discontiguous and we leave an empty slot before `rx`.
-            // PANIC:
-            // - The index `rx - 1` is non-negative because `rx != 0` in this branch.
-            // - The index does not exceed `before_tx.len()` (which equals `tx`) because
-            //   `rx <= tx` in this branch.
-            (after_tx, &mut before_tx[..(rx - 1)])
-        } else {
-            // The area is contiguous and we leave an empty slot before `rx`.
-            // PANIC:
-            // - The index `rx - tx - 1` is non-negative because `rx > tx` in this branch.
-            // - The index does not exceed `after_tx.len()` (which is `MSGQ_NUM_PAGES - tx`)
-            //   because `rx < MSGQ_NUM_PAGES` by the `gsp_read_ptr` invariant.
-            (&mut after_tx[..(rx - tx - 1)], &mut [])
+        // - `data` points to an array of `MSGQ_NUM_PAGES` elements.
+        // - The area starting at `tx` and ending at `rx - 2` modulo `MSGQ_NUM_PAGES`,
+        //   inclusive, belongs to the driver for writing and is not accessed concurrently by
+        //   the GSP.
+        // - `data.add(tx)` advances `tx` elements, and `tail_end - tx` further elements are
+        //   accessed. Per the invariant above, `tail_end <= MSGQ_NUM_PAGES`, so
+        //   `tx + (tail_end - tx)` = `tail_end` does not exceed the array bounds.
+        // - Per the invariant above, `wrap_end < MSGQ_NUM_PAGES`.
+        unsafe {
+            (
+                core::slice::from_raw_parts_mut(data.add(tx), tail_end - tx),
+                core::slice::from_raw_parts_mut(data, wrap_end),
+            )
         }
     }
 
@@ -305,27 +320,47 @@ fn driver_write_area_size(&self) -> usize {
     /// As the message queue is a circular buffer, the region may be discontiguous in memory. In
     /// that case the second slice will have a non-zero length.
     fn driver_read_area(&self) -> (&[[u8; GSP_PAGE_SIZE]], &[[u8; GSP_PAGE_SIZE]]) {
-        let tx = self.gsp_write_ptr() as usize;
-        let rx = self.cpu_read_ptr() as usize;
+        let tx = num::u32_as_usize(self.gsp_write_ptr());
+        let rx = num::u32_as_usize(self.cpu_read_ptr());
+
+        // Pointer to the start of the GSP message queue.
+        //
+        // SAFETY:
+        // - `self.0` contains exactly one element.
+        // - `gspq.msgq.data[0]` is within the bounds of that element.
+        let data = unsafe { &raw const (*self.0.start_ptr()).gspq.msgq.data[0] };
+
+        // The two slices we return:
+        // - The tail slice starts at `rx` and ends either at `tx` (if the area doesn't wrap) or
+        //   the end of the queue.
+        // - The wrap slice starts at `0` and is either empty (if the area doesn't wrap) or ends at
+        //   `tx`.
+        //
+        // INVARIANTS:
+        // Per the invariants of `cpu_read_ptr` and `gsp_write_ptr`, `tx` and `rx` are in the range
+        // `0..MSGQ_NUM_PAGES`, so:
+        // - `rx <= tail_end <= MSGQ_NUM_PAGES`.
+        // - `wrap_end < MSGQ_NUM_PAGES`.
+        let (tail_end, wrap_end) = if rx <= tx {
+            (tx, 0)
+        } else {
+            (num::u32_as_usize(MSGQ_NUM_PAGES), tx)
+        };
 
         // SAFETY:
-        // - The `CoherentAllocation` contains exactly one object.
-        // - We will only access the driver-owned part of the shared memory.
-        // - Per the safety statement of the function, no concurrent access will be performed.
-        let gsp_mem = &unsafe { self.0.as_slice(0, 1) }.unwrap()[0];
-        let data = &gsp_mem.gspq.msgq.data;
-
-        // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
-        // belongs to the driver for reading.
-        // PANIC:
-        // - per the invariant of `cpu_read_ptr`, `rx < MSGQ_NUM_PAGES`
-        // - per the invariant of `gsp_write_ptr`, `tx < MSGQ_NUM_PAGES`
-        if rx <= tx {
-            // The area is contiguous.
-            (&data[rx..tx], &[])
-        } else {
-            // The area is discontiguous.
-            (&data[rx..], &data[..tx])
+        // - `data` points to an array of `MSGQ_NUM_PAGES` elements.
+        // - The area starting at `rx` and ending at `tx - 1` modulo `MSGQ_NUM_PAGES`,
+        //   inclusive, belongs to the driver for reading and is not accessed concurrently by
+        //   the GSP.
+        // - `data.add(rx)` advances `rx` elements, and `tail_end - rx` further elements are
+        //   accessed. Per the invariant above, `tail_end <= MSGQ_NUM_PAGES`, so `rx + (tail_end -
+        //   rx)` = `tail_end` does not exceed the array bounds.
+        // - Per the invariant above, `wrap_end < MSGQ_NUM_PAGES`.
+        unsafe {
+            (
+                core::slice::from_raw_parts(data.add(rx), tail_end - rx),
+                core::slice::from_raw_parts(data, wrap_end),
+            )
         }
     }
 

---
base-commit: a19457958c3018783881c4416f272cd594f13049
change-id: 20260319-cmdq-ub-fix-d57b09a745b9

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>