Re: [PATCH 5/5] gpu: nova-core: vbios: use checked ops and accesses in `FwSecBiosImage::ucode`

[Alexandre Courbot]

On Mon Apr 13, 2026 at 3:20 PM JST, Eliot Courtney wrote:
> On Sat Apr 11, 2026 at 12:05 AM JST, Joel Fernandes wrote:
>> Reviewed-by: Joel Fernandes <joelagnelf@xxxxxxxxxx> , one comment below
>>
>> On 4/10/2026 4:38 AM, Eliot Courtney wrote:
>>> Use checked arithmetic and access for extracting the microcode since the
>>> offsets are firmware derived.
>>> 
>>> Fixes: 47c4846e4319 ("gpu: nova-core: vbios: Add support for FWSEC ucode extraction")
>>> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
>>> ---
>>>  drivers/gpu/nova-core/vbios.rs | 19 ++++++++++++-------
>>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
>>> index 3bd3ac3a69f2..b509cd8407a5 100644
>>> --- a/drivers/gpu/nova-core/vbios.rs
>>> +++ b/drivers/gpu/nova-core/vbios.rs
>>> @@ -1027,16 +1027,21 @@ pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
>>>  
>>>      /// Get the ucode data as a byte slice
>>>      pub(crate) fn ucode(&self, desc: &FalconUCodeDesc) -> Result<&[u8]> {
>>> -        let falcon_ucode_offset = self.falcon_ucode_offset;
>>> -
>>>          // The ucode data follows the descriptor.
>>> -        let ucode_data_offset = falcon_ucode_offset + desc.size();
>>> -        let size = usize::from_safe_cast(desc.imem_load_size() + desc.dmem_load_size());
>>> +        let data = self
>>> +            .base
>>> +            .data
>>> +            .get(self.falcon_ucode_offset..)
>>> +            .ok_or(ERANGE)?;
>>> +        let size = usize::from_safe_cast(
>>> +            desc.imem_load_size()
>>> +                .checked_add(desc.dmem_load_size())
>>> +                .ok_or(ERANGE)?,
>>> +        );
>>>  
>>>          // Get the data slice, checking bounds in a single operation.
>>> -        self.base
>>> -            .data
>>> -            .get(ucode_data_offset..ucode_data_offset + size)
>>> +        data.get(desc.size()..)
>>> +            .and_then(|data| data.get(..size))
>>
>> It might be worth adding something like:
>>
>> data.get_slice(start, size) -> Result
>>
>> in R4L longer term if the data.get(start..).and_then(|data|
>> data.get(..size)) pattern is common. It seems to be so in this series.
>
> Yeah, this sounds reasonable although I think we'd need a name that
> makes it more obvious it takes a size. I had a look at rust standard
> library code and it seems a common pattern is to use split_at_checked
> for the initial split. Unfortunately we can't use that yet I think since
> it's in 1.80.0, but maybe we can use it very soon.

IIUC MRSV will be 1.85 when -rc1 is released, so we should be able to
use `split_at_checked`. I've been looking forward to it as well.

>
> I had a look and it looks like there aren't really other existing
> patterns like .get(start..).and_then(.. get(..size)) in the rust kernel
> code. And in nova-core other usages of getting start..start+size slices
> have local proofs that start+size doesn't overflow.
>
> For the most part in other code hopefully we have local proofs that
> start..start+size won't overflow and we can just .get(start..start+size).

Rust is really missing a way to build a range using a starting point and
a size... but I guess there is a good reason for that.