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

[Alexandre Courbot]

On Mon Apr 13, 2026 at 3:04 PM JST, Eliot Courtney wrote:
> 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?

Ideally we use the type system to maintain the proof that OOB cannot
happen - typically by calling `get` early and working with the returned
slice from then on. The problem with this code is that while there is a
local proof that OOB cannot occur *today*, there is no guarantee that
this proof won't be modified (and break the invariant we rely on) by
future code.

Looking at the code it looks like it deserves a larger refactor. We are
setting `pmu_in_first_fwsec` if the offset is valid for the first fwsec,
and modify `offset` if not. Then we check `pmu_in_first_fwsec` to get
the PMU lookup table from the right source. And after that neither
`pmu_in_first_fwsec` not `offset` are ever used again. So this looks
like this could be factored out into a single test (maybe a match on the
result of `get`?), where we simplify things further and don't mutate
variables. Things tend to fall into place with properly guaranteed
invariants when we do that.