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

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

[Alexandre Courbot]

`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, even if never accessed, is undefined
behavior.

Fix this by rewriting these methods to use pointer projections in order
to create slices to valid regions only. It should eventually be 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>
---
Changes in v3:
- Rebase on top of latest `drm-rust-next` (with `Coherent` patches).
- Use pointer projections. (thanks Gary!)
- Link to v2: https://patch.msgid.link/20260323-cmdq-ub-fix-v2-1-77d1213c3f7f@nvidia.com

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 | 104 ++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index c853be23e3a5..d85ed96334d4 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -14,6 +14,7 @@
     io::poll::read_poll_timeout,
     new_mutex,
     prelude::*,
+    ptr,
     sync::{
         aref::ARef,
         Mutex, //
@@ -252,38 +253,42 @@ 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());
+
+        // Command queue data.
+        let data = ptr::project!(mut self.0.as_mut_ptr(), .cpuq.msgq.data);
+
+        let (tail_slice, wrap_slice) = if rx == 0 {
+            // Contiguous, leave an empty slot at end of the buffer.
+            (
+                ptr::project!(mut data, [tx..num::u32_as_usize(MSGQ_NUM_PAGES) - 1]),
+                ptr::project!(mut data, [..0]),
+            )
+        } else if rx <= tx {
+            // Discontiguous, leave an empty slot before `rx`.
+            (
+                ptr::project!(mut data, [tx..]),
+                ptr::project!(mut data, [..rx - 1]),
+            )
+        } else {
+            // Contiguous, leave an empty slot before `rx`.
+            (
+                ptr::project!(mut data, [tx..rx - 1]),
+                ptr::project!(mut data, [..0]),
+            )
+        };
 
         // SAFETY:
-        // - 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 { &mut *self.0.as_mut() };
-        // 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 [])
-        }
+        // - Since `data` was created from a valid pointer, both `tail_slice` and `wrap_slice` are
+        //   pointers to valid arrays.
+        // - 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.
+        // - The caller holds a reference to `self` for as long as the returned slices are live,
+        //   meaning the CPU write pointer cannot be advanced and thus that the returned area
+        //   remains exclusive to the CPU for the duration of the slices.
+        (unsafe { &mut *tail_slice }, unsafe { &mut *wrap_slice })
     }
 
     /// Returns the size of the region of the CPU message queue that the driver is currently allowed
@@ -305,27 +310,28 @@ 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());
+
+        // Message queue data.
+        let data = ptr::project!(self.0.as_ptr(), .gspq.msgq.data);
+
+        let (tail_slice, wrap_slice) = if rx <= tx {
+            (ptr::project!(data, [rx..tx]), ptr::project!(data, [..0]))
+        } else {
+            (ptr::project!(data, [rx..]), ptr::project!(data, [..tx]))
+        };
 
         // SAFETY:
-        // - 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_ptr() };
-        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])
-        }
+        // - Since `data` was created from a valid pointer, both `tail_slice` and `wrap_slice` are
+        //   pointers to valid arrays.
+        // - 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.
+        // - The caller holds a reference to `self` for as long as the returned slices are live,
+        //   meaning the CPU read pointer cannot be advanced and thus that the returned area
+        //   remains exclusive to the CPU for the duration of the slices.
+        (unsafe { &*tail_slice }, unsafe { &*wrap_slice })
     }
 
     /// Allocates a region on the command queue that is large enough to send a command of `size`

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

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

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

[Alexandre Courbot]

On Thu Mar 26, 2026 at 2:43 PM JST, Alexandre Courbot wrote:
> `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, even if never accessed, is undefined
> behavior.
>
> Fix this by rewriting these methods to use pointer projections in order
> to create slices to valid regions only. It should eventually be 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'd like to merge this before -rc6 is tagged and drm-rust-next is closed
if possible (i.e. this weekend) - so acks/reviews would be very welcome
if there are no outstanding issues! :)

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

[Gary Guo]

On Thu Mar 26, 2026 at 5:43 AM GMT, Alexandre Courbot wrote:
> `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, even if never accessed, is undefined
> behavior.
> 
> Fix this by rewriting these methods to use pointer projections in order
> to create slices to valid regions only. It should eventually be 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>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
> Changes in v3:
> - Rebase on top of latest `drm-rust-next` (with `Coherent` patches).
> - Use pointer projections. (thanks Gary!)
> - Link to v2: https://patch.msgid.link/20260323-cmdq-ub-fix-v2-1-77d1213c3f7f@nvidia.com
> 
> 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 | 104 ++++++++++++++++++++------------------
>  1 file changed, 55 insertions(+), 49 deletions(-)

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

[Danilo Krummrich]

On Thu Mar 26, 2026 at 6:43 AM CET, Alexandre Courbot wrote:
> +        // - Since `data` was created from a valid pointer, both `tail_slice` and `wrap_slice` are
> +        //   pointers to valid arrays.
> +        // - 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.
> +        // - The caller holds a reference to `self` for as long as the returned slices are live,
> +        //   meaning the CPU write pointer cannot be advanced and thus that the returned area
> +        //   remains exclusive to the CPU for the duration of the slices.
> +        (unsafe { &mut *tail_slice }, unsafe { &mut *wrap_slice })

I think this does miss the justification for tail_slice and wrap_slice to not
overlap, i.e. don't we need something like:

	- `tail_slice` and `wrap_slice` point to non-overlapping sub-ranges of `data` in all
	  branches (in the `rx <= tx` case, `wrap_slice` ends at `rx - 1` which is strictly less
	  than `tx` where `tail_slice` starts; in the other cases one of the slices is empty),
	  so creating two `&mut` references from them does not violate aliasing rules.

With this considered,

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

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

[kernel test robot]

Hi Alexandre,

kernel test robot noticed the following build errors:

[auto build test ERROR on dff8302ca1d0e773c90dbeeb05e759f995c95482]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexandre-Courbot/gpu-nova-core-gsp-fix-undefined-behavior-in-command-queue-code/20260327-003216
base:   dff8302ca1d0e773c90dbeeb05e759f995c95482
patch link:    https://lore.kernel.org/r/20260326-cmdq-ub-fix-v3-1-96af2148ca5c%40nvidia.com
patch subject: [PATCH v3] gpu: nova-core: gsp: fix undefined behavior in command queue code
config: um-randconfig-002-20260327 (https://download.01.org/0day-ci/archive/20260328/202603280326.ucDKVaf2-lkp@intel.com/config)
compiler: clang version 23.0.0git (https://github.com/llvm/llvm-project 054e11d1a17e5ba88bb1a8ef32fad3346e80b186)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260328/202603280326.ucDKVaf2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202603280326.ucDKVaf2-lkp@intel.com/

All errors (new ones prefixed by >>):

   /usr/bin/ld: warning: .tmp_vmlinux1 has a LOAD segment with RWX permissions
   /usr/bin/ld: drivers/gpu/nova-core/nova_core.o: in function `<core::ops::range::Range<usize> as kernel::ptr::projection::ProjectIndex<[[u8; 4096]]>>::index::{closure#0}':
>> rust/kernel/build_assert.rs:28:(.text+0xfb16): undefined reference to `rust_build_error'
   clang: error: linker command failed with exit code 1 (use -v to see invocation)


vim +28 rust/kernel/build_assert.rs

614724e780f587 Miguel Ojeda 2024-11-23   7  
0f595bab9d1c1a Gary Guo     2022-11-10   8  /// Fails the build if the code path calling `build_error!` can possibly be executed.
0f595bab9d1c1a Gary Guo     2022-11-10   9  ///
0f595bab9d1c1a Gary Guo     2022-11-10  10  /// If the macro is executed in const context, `build_error!` will panic.
0f595bab9d1c1a Gary Guo     2022-11-10  11  /// If the compiler or optimizer cannot guarantee that `build_error!` can never
0f595bab9d1c1a Gary Guo     2022-11-10  12  /// be called, a build error will be triggered.
0f595bab9d1c1a Gary Guo     2022-11-10  13  ///
0f595bab9d1c1a Gary Guo     2022-11-10  14  /// # Examples
0f595bab9d1c1a Gary Guo     2022-11-10  15  ///
0f595bab9d1c1a Gary Guo     2022-11-10  16  /// ```
0f595bab9d1c1a Gary Guo     2022-11-10  17  /// #[inline]
0f595bab9d1c1a Gary Guo     2022-11-10  18  /// fn foo(a: usize) -> usize {
0f595bab9d1c1a Gary Guo     2022-11-10  19  ///     a.checked_add(1).unwrap_or_else(|| build_error!("overflow"))
0f595bab9d1c1a Gary Guo     2022-11-10  20  /// }
0f595bab9d1c1a Gary Guo     2022-11-10  21  ///
0f595bab9d1c1a Gary Guo     2022-11-10  22  /// assert_eq!(foo(usize::MAX - 1), usize::MAX); // OK.
0f595bab9d1c1a Gary Guo     2022-11-10  23  /// // foo(usize::MAX); // Fails to compile.
0f595bab9d1c1a Gary Guo     2022-11-10  24  /// ```
0f595bab9d1c1a Gary Guo     2022-11-10  25  #[macro_export]
0f595bab9d1c1a Gary Guo     2022-11-10  26  macro_rules! build_error {
0f595bab9d1c1a Gary Guo     2022-11-10  27      () => {{
614724e780f587 Miguel Ojeda 2024-11-23 @28          $crate::build_assert::build_error("")
0f595bab9d1c1a Gary Guo     2022-11-10  29      }};
0f595bab9d1c1a Gary Guo     2022-11-10  30      ($msg:expr) => {{
614724e780f587 Miguel Ojeda 2024-11-23  31          $crate::build_assert::build_error($msg)
0f595bab9d1c1a Gary Guo     2022-11-10  32      }};
0f595bab9d1c1a Gary Guo     2022-11-10  33  }
0f595bab9d1c1a Gary Guo     2022-11-10  34  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Alexandre,

kernel test robot noticed the following build errors:

[auto build test ERROR on dff8302ca1d0e773c90dbeeb05e759f995c95482]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexandre-Courbot/gpu-nova-core-gsp-fix-undefined-behavior-in-command-queue-code/20260327-003216
base:   dff8302ca1d0e773c90dbeeb05e759f995c95482
patch link:    https://lore.kernel.org/r/20260326-cmdq-ub-fix-v3-1-96af2148ca5c%40nvidia.com
patch subject: [PATCH v3] gpu: nova-core: gsp: fix undefined behavior in command queue code
config: x86_64-randconfig-102-20260327 (https://download.01.org/0day-ci/archive/20260328/202603281331.1ESuqgfz-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260328/202603281331.1ESuqgfz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202603281331.1ESuqgfz-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: rust_build_error
   >>> referenced by build_assert.rs:28 (rust/kernel/build_assert.rs:28)
   >>>               vmlinux.o:(<nova_core::gsp::cmdq::DmaGspMem>::allocate_command)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki