Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] gsp: spurious checksum failures due to untruncated message buffers --
commit 1576576bd4ef9c21c59c6b207630d4431a81e34d Author: Alexandre Courbot <[email protected]> gpu: nova-core: gsp: introduce and use proper RpcMessageHeader type This commit untangles the GSP command queue transport and message layer code by making the RPC message header more explicit. It introduces the RpcMessageHeader wrapping type to replace GspMsgElement for querying RPC header data. The initialization of the RPC header is now handled separately by the command queue code. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs [ ... ] > @@ -768,16 +774,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 > + { > + dev_err!(&self.dev, "GSP receive: bad checksum\n"); > + return Err(EIO); > + } > + [Severity: High] Does this code compute the checksum over the entire untruncated message buffers? By moving Cmdq::calculate_checksum() to run before slice_1 and slice_2 are truncated to payload_length, it appears to process uninitialized page padding and any subsequent messages present in the ring buffer. The previous code correctly computed the checksum over the exact message size after truncating the slices: // Cut the message slices down to the actual length of the message. let (slice_1, slice_2) = if slice_1.len() > payload_length { ... }; // Validate checksum. if Cmdq::calculate_checksum( ... ) != 0 Could this result in the driver dropping valid GSP messages due to spurious checksum failures, stalling the command queue and preventing GPU initialization? It appears this flaw persists in the final commit of the series (41ab580cce9e4df6dca3c50983301c4f117e6a64). Even though the code is refactored into wait_for_element() and wait_for_rpc_msg(), wait_for_element() still calls calculate_checksum() on the untruncated slices before returning the message to wait_for_rpc_msg() where the size truncation finally happens. > + let rpc_header = header.rpc_header(); > + let payload_length = rpc_header.length(); > + > dev_dbg!( > &self.dev, > "GSP RPC: receive: seq# {}, function={:?}, length=0x{:x}\n", > - header.sequence(), > - header.function(), > - header.length(), > + rpc_header.sequence(), > + rpc_header.function(), > + payload_length, > ); > > - let payload_length = header.payload_length(); > - > // Check that the driver read area is large enough for the message. > if slice_1.len() + slice_2.len() < payload_length { > return Err(EIO); > @@ -796,21 +814,6 @@ fn wait_for_msg(&self, timeout: Delta) -> > Result<GspMessage<'_>> { > ) > }; > > - // Validate checksum. > - if Cmdq::calculate_checksum(SBufferIter::new_reader([ > - header.as_bytes(), > - slice_1, > - slice_2, > - ])) != 0 > - { > - dev_err!( > - &self.dev, > - "GSP RPC: receive: Call {} - bad checksum\n", > - header.sequence() > - ); > - return Err(EIO); > - } > - > Ok(GspMessage { > header, > contents: (slice_1, slice_2), -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
