Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] The parser incorrectly rejects firmware sections where `va_start == va_end`, which breaks compatibility with valid firmware binaries. - [High] Firmware entries can silently extend beyond the header region, causing the parser to incorrectly read payload data as headers. - [Medium] The parser fails to validate that virtual address ranges are page-aligned. - [Medium] Casting the firmware binary length to `u32` can cause silent integer truncation on 64-bit architectures, leading to incorrect size validation. - [Low] The catch-all match arm for unknown entry types contains unreachable dead code and overly convoluted boolean logic. --
--- Patch [6]: [PATCH v8 6/7] drm/tyr: add parser for firmware binary --- commit a6abcd1e77314e56a03e26729fd17f0e939b696a Author: Daniel Almeida <[email protected]> drm/tyr: add parser for firmware binary This commit adds a parser for the Mali CSF GPU firmware binary format. The firmware consists of a header followed by entries describing how to load firmware sections into the MCU's memory. > diff --git a/drivers/gpu/drm/tyr/fw/parser.rs > b/drivers/gpu/drm/tyr/fw/parser.rs > new file mode 100644 > index 0000000000000..f71cdcf760bd4 > --- /dev/null > +++ b/drivers/gpu/drm/tyr/fw/parser.rs [ ... ] > + fn parse_fw_header(&mut self) -> Result<FirmwareHeader> { [ ... ] > + let fw_header: FirmwareHeader = match FirmwareHeader::new(&mut > self.cursor) { > + Ok(fw_header) => fw_header, > + Err(e) => { > + dev_err!(self.cursor.dev, "Invalid firmware file: {}\n", > e.to_errno()); > + return Err(e); > + } > + }; > + > + if fw_header.size > self.cursor.len() as u32 { [Severity: Medium] Could casting the firmware slice length down to 32 bits cause issues on 64-bit architectures? If a firmware file happens to be larger than 4GB, the truncated length might falsely trigger the truncated image error. Would it be safer to cast fw_header.size to usize instead? > + dev_err!(self.cursor.dev, "Firmware image is truncated\n"); > + return Err(EINVAL); > + } > + Ok(fw_header) > + } > + > + fn parse_entry(&mut self) -> Result<EntrySection> { [ ... ] > + let entry_section = { > + let mut entry_cursor = self > + .cursor > + .view(self.cursor.pos()..self.cursor.pos() + > section_hdr_size)?; [Severity: High] Can an entry size silently extend beyond the header region here? The FwParser::parse() method iterates until cursor.pos() < fw_header.size, but cursor.view() checks its bounds against self.data.len() (the full binary file size). If an entry size extends past fw_header.size, will the parser read past the header region into the payload data, interpreting raw payload bytes as structured headers? > + > + match entry_section.entry_hdr.entry_type() { > + Ok(EntryType::Iface) => Ok(EntrySection { > + entry_hdr: entry_section.entry_hdr, > + inner: Self::parse_section_entry(&mut entry_cursor, > firmware_size)?, > + }), > + Ok( > + EntryType::Config > + | EntryType::FutfTest > + | EntryType::TraceBuffer > + | EntryType::TimelineMetadata > + | EntryType::BuildInfoMetadata, > + ) => Ok(entry_section), > + > + entry_type => { > + if entry_type.is_err() || > !entry_section.entry_hdr.optional() { > + if !entry_section.entry_hdr.optional() { [Severity: Low] Since EntryType is an exhaustive enum, does this catch-all arm only match the Err variants from TryFrom? If entry_type.is_err() is always true here, it looks like the else block returning Ok(entry_section) is unreachable dead code. Can this logic be simplified to just check the optional flag? > + dev_err!( > + self.cursor.dev, > + "Failed to handle firmware entry type: {}\n", > + entry_type > + > .map_or(entry_section.entry_hdr.entry_type_raw(), |e| e as u8) > + ); > + Err(EINVAL) > + } else { > + Ok(entry_section) > + } > + } else { > + Ok(entry_section) > + } > + } > + } > + }; [ ... ] > + fn parse_section_entry( > + entry_cursor: &mut Cursor<'_>, > + firmware_size: usize, > + ) -> Result<Option<ParsedSection>> { [ ... ] > + if section_hdr.va.start == CSF_MCU_SHARED_REGION_START > + && !section_hdr.section_flags.shared() > + { > + dev_err!( > + entry_cursor.dev, > + "Interface at 0x{:x} must be shared\n", > + CSF_MCU_SHARED_REGION_START > + ); > + return Err(EINVAL); > + } > + > + let mut vm_map_flags = VmMapFlags::empty(); [Severity: Medium] Does the virtual address range need to be validated for page alignment before setting up the VM mapping flags? Passing unaligned virtual addresses to the GPU VM subsystem might cause mapping errors or corrupted page tables. > + > + if !section_hdr.section_flags.write() { > + vm_map_flags |= VmFlag::Readonly; > + } [ ... ] > +impl SectionHeader { > + fn new(cursor: &mut Cursor<'_>) -> Result<Self> { > + let section_flags = SectionFlags::try_from_fw(cursor.read_u32()?)?; > + > + let va_start = cursor.read_u32()?; > + let va_end = cursor.read_u32()?; > + > + let va = va_start..va_end; > + > + if va.is_empty() { [Severity: High] Does this check incorrectly reject valid firmware sections? The Rust Range::is_empty() function returns true when start >= end, which means sections with va_start == va_end are rejected with EINVAL. If a firmware section contains payload data but has no VM mapping footprint (where va_start == va_end is intentional), will this cause the parser to falsely reject it? > + dev_err!( > + cursor.dev, > + "Invalid firmware file: empty VA range at pos {}\n", > + cursor.pos(), > + ); > + return Err(EINVAL); > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
