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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing validation that the firmware section data size does not exceed the virtual address mapping size.
- [Low] Crate-local imports in the `fw` module are not sorted alphabetically.
--

--- Patch [6]: [PATCH v10 6/7] drm/tyr: add parser for firmware binary ---
commit 2d4004e4d7f81ddd626b7391da06ddfa0d78eb29
Author: Daniel Almeida <daniel.almeida@collabora.com>

drm/tyr: add parser for firmware binary
    
This commit adds a parser for the Mali CSF GPU firmware binary format. It
extracts section metadata, validates the basic firmware structure and
alignment, and ignores protected-mode sections.

> diff --git a/drivers/gpu/drm/tyr/fw/parser.rs b/drivers/gpu/drm/tyr/fw/parser.rs
> new file mode 100644
> index 0000000000000..c4d0ad1d7899d
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/fw/parser.rs
[ ... ]
> +use crate::{
> +    fw::{
> +        CacheMode,
> +        SectionFlags,
> +        CSF_MCU_SHARED_REGION_START, //

[Severity: Low]
This isn't a bug, but the Rust coding guidelines require nested imports to be 
sorted alphabetically. CSF_MCU_SHARED_REGION_START should be listed before
SectionFlags.

> +    },
> +    vm::{
> +        VmFlag,
> +        VmMapFlags, //
> +    }, //
> +};
[ ... ]
> +    fn parse_section_entry(
> +        entry_cursor: &mut Cursor<'_>,
> +        firmware_size: usize,
> +    ) -> Result<Option<ParsedSection>> {
> +        let section_hdr: SectionHeader = SectionHeader::new(entry_cursor)?;
> +
> +        if section_hdr.data.end < section_hdr.data.start {

[Severity: Medium]
Does this code need to ensure the initialization data size doesn't exceed the
destination virtual address mapping size?

If a malformed firmware binary is provided where the data length
(data.end - data.start) is greater than the virtual address length
(va.end - va.start), could this lead to out-of-bounds writes when a firmware
loader uses the virtual address size for memory allocation but copies the
initialization data based on the binary data size?

> +            dev_err!(
> +                entry_cursor.dev,
> +                "Firmware corrupted, data.end < data.start (0x{:x} < 0x{:x})",
> +                section_hdr.data.end,
> +                section_hdr.data.start
> +            );
> +            return Err(EINVAL);
> +        }
> +
> +        if section_hdr.data.end as usize > firmware_size {
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-fw-boot-b4-v10-0-9187aefa3f2f@collabora.com?part=6