Re: [PATCH v3 02/11] gpu: nova-core: vbios: limit `BitToken` entry reads

[Eliot Courtney]

On Wed Apr 29, 2026 at 10:35 PM JST, Alexandre Courbot wrote:
> On Tue Apr 21, 2026 at 5:20 PM JST, Eliot Courtney wrote:
>> If `header.token_size` is smaller than `BitToken`, then we currently can
>> read past the end of `image.base.data`. Check that the token size is at
>> least as big as `BitToken`.
>>
>> Fixes: dc70c6ae2441 ("gpu: nova-core: vbios: Add support to look up PMU table in FWSEC")
>> Reviewed-by: Joel Fernandes <joelagnelf@xxxxxxxxxx>
>> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
>> ---
>>  drivers/gpu/nova-core/vbios.rs | 34 +++++++++++++++++-----------------
>>  1 file changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
>> index 6de7e58e0da0..de856000de23 100644
>> --- a/drivers/gpu/nova-core/vbios.rs
>> +++ b/drivers/gpu/nova-core/vbios.rs
>> @@ -423,31 +423,31 @@ impl BitToken {
>>      /// Find a BIT token entry by BIT ID in a PciAtBiosImage
>>      fn from_id(image: &PciAtBiosImage, token_id: u8) -> Result<Self> {
>>          let header = &image.bit_header;
>> +        let entry_size = usize::from(header.token_size);
>> +
>> +        if entry_size < size_of::<BitToken>() {
>> +            return Err(EINVAL);
>> +        }
>
> You can get rid of this check if you convert the code as suggested
> below.
>
>>  
>>          // Offset to the first token entry
>>          let tokens_start = image.bit_offset + usize::from(header.header_size);
>>  
>>          for i in 0..usize::from(header.token_entries) {
>> -            let entry_offset = tokens_start + (i * usize::from(header.token_size));
>> -
>> -            // Make sure we don't go out of bounds
>> -            if entry_offset + usize::from(header.token_size) > image.base.data.len() {
>> -                return Err(EINVAL);
>> -            }
>> +            let entry_offset = tokens_start + (i * entry_size);
>
> Should we use checked arithmetic here?
>
>> +            let entry = image
>> +                .base
>> +                .data
>> +                .get(entry_offset..)
>> +                .and_then(|data| data.get(..entry_size))
>> +                .ok_or(EINVAL)?;
>>  
>>              // Check if this token has the requested ID
>> -            if image.base.data[entry_offset] == token_id {
>> +            if entry[0] == token_id {
>>                  return Ok(BitToken {
>> -                    id: image.base.data[entry_offset],
>> -                    data_version: image.base.data[entry_offset + 1],
>> -                    data_size: u16::from_le_bytes([
>> -                        image.base.data[entry_offset + 2],
>> -                        image.base.data[entry_offset + 3],
>> -                    ]),
>> -                    data_offset: u16::from_le_bytes([
>> -                        image.base.data[entry_offset + 4],
>> -                        image.base.data[entry_offset + 5],
>> -                    ]),
>> +                    id: entry[0],
>> +                    data_version: entry[1],
>> +                    data_size: u16::from_le_bytes([entry[2], entry[3]]),
>> +                    data_offset: u16::from_le_bytes([entry[4], entry[5]]),
>
> A common pattern in this file (with several such sites still to fix), is
> that since Nova only supports little-endian we can leverage `FromBytes`
> in order to avoid all these `from_le_bytes` call. Here this would look
> as follows:
>
>     for i in 0..usize::from(header.token_entries) {
>         let entry_offset = i
>             .checked_mul(entry_size)
>             .and_then(|off| tokens_start.checked_add(off))
>             .ok_or(EINVAL)?;
>
>         let entry = image
>             .base
>             .data
>             .get(entry_offset..entry_offset + entry_size)
>             .and_then(|data| data.get(..entry_size))
>             .ok_or(EINVAL)?;
>
>         let (token, _) = BitToken::from_bytes_copy_prefix(entry).ok_or(EINVAL)?;
>
>         if token.id == token_id {
>             return Ok(token);
>         }
>     }
>
> which has several benefits:
>
> - No error-prone `entry[index]` accesses,
> - The size check on `entry_size` is done for free by
>   `from_bytes_copy_prefix`, and the slice bounds cannot be wrong,
> - Shorter, more readable code overall.
>
> Unfortunately we cannot just use `from_bytes_prefix` because we don't
> have any alignment guarantee, but this is still an improvement IMHO.
>
> If you go that way and derive `FromBytes` on `BitToken`, don't forget to
> also make it `#[repr(C)]`. :)

I agree this is better, thanks!