Re: [PATCH 1/2] gpu: nova-core: gsp: cmdq: wrap the sequence number on overflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] nova-core: double fetch in receive_msg() and
  send_single_command() [1]
--

commit f82fa576ff320e318515a10dc3abe76b072d7cb3
Author: Eliot Courtney <ecourtney@nvidia.com>

gpu: nova-core: gsp: cmdq: wrap the sequence number on overflow

This commit uses wrapping_add(1) when incrementing the command queue
sequence number so that it wraps around on overflow.

> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 5f3de456042e4..33074bb53e8c4 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -578,7 +578,7 @@ fn send_command_element<E: CommandElement>(&mut self, command: E) -> Result<u32>
>  
>          // All set - update the write pointer and inform the GSP of the new command.
>          let elem_count = dst.header.element_count();
> -        self.seq += 1;
> +        self.seq = self.seq.wrapping_add(1);
>          self.gsp_mem.advance_cpu_write_ptr(elem_count);

[Severity: Critical]
This problem wasn't introduced by this patch, but can a compromised GSP
alter dst.header.element_count() or message.header.msg_length() in shared
DMA memory between checksum calculation and the pointer updates [1]?

In send_command_element(), dst.header lives in coherent DMA memory that
the GSP can modify concurrently; re-reading dst.header.element_count()
after computing the checksum lets the GSP supply an out-of-bounds count to
advance_cpu_write_ptr().  Similarly, in receive_msg(), wait_for_msg()
checks bounds and validates the checksum using header.msg_length(), and
receive_msg() then fetches message.header.msg_length() a second time from
shared memory to compute elem_count for advance_cpu_read_ptr():

drivers/gpu/nova-core/gsp/cmdq.rs:receive_msg() {
    ...
        let message = self.wait_for_msg(timeout)?;
        let elem_count = u32::try_from(message.header.msg_length().div_ceil(GSP_PAGE_SIZE))
            .map_err(|_| {
                dev_err!(&self.dev, "GSP receive: message length overflow\n");
                EIO
            })?;
        ...
        // Advance the read pointer past this message.
        self.gsp_mem.advance_cpu_read_ptr(elem_count);
    ...
}

Either double fetch allows the GSP to desynchronize the ring buffer
pointers and trigger out-of-bounds reads and writes in shared memory.

[1] https://sashiko.dev/#/bug/linux-a84fa791-1958-42fa-b404-d7ebcb48d6c9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-msgq-64-pages-v1-0-d8c56b6fdd6c@nvidia.com?part=1