Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Integer underflow when parsing a firmware entry with size 0, leading to a kernel panic. - [High] Missing validation of firmware data offsets against the binary size. - [Low] Unnecessary memory allocations and string parsing for the section name which is immediately discarded. - [Low] Redundant and unreachable virtual address range validation check. --
--- Patch [6]: [PATCH v7 6/7] drm/tyr: add parser for firmware binary --- commit e96b99b53f7521bb5685e37e8217c78b4047a1ee Author: Daniel Almeida <[email protected]> drm/tyr: add parser for firmware binary Add 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..198a754b294f4 > --- /dev/null > +++ b/drivers/gpu/drm/tyr/fw/parser.rs [ ... ] > + fn parse_entry(&mut self) -> Result<EntrySection> { > + let entry_section = EntrySection { > + entry_hdr: EntryHeader(self.cursor.read_u32()?), > + inner: None, > + }; > + > + if self.cursor.pos() % size_of::<u32>() != 0 > + || entry_section.entry_hdr.size() as usize % size_of::<u32>() != > 0 > + { > + pr_err!( > + "Firmware entry isn't 32 bit aligned, offset={:#x} > size={:#x}\n", > + self.cursor.pos() - size_of::<u32>(), > + entry_section.entry_hdr.size() > + ); > + return Err(EINVAL); > + } > + > + let section_hdr_size = entry_section.entry_hdr.size() as usize - > size_of::<EntryHeader>(); [Severity: High] If a firmware entry has a size of 0, will this subtraction underflow? If entry_hdr.size() is 0, the alignment check earlier in FwParser::parse_entry() passes since 0 % 4 == 0. This subtraction would then wrap around to a very large value. When this underflowed value is added to self.cursor.pos(), it can wrap around to a value smaller than the current position, creating a range where range.start > range.end. When Cursor::view() receives this range, the slice operation &self.data[range] will unconditionally panic, leading to a kernel crash. Should there be a check to ensure entry_hdr.size() is at least size_of::<EntryHeader>()? > + > + let entry_section = { > + let mut entry_cursor = self > + .cursor > + .view(self.cursor.pos()..self.cursor.pos() + > section_hdr_size)?; [ ... ] > + fn parse_section_entry(entry_cursor: &mut Cursor<'_>) -> > Result<Option<ParsedSection>> { > + let section_hdr: SectionHeader = SectionHeader::new(entry_cursor)?; > + > + if section_hdr.data.end < section_hdr.data.start { > + pr_err!( > + "Firmware corrupted, data.end < data.start (0x{:x} < > 0x{:x})\n", > + section_hdr.data.end, > + section_hdr.data.start > + ); > + return Err(EINVAL); > + } > + > + if section_hdr.va.end < section_hdr.va.start { [Severity: Low] Is this validation check reachable? SectionHeader::new() already validates that the virtual address range is not empty by calling va.is_empty(). Since Range::is_empty() returns true if start >= end, the condition section_hdr.va.end < section_hdr.va.start here in FwParser::parse_section_entry() appears to be dead code. > + pr_err!( > + "Firmware corrupted, section_hdr.va.end < > section_hdr.va.start (0x{:x} < 0x{:x})\n", > + section_hdr.va.end, > + section_hdr.va.start > + ); > + return Err(EINVAL); > + } > + > + if section_hdr.section_flags.contains(SectionFlag::Prot) { [ ... ] > + let name_len = entry_cursor.len() - entry_cursor.pos(); > + let name_bytes = entry_cursor.read(name_len)?; > + > + let mut name = KVec::with_capacity(name_bytes.len() + 1, > GFP_KERNEL)?; > + name.extend_from_slice(name_bytes, GFP_KERNEL)?; > + name.push(0, GFP_KERNEL)?; > + > + let _name = CStr::from_bytes_with_nul(&name) > + .ok() > + .and_then(|name| CString::try_from(name).ok()); [Severity: Low] Are these string allocations and validations used anywhere? The parsed section name is bound to _name, which is intentionally unused and immediately dropped. The allocated memory for the KVec and CString is freed without being attached to the ParsedSection or used further. Could this parsing be removed to save CPU cycles and avoid unnecessary memory allocations during firmware loading? > + > + let cache_mode = section_hdr.section_flags.cache_mode(); [ ... ] > +impl SectionHeader { > + /// Reads and validates a section header from the cursor. > + /// > + /// Parses section flags, virtual address range, and data range from the > firmware binary. > + fn new(cursor: &mut Cursor<'_>) -> Result<Self> { > + let section_flags = cursor.read_u32()?; > + let section_flags = SectionFlags::try_from(section_flags)?; > + > + let va_start = cursor.read_u32()?; > + let va_end = cursor.read_u32()?; > + > + let va = va_start..va_end; > + > + if va.is_empty() { > + pr_err!( > + "Invalid firmware file: empty VA range at pos {}\n", > + cursor.pos(), > + ); > + return Err(EINVAL); > + } > + > + let data_start = cursor.read_u32()?; > + let data_end = cursor.read_u32()?; > + let data = data_start..data_end; [Severity: High] Does this code validate that the data offsets are within the firmware binary? The data_start and data_end offsets are read directly from the firmware without being checked against the total size of the firmware binary. When FwParser::parse_section_entry() returns the ParsedSection with this data_range, downstream code that uses it to slice the firmware could encounter an out-of-bounds access. In Rust, this would trigger an unconditional panic and crash the kernel if the firmware is corrupted or maliciously crafted. > + > + Ok(Self { > + section_flags, > + va, > + data, > + }) > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
