Re: [PATCH 3/5] gpu: nova-core: vbios: use checked accesses in `setup_falcon_data`

[Joel Fernandes]

Hi Eliot,

On 4/10/2026 4:38 AM, Eliot Courtney wrote:
> Use checked arithmetic and accesses where the values are firmware
> derived to prevent potential overflow.
> 
> Fixes: dc70c6ae2441 ("gpu: nova-core: vbios: Add support to look up PMU table in FWSEC")
> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/vbios.rs | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index de856000de23..2b0dc1a9125d 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -936,17 +936,12 @@ fn setup_falcon_data(
>  
>          self.falcon_data_offset = Some(offset);
>  
> -        if pmu_in_first_fwsec {
> -            self.pmu_lookup_table = Some(PmuLookupTable::new(
> -                &self.base.dev,
> -                &first_fwsec.base.data[offset..],
> -            )?);
> +        let pmu_lookup_data = if pmu_in_first_fwsec {
> +            &first_fwsec.base.data[offset..]

I suggest use get() here as well for consistency with your use of get()
further below.
  first_fwsec.base.data.get(offset..).ok_or(EINVAL)?

>          } else {
> -            self.pmu_lookup_table = Some(PmuLookupTable::new(
> -                &self.base.dev,
> -                &self.base.data[offset..],
> -            )?);
> -        }
> +            self.base.data.get(offset..).ok_or(EINVAL)?
> +        };
> +        self.pmu_lookup_table = Some(PmuLookupTable::new(&self.base.dev, pmu_lookup_data)?);
>  
>          match self
>              .pmu_lookup_table
> @@ -955,8 +950,9 @@ fn setup_falcon_data(
>              .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
>          {
>              Ok(entry) => {
> -                let mut ucode_offset = usize::from_safe_cast(entry.data);
> -                ucode_offset -= pci_at_image.base.data.len();
> +                let mut ucode_offset = usize::from_safe_cast(entry.data)
> +                    .checked_sub(pci_at_image.base.data.len())
> +                    .ok_or(EINVAL)?;
>                  if ucode_offset < first_fwsec.base.data.len() {
>                      dev_err!(self.base.dev, "Falcon Ucode offset not in second Fwsec.\n");
>                      return Err(EINVAL);

How about replace this whole block with:

  self.falcon_ucode_offset = Some(
      usize::from_safe_cast(entry.data)
          .checked_sub(pci_at_image.base.data.len())
          .and_then(|o| o.checked_sub(first_fwsec.base.data.len()))
          .ok_or(EINVAL)
          .inspect_err(|_| {
              dev_err!(self.base.dev,
                       "Falcon Ucode offset not in second Fwsec.\n");
          })?,
  );

That way, the error message also shows up when
checked_sub(pci_at_image.base.data.len()) fails and it is a bit cleaner.

If you agree with both the above suggestions:

Reviewed-by: Joel Fernandes <joelagnelf@xxxxxxxxxx>

thanks,

--
Joel Fernandes