Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Firmware structures use native integer types (e.g., `u32`, `u64`, `u16`) instead of explicit little-endian types, which causes data corruption when parsing firmware metadata on big-endian architectures. -- commit 5588d64059dd57b42b45063f16c1c4ba8ddc35f4 Author: Pedro Yudi Honda <[email protected]> drm/nova: use zerocopy in fwsec.rs In firmware/fwsec.rs, replace the following transmute traits with their zerocopy equivalents: - transmute::FromBytes -> zerocopy::FromBytes - transmute::AsBytes -> zerocopy::IntoBytes - add zerocopy::KnownLayout where necessary Update call sites accordingly. > diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs > b/drivers/gpu/nova-core/firmware/fwsec.rs > index 95e0dd77746b0..1b75cdc02256c 100644 > --- a/drivers/gpu/nova-core/firmware/fwsec.rs > +++ b/drivers/gpu/nova-core/firmware/fwsec.rs [ ... ] > #[repr(C, packed)] > -#[derive(Debug)] > +#[derive(Debug, FromBytes)] > struct FalconAppifV1 { > id: u32, > dmem_base: u32, > } [Severity: High] This is a pre-existing issue, but will this struct misinterpret the firmware data on big-endian architectures? The firmware is little-endian, but these fields use native u32 types. When read_from_prefix() parses the little-endian bytes directly into native types on a big-endian host, it will corrupt dmem_base and id. This causes out-of-bounds calculations when using dmem_base as an offset, leading to an EINVAL failure during driver initialization. Since this patch migrates the code to use zerocopy, would it make sense to update these structures to use zerocopy's endian-aware types (such as zerocopy::byteorder::little_endian::U32) instead of native integers? [ ... ] > -#[derive(Debug)] > +#[derive(Debug, FromBytes, IntoBytes, zerocopy_derive::KnownLayout)] > #[repr(C, packed)] > struct FalconAppifDmemmapperV3 { > signature: u32, > os_code_offset: u32, > os_code_size: u32, [Severity: High] This is a pre-existing issue, but does this struct also need endian-aware types for its u32 fields to prevent data corruption during parsing on big-endian hosts? [ ... ] > -#[derive(Debug)] > +#[derive(Debug, FromBytes, IntoBytes, zerocopy_derive::KnownLayout)] > #[repr(C, packed)] > struct ReadVbios { > ver: u32, > hdr: u32, > addr: u64, [Severity: High] This is a pre-existing issue, but does this struct also need endian-aware types for its native u32 and u64 fields? [ ... ] > -#[derive(Debug)] > +#[derive(Debug, FromBytes, IntoBytes, zerocopy_derive::KnownLayout)] > #[repr(C, packed)] > struct FrtsRegion { > ver: u32, > hdr: u32, > addr: u32, [Severity: High] This is a pre-existing issue, but will this command structure send corrupted coordinates to the firmware if written from a big-endian architecture? It appears to use native u32 types instead of little-endian types. [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
