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

[Eliot Courtney]

On Fri Apr 10, 2026 at 11:53 PM JST, Joel Fernandes wrote:
> 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)?

This one has a local proof that it won't ever OOB, so I didn't use
get(). Not sure what the convention is, but what makes most sense to me
is to use get() if there is no local proof that it will always succeed
and use [] if there is such a proof. WDYT? Do you know if there's a
decided convention for this?

>
>>          } 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.

Yeah, this looks like a better way to do it. Thanks! Will apply.

>
> If you agree with both the above suggestions:
>
> Reviewed-by: Joel Fernandes <joelagnelf@xxxxxxxxxx>
>
> thanks,
>
> --
> Joel Fernandes