Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] gpu: nova-core: spurious checksum failure due to un-truncated slices - [High] gpu: nova-core: infinite loop via unvalidated element_count from GSP --
commit 90b2987a6783eb2acf6f7b5e9bf5e7fbc3108e1d Author: Alexandre Courbot <[email protected]> gpu: nova-core: gsp: cmdq: split the transport part of the send path Move the transport part of send_single_command into send_command_element. The RPC part of send_single_command is passed as a closure to set up moving the RPC code into its own sub-module. No functional change intended. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index a5595da23407b..602824236f64e 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs [ ... ] > @@ -768,16 +807,28 @@ 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 > + { [Severity: High] Does this code spuriously fail the checksum validation when the GSP leaves padding in the ring buffer or batches messages? The patch moves the Cmdq::calculate_checksum call in wait_for_msg before slice_1 and slice_2 are truncated to the actual payload_length. Because calculate_checksum iterates and hashes all bytes in the slices, it will now hash the entire available read area (including trailing ring buffer data) instead of just the single message. > + dev_err!(&self.dev, "GSP receive: bad checksum\n"); > + return Err(EIO); > + } [ ... ] > @@ -863,9 +899,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()); [Severity: High] Can an untrusted or erroneously crafted message header from the GSP cause an infinite loop here? The patch changes the read pointer advancement in receive_msg to use message.header.element_count() (the raw elemCount field from the GSP) instead of safely deriving it from the bounded payload length. If the GSP firmware provides an elemCount of 0 (or a multiple of MSGQ_NUM_PAGES, i.e., 63), advance_cpu_read_ptr adds 0 to the read pointer. If the driver then encounters an unexpected function code and retries, it will continuously re-read the exact same zero-advancing message, resulting in a deadlock. > > result > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
