Re: [PATCH v7 6/7] drm/tyr: add parser for firmware binary

[sashiko-bot@kernel.org]

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 <daniel.almeida@collabora.com>

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/20260713-fw-boot-b4-v7-0-afd8dc1494dd@collabora.com?part=6