Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Command parsing errors silently terminate execution while reporting success. - [High] Truncated commands are executed with zeroed payloads instead of failing. - [High] `seq_info.cmd_index` is improperly used as the total command count. -- --- Patch [2]: [PATCH v5 02/13] gpu: nova-core: gsp: sequencer: do not store sequence into GspSequencer --- commit b9f8dfb196e815ccff55e396434b9fbb45c0cc05 Author: Alexandre Courbot <[email protected]> gpu: nova-core: gsp: sequencer: do not store sequence into GspSequencer Moves the sequence out of the GspSequencer struct to avoid borrow-checker limitations. Iterator creation is moved to GspSeqIter::new so it no longer borrows the entire 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, [Severity: High] This is a pre-existing issue, but does this incorrectly use the DWord size as the command count? If seq.cmd_index represents the buffer length in DWords, assigning it to total_cmds breaks the iteration bounds check. In GspSeqIter::next(), cmds_processed is incremented by 1 per command, which means the condition cmds_processed >= total_cmds won't trigger at the actual end of valid commands. Could this cause the iterator to parse and execute padding bytes as commands? > + cmds_processed: 0, > + dev, > + } > + } > +} [ ... ] > @@ -363,14 +358,14 @@ pub(crate) fn run( > > dev_dbg!(sequencer.dev, "Running CPU Sequencer commands\n"); > > - for cmd_result in sequencer.iter() { > + for cmd_result in GspSeqIter::new(&seq_info, sequencer.dev) { [Severity: High] This is a pre-existing issue, but is it possible for truncated commands to be parsed and executed with zeroed payloads? Looking at GspSeqIter::next(), if the remaining buffer is smaller than CMD_SIZE, it appears to zero-pad the data into a buffer of size CMD_SIZE before passing it to GspSeqCmd::new(): let cmd_result = GspSeqCmd::new(&buffer, self.dev); Inside GspSeqCmd::new(), the safety check uses data.len(): if data.len() < size { dev_err!(dev, "Data is not enough for command\n"); return Err(EINVAL); } Since the padded buffer length is always exactly CMD_SIZE, this check might act as dead code. Does this mean truncated commands would succeed using the zero padding as their payload rather than failing with an error? [Severity: High] This is another pre-existing issue, but does command parsing silently terminate on errors while reporting success to the caller? In GspSeqIter::next(), when parsing fails, the error closure logs the failure but returns None: cmd_result.map_or_else( |_err| { dev_err!(self.dev, "Error parsing command at offset {}\n", offset); None }, Since None signals normal iterator termination, this loop in GspSequencer::run() will exit cleanly and return Ok(()) without triggering an error. Could this leave the hardware sequence incomplete while the driver incorrectly assumes the boot sequence succeeded? > match cmd_result { > Ok(cmd) => cmd.run(&sequencer)?, > Err(e) => { > dev_err!( > sequencer.dev, > "Error running command at index {}\n", > - sequencer.seq_info.cmd_index > + seq_info.cmd_index > ); > return Err(e); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
