Re: [PATCH v12 13/22] gpu: nova-core: Hopper/Blackwell: add FMC signature extraction

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch uses `field: expr` instead of `field <- expr` inside `init!`, violating Rust subsystem guidelines and defeating the goal of avoiding stack allocation.
--

commit 7149f8afdf16eef886b0d72d3c2139493024923e
Author: John Hubbard <jhubbard@xxxxxxxxxx>

gpu: nova-core: Hopper/Blackwell: add FMC signature extraction
    
This commit extracts the SHA-384 hash, RSA public key, and RSA signature
from FMC ELF32 firmware sections. The signatures are stored in a
heap-allocated structure used for FSP Chain of Trust verification.

> diff --git a/drivers/gpu/nova-core/firmware/fsp.rs b/drivers/gpu/nova-core/firmware/fsp.rs
> index 011be1e571c25..db61905eac9de 100644
> --- a/drivers/gpu/nova-core/firmware/fsp.rs
> +++ b/drivers/gpu/nova-core/firmware/fsp.rs

[ ... ]

> @@ -41,7 +63,73 @@ pub(crate) fn new(
>  
>          Ok(Self {
>              fmc_image,
> -            fmc_elf: fw,
> +            fmc_sigs: Self::extract_fmc_signatures(&fw, dev)?,
>          })
>      }
> +
> +    /// Extract FMC firmware signatures for Chain of Trust verification.

[ ... ]

> +        // Initialize the signatures in place to avoid building the large `FmcSignatures` on the
> +        // stack, then fill each section from the firmware.
> +        let signatures = KBox::init(
> +            init!(FmcSignatures {
> +                hash384: [0; _],
> +                public_key: [0; _],
> +                signature: [0; _],
> +            })

[Severity: Medium]
Does this init! block allocate these arrays on the stack before moving them 
to the heap?

According to the Rust subsystem coding guidelines, fields that require in-place
initialization should use the `field <- expr` syntax rather than `field: expr`. 

Because `hash384: [0; _]` and the other fields use the colon syntax, the
arrays (which are up to 384 bytes each) might be evaluated as temporaries on
the stack, which defeats the goal mentioned in the comment above.

Could these be updated to use the in-place initialization syntax, such as 
`hash384 <- kernel::init::zeroed()` or similar?

> +            .chain(|sigs| {
> +                // PANIC: src and dst lengths are both FSP_HASH_SIZE (verified above).
> +                sigs.hash384.copy_from_slice(hash_section);
> +                // PANIC: dst is sliced to src.len(); src.len() <= FSP_PKEY_SIZE per `get_section`.
> +                sigs.public_key[..pkey_section.len()].copy_from_slice(pkey_section);
> +                // PANIC: dst is sliced to src.len(); src.len() <= FSP_SIG_SIZE per `get_section`.
> +                sigs.signature[..sig_section.len()].copy_from_slice(sig_section);
> +                Ok(())
> +            }),
> +            GFP_KERNEL,
> +        )?;
> +
> +        Ok(signatures)
> +    }
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602032111.224790-1-jhubbard@xxxxxxxxxx?part=13