Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] gpu/nova-core: checksum validation fails on untrimmed queue buffer slices - [High] gpu/nova-core: unvalidated element_count from GSP allows arbitrary read pointer manipulation --
commit 4799dd94b859632b92a2a5821daacb47098f5faa Author: Alexandre Courbot <[email protected]> gpu: nova-core: gsp: cmdq: split the transport part of the receive path This commit splits wait_for_msg into wait_for_element and wait_for_msg. The transport layer is moved into wait_for_element, while the RPC layer that trims the payload slices remains in wait_for_msg. This prepares the code for moving the RPC layer into its own module. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index a5595da23407b..d50a2a594f828 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs [ ... ] > @@ -755,7 +584,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<GspMessage<'_>> { > + 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()), > @@ -768,15 +597,41 @@ fn wait_for_msg(&self, timeout: Delta) -> > Result<GspMessage<'_>> { > // Extract the `GspMsgElement`. > let (header, slice_1) = > GspMsgElement::from_bytes_prefix(slice_1).ok_or(EIO)?; > > + // Validate checksum. > + if Cmdq::calculate_checksum(SBufferIter::new_reader([ > + header.as_bytes(), > + slice_1, > + slice_2, > + ])) != 0 > + { > + dev_err!(&self.dev, "GSP receive: bad checksum\n"); > + return Err(EIO); > + } > + > + Ok(GspMessage { > + header, > + contents: (slice_1, slice_2), > + }) > + } [Severity: High] Does this code calculate the checksum over the entire available read area, including trailing padding or queued messages? In the original wait_for_msg(), slice_1 and slice_2 were trimmed to the message payload_length before the checksum calculation. Since wait_for_element() performs validation before trimming, won't valid messages fail the checksum if there's extra data in the buffer? [ ... ] > @@ -863,9 +703,8 @@ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: > Delta) -> Result<M> > }; > > // 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(message.header.element_count()); > > result > } [Severity: High] Does this code allow an untrusted GSP to arbitrarily manipulate the host's ring buffer read pointer? The element_count() value comes directly from the GSP message header and is never validated against the available read area or queue capacity, unlike payload_length. Can a malicious GSP send a small payload but a huge element_count to force the kernel to process uninitialized memory or stale data? (Note: In the final state of the patch series, this logic ends up in receive_rpc_msg(), but the vulnerability is introduced here). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
