Re: [PATCH v2 3/5] drm/nova: use `zerocopy` in fwsec.rs

[sashiko-bot@kernel.org]

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 <niyudi.honda@usp.br>

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/20260701174056.12604-1-niyudi.honda@gmail.com?part=3