Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] gpu: nova-core: double fetch of message length allows queue desynchronization --
commit 946fd7a5c2c75e7ae4c51655010ee1c1c0977129 Author: Alexandre Courbot <[email protected]> gpu: nova-core: gsp: cmdq: validate checksum earlier on receive This moves the checksum validation before accessing the RPC header to avoid printing an unvalidated sequence number. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index 1b2f346a237f0..b1d46d6d4d4ab 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs > @@ -792,6 +792,15 @@ 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 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 [Severity: High] Does this code introduce a double fetch regression? Because the GSP message queue resides in DMA-coherent shared memory, the device can concurrently modify header.length(). By using header.length() to bound the checksum calculation via take() here, but later using header.payload_length() to slice the payload: drivers/gpu/nova-core/gsp/cmdq.rs:wait_for_msg() { ... let payload_length = header.payload_length(); ... } could the GSP modify the length field between these fetches, causing the slices to cover unvalidated payload data and bypassing the checksum? Furthermore, there is a third fetch of the length later in receive_msg(): drivers/gpu/nova-core/gsp/cmdq.rs:receive_msg() { ... self.gsp_mem.advance_cpu_read_ptr(u32::try_from( message.header.length().div_ceil(GSP_PAGE_SIZE), )?); ... } Could modifying the length before this fetch cause the CPU to advance the pointer by an arbitrary amount, desynchronizing the command queue and potentially processing arbitrary data as commands? Do follow-up patches in this series retain this same structure in other areas, such as consume_element and RpcMessage::parse? > + { > + dev_err!(&self.dev, "GSP receive: bad checksum\n"); > + return Err(EIO); > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
