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 <[email protected]> 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/[email protected]?part=1
