Re: [PATCH v4 01/13] gpu: nova-core: gsp: sequencer: use GspBootContext

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect loop bounds check causes RPC message padding bytes to be executed as hardware sequencer commands (e.g., unintended RegWrite to address 0).
- [High] Fixed-size zero-padded buffer bypasses length checks for truncated commands, leading to execution of zeroed out-of-bounds payloads.
- [High] Unaligned stack array passed to `FromBytes::from_bytes` may cause spurious parsing failures and abort the GSP boot process depending on compiler stack layout.
--

commit c264aea160f8cb747324a1d5a568d1e3a869623a
Author: Alexandre Courbot <acourbot@nvidia.com>

gpu: nova-core: gsp: sequencer: use GspBootContext

`GspBootContext` contains all the resources currently carried by
`GspSequencerParams`, so replace the latter with the former for better
integration with the boot process and less code.

> diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
> index 13983d42b12bc..f55205bd61f35 100644
> --- a/drivers/gpu/nova-core/gsp/sequencer.rs
> +++ b/drivers/gpu/nova-core/gsp/sequencer.rs

[ ... ]

> @@ -335,24 +336,13 @@ fn iter(&self) -> GspSeqIter<'_> {
>      }
>  }

[Severity: High]
This is a pre-existing issue, but does this bounds check in GspSeqIter::next()
fail to prevent processing alignment padding bytes?

In drivers/gpu/nova-core/gsp/sequencer.rs:GspSeqIter::new(), total_cmds is
initialized using the length in DWORDs rather than the command count:

    GspSeqIter {
        ...
        total_cmds: self.seq_info.cmd_index,
        ...
    }

Then in GspSeqIter::next(), the loop bounds check compares commands processed
against this DWORD count:

    if self.cmds_processed >= self.total_cmds ||
       self.current_offset >= self.cmd_data.len() {
        return None;
    }

If the message buffer contains RPC alignment padding (e.g., trailing zeros),
could cmds_processed fail to reach total_cmds, causing these zeros to be
incorrectly parsed as hardware sequencer commands (where opcode 0 is RegWrite
to address 0)?

[Severity: High]
This is a pre-existing issue, but does passing a zero-padded buffer bypass
the length check for truncated commands?

In drivers/gpu/nova-core/gsp/sequencer.rs:GspSeqIter::next(), a truncated
command is copied into a fixed 24-byte zero-initialized array, and a slice
covering the entire array is passed to GspSeqCmd::new():

    let mut buffer = [0u8; CMD_SIZE];
    ...
    buffer[..copy_len].copy_from_slice(
        &self.cmd_data[offset..offset + copy_len]
    );
    let cmd_result = GspSeqCmd::new(&buffer, self.dev);

Inside GspSeqCmd::new(), the length check evaluates data.len(), which will
always be 24 regardless of copy_len:

    if data.len() < size {
        dev_err!(dev, "Data is not enough for command\n");
        return Err(EINVAL);
    }

Could this allow execution of malformed, zero-padded hardware commands?

[Severity: High]
This is a pre-existing issue, but can the unaligned stack array passed to
FromBytes::from_bytes() cause spurious parsing failures?

In drivers/gpu/nova-core/gsp/sequencer.rs:GspSeqIter::next(), buffer is backed
by a u8 array which only guarantees 1-byte alignment:

    let mut buffer = [0u8; CMD_SIZE];

When this buffer is passed to GspSeqCmd::new() and then to
SequencerBufferCmd::from_bytes():

    let fw_cmd = fw::SequencerBufferCmd::from_bytes(data).ok_or(EINVAL)?;

If the compiler places the array at a misaligned address on the stack, could
from_bytes() (which expects 4-byte alignment for u32 fields) fail silently and
abort the GSP boot process?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-nova-bootcontext-v4-0-5539d8469590@nvidia.com?part=1