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

[Eliot Courtney]

On Mon Apr 13, 2026 at 4:10 PM JST, Alexandre Courbot wrote:
> 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.

Yeah fair enough. Let me send a more complete refactor in the next
version. Thanks!