Re: [PATCH v3 6/9] gpu: nova-core: gsp: cmdq: split the transport part of the receive path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nova-core: ring buffer desynchronization via unvalidated element count
--

commit 1b11098df54520332b501e33775803bd797ce2f8
Author: Alexandre Courbot <acourbot@nvidia.com>

gpu: nova-core: gsp: cmdq: split the transport part of the receive path

wait_for_msg mixes two layers: the transport layer which polls the
queue, extracts the element header and validates the checksum, and the
RPC layer which reads the RPC header and trims the payload slices to the
length advertised by the RPC header.

Move the transport layer into wait_for_element, and introduce
consume_element, a transport-level method which reads the message's
contents using an implementation of the MessageElement trait before
advancing the CPU read pointer past it, and parse_rpc_message, which
validates the RPC layer. This sets things up for moving the RPC code
into its own module, leaving the transport agnostic of the message type.

> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index b8a9e02b76fe5..07036972dbecf 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -805,7 +905,7 @@ fn send_command<M>(&mut self, command: M) -> Result
>      ///   message queue.
>      ///
>      /// Error codes returned by the message constructor are propagated as-is.
> -    fn wait_for_msg(&self, timeout: Delta) -> Result<RpcMessage<'_>> {
> +    fn wait_for_element(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>          // Wait for a message to arrive from the GSP.
>          let (slice_1, slice_2) = read_poll_timeout(
>              || Ok(self.gsp_mem.driver_read_area()),
> @@ -827,108 +927,27 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<RpcMessage<'_>> {
>              return Err(EIO);
>          }
>  
> -        self.parse_rpc_message(GspMessage {
> +        Ok(GspMessage {
>              header,
>              contents: (slice_1, slice_2),
>          })
>      }

[Severity: High]
Does this code allow a message with a maliciously large length to pass
validation?

Looking at wait_for_element() which validates the checksum using .take():

nova-core/gsp/cmdq.rs:wait_for_element() {
    ...
    // Validate checksum after truncating the message to its exact length.
    if Cmdq::calculate_checksum(
        SBufferIter::new_reader([header.as_bytes(), slice_1, slice_2]).take(header.length()),
    ) != 0
    ...
}

Because .take() silently stops when the iterator is exhausted, this allows
a message with a maliciously large length to pass validation if the
truncated checksum matches.

Since the RPC bounds check has been moved out of wait_for_element() and
into M::read(), the length check is deferred until it is too late to
prevent pointer advancement in consume_element().

[ ... ]

> -    fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
> -    where
> -        // This allows all error types, including `Infallible`, to be used for `M::InitError`.
> -        Error: From<M::InitError>,
> -    {
> -        let message = self.wait_for_msg(timeout)?;
> -        let function = message.header.function().map_err(|_| EINVAL)?;
> -
> -        // Extract the message. Store the result as we want to advance the read pointer even in
> -        // case of failure.
> -        let result = if function == M::FUNCTION {
> -            let (cmd, contents_1) = M::Message::from_bytes_prefix(message.contents.0).ok_or(EIO)?;
> -            let mut sbuffer = SBufferIter::new_reader([contents_1, message.contents.1]);
> -
> -            M::read(cmd, &mut sbuffer)
> -                .map_err(|e| e.into())
> -                .inspect(|_| {
> -                    if !sbuffer.is_empty() {
> -                        dev_warn!(
> -                            &self.dev,
> -                            "GSP message {:?} has unprocessed data\n",
> -                            function
> -                        );
> -                    }
> -                })
> -        } else {
> -            Err(ERANGE)
> -        };
> +    /// Errors from [`Self::wait_for_element`] and from [`MessageElement::read`] are propagated
> +    /// as-is.
> +    fn consume_element<M: MessageElement>(&mut self, timeout: Delta) -> Result<M> {
> +        let message = self.wait_for_element(timeout)?;
> +        let elem_count = u32::try_from(message.header.length().div_ceil(GSP_PAGE_SIZE))?;
> +        let result = M::read(self.dev, message);
>  
> -        // Advance the read pointer past this message.
> -        self.gsp_mem.advance_cpu_read_ptr(u32::try_from(
> -            message.header.length().div_ceil(GSP_PAGE_SIZE),
> -        )?);
> +        self.gsp_mem.advance_cpu_read_ptr(elem_count);

[Severity: High]
Can this corrupt the ring buffer state by advancing the read pointer even if
M::read() fails bounds checking?

In consume_element(), the driver computes the queue advancement elem_count
using message.header.length() (which retrieves the length from the
unvalidated RPC header).

If the GSP firmware sends a massive or padded length, M::read() correctly
bounds-checks and rejects it, but consume_element() temporarily ignores
this failure and unconditionally advances the CPU read pointer by the
unvalidated, potentially massive elem_count.

Recomputing elem_count via div_ceil instead of using the firmware-provided
transport field message.header.element_count() guarantees desynchronization
if the GSP ever pads messages.

Can this be exploited by a malfunctioning or compromised GSP firmware
to irrevocably corrupt the shared queue?

>  
>          result
>      }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-cmdq-rpc-v3-0-91613f06520b@nvidia.com?part=6