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 <[email protected]> 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/[email protected]?part=6
