Re: [PATCH v4 01/10] gpu: nova-core: gsp: cmdq: validate checksum earlier on receive

[sashiko-bot@kernel.org]

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 <acourbot@nvidia.com>

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/20261009-cmdq-rpc-v4-0-c9ab8de1d3f2@nvidia.com?part=1