Re: [PATCH v4 02/13] gpu: nova-core: gsp: sequencer: do not store sequence into GspSequencer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The sequencer iterator silently terminates upon encountering a command parsing error, causing the boot sequence to falsely report success.
- [High] Truncated commands at the end of the sequence buffer are silently executed with zero-padded payloads instead of failing.
--

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

gpu: nova-core: gsp: sequencer: do not store sequence into GspSequencer

The sequence is currently stored in the `GspSequencer` even though its
lifetime is limited to `GspSequencer::run`. This object-oriented design
does not play well with the borrow-checker, as `GspSequencer::iter`
borrows a reference to the `GspSequencer`, which makes it difficult to
introduce mutable references in `GspBootContext`, as we want to do in
order to make the `Falcon` references mutable.

Thus, store the sequence locally in `GspSequencer::run`, and move
iterator creation to `GspSeqIter::new` so it no longer needs to borrow
the whole `GspSequencer`.

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

[ ... ]

> @@ -281,6 +279,18 @@ pub(crate) struct GspSeqIter<'a> {
>      dev: &'a device::Device,
>  }
>  
> +impl<'a> GspSeqIter<'a> {
> +    fn new(seq: &'a GspSequence, dev: &'a device::Device) -> Self {
> +        Self {
> +            cmd_data: &seq.cmd_data,
> +            current_offset: 0,
> +            total_cmds: seq.cmd_index,
> +            cmds_processed: 0,
> +            dev,
> +        }
> +    }
> +}
> +
>  impl<'a> Iterator for GspSeqIter<'a> {
>      type Item = Result<GspSeqCmd>;
>  
> @@ -322,20 +332,6 @@ fn next(&mut self) -> Option<Self::Item> {
>      }
>  }
>  
> -impl<'a> GspSequencer<'a> {
> -    fn iter(&self) -> GspSeqIter<'_> {
> -        let cmd_data = &self.seq_info.cmd_data[..];
> -
> -        GspSeqIter {
> -            cmd_data,
> -            current_offset: 0,
> -            total_cmds: self.seq_info.cmd_index,
> -            cmds_processed: 0,
> -            dev: self.dev,
> -        }
> -    }
> -}
> -

[Severity: High]
This isn't a bug introduced by this patch, but while looking at
GspSeqIter::next() I noticed that it silently terminates on parsing errors:

drivers/gpu/nova-core/gsp/sequencer.rs:GspSeqIter::next() {
    ...
    let cmd_result = GspSeqCmd::new(&buffer, self.dev);

    cmd_result.map_or_else(
        |_err| {
            dev_err!(self.dev, "Error parsing command at offset {}\n", offset);
            None
        },
    ...
}

If firmware sends invalid data and GspSeqCmd::new() returns an error, the
iterator returns None instead of propagating the error. This silently stops
the iterator loop, causing the remaining hardware initialization commands
to be skipped while GspSequencer::run() falsely reports success.

Can this leave the GPU in an uninitialized state and cause system hangs or
memory corruption?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential bounds checking
bypass when GspSeqIter::next() processes truncated commands at the end of the
sequence buffer:

drivers/gpu/nova-core/gsp/sequencer.rs:GspSeqIter::next() {
    ...
    let mut buffer = [0u8; CMD_SIZE];
    let copy_len = if offset + CMD_SIZE <= self.cmd_data.len() {
        CMD_SIZE
    } else {
        self.cmd_data.len() - offset
    };
    buffer[..copy_len].copy_from_slice(&self.cmd_data[offset..offset + copy_len]);
    let cmd_result = GspSeqCmd::new(&buffer, self.dev);
    ...
}

When copy_len is less than CMD_SIZE, the data is copied into the zero-padded
buffer and passed to GspSeqCmd::new() as a full CMD_SIZE slice.

Inside GspSeqCmd::new():

drivers/gpu/nova-core/gsp/sequencer.rs:GspSeqCmd::new() {
    ...
    if data.len() < size {
        dev_err!(dev, "Data is not enough for command\n");
        return Err(EINVAL);
    }
    ...
}

Since data.len() is always evaluated as the full buffer length (CMD_SIZE),
this check always passes, neutralizing the intended length validation.

Does this execute truncated firmware commands with zeroed payloads instead of
failing, potentially causing unintended hardware actions or state corruption?

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