[PATCH 0/5] gpu: nova-core: vbios: harden various array accesses

[PATCH 0/5] gpu: nova-core: vbios: harden various array accesses

[Eliot Courtney]

We have some code that accesses arrays based on values from firmware.
This patch series makes a bunch of those accesses more robust. This
series only touches accesses that are not guaranteed to be safe by local
invariants - some accesses are safe due to earlier checks and I haven't
modified those.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
Eliot Courtney (5):
      gpu: nova-core: vbios: fix various cases of reading past `BIOS_MAX_SCAN_LEN`
      gpu: nova-core: vbios: limit `BitToken` entry reads
      gpu: nova-core: vbios: use checked accesses in `setup_falcon_data`
      gpu: nova-core: vbios: use checked access in `FwSecBiosImage::header`
      gpu: nova-core: vbios: use checked ops and accesses in `FwSecBiosImage::ucode`

 drivers/gpu/nova-core/vbios.rs | 99 +++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 49 deletions(-)
---
base-commit: a7a080bb4236ebe577b6776d940d1717912ff6dd
change-id: 20260409-fix-vbios-d668e9c21d23

Best regards,
--  
Eliot Courtney <ecourtney@nvidia.com>

[PATCH 1/5] gpu: nova-core: vbios: fix various cases of reading past `BIOS_MAX_SCAN_LEN`

[Eliot Courtney]

Fix various cases that allow reading past `BIOS_MAX_SCAN_LEN` when
scanning the VBIOS.

Fix bug where `read_more_at_offset` would unnecessarily read more data.
This happens when the window to read has some part cached and some part
not. It would read `len` bytes instead of just the uncached portion,
which could read past `BIOS_MAX_SCAN_LEN`.

Also add more checked arithmetic to catch potential overflows.
`read_bios_image_at_offset` is called with a length from the VBIOS
header, so we should be more defensive here.

Fixes: 6fda04e7f0cd ("gpu: nova-core: vbios: Add base support for VBIOS construction and iteration")
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index ebda28e596c5..6de7e58e0da0 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -132,17 +132,14 @@ fn read_more(&mut self, len: usize) -> Result {
 
     /// Read bytes at a specific offset, filling any gap.
     fn read_more_at_offset(&mut self, offset: usize, len: usize) -> Result {
-        if offset > BIOS_MAX_SCAN_LEN {
+        let end = offset.checked_add(len).ok_or(EINVAL)?;
+
+        if end > BIOS_MAX_SCAN_LEN {
             dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
             return Err(EINVAL);
         }
 
-        // If `offset` is beyond current data size, fill the gap first.
-        let current_len = self.data.len();
-        let gap_bytes = offset.saturating_sub(current_len);
-
-        // Now read the requested bytes at the offset.
-        self.read_more(gap_bytes + len)
+        self.read_more(end.saturating_sub(self.data.len()))
     }
 
     /// Read a BIOS image at a specific offset and create a [`BiosImage`] from it.
@@ -155,8 +152,9 @@ fn read_bios_image_at_offset(
         len: usize,
         context: &str,
     ) -> Result<BiosImage> {
+        let end = offset.checked_add(len).ok_or(EINVAL)?;
         let data_len = self.data.len();
-        if offset + len > data_len {
+        if end > data_len {
             self.read_more_at_offset(offset, len).inspect_err(|e| {
                 dev_err!(
                     self.dev,
@@ -167,7 +165,7 @@ fn read_bios_image_at_offset(
             })?;
         }
 
-        BiosImage::new(self.dev, &self.data[offset..offset + len]).inspect_err(|err| {
+        BiosImage::new(self.dev, &self.data[offset..end]).inspect_err(|err| {
             dev_err!(
                 self.dev,
                 "Failed to {} at offset {:#x}: {:?}\n",
@@ -189,7 +187,7 @@ fn next(&mut self) -> Option<Self::Item> {
             return None;
         }
 
-        if self.current_offset > BIOS_MAX_SCAN_LEN {
+        if self.current_offset >= BIOS_MAX_SCAN_LEN {
             dev_err!(self.dev, "Error: exceeded BIOS scan limit, stopping scan\n");
             return None;
         }

-- 
2.53.0

[PATCH 2/5] gpu: nova-core: vbios: limit `BitToken` entry reads

[Eliot Courtney]

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")
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 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);
+        }
 
         // 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);
+            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]]),
                 });
             }
         }

-- 
2.53.0

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

[Eliot Courtney]

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@nvidia.com>
---
 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..]
         } 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);

-- 
2.53.0

[PATCH 4/5] gpu: nova-core: vbios: use checked access in `FwSecBiosImage::header`

[Eliot Courtney]

Use checked access in `FwSecBiosImage::header` for getting the header
version since the value is firmware derived.

Fixes: 47c4846e4319 ("gpu: nova-core: vbios: Add support for FWSEC ucode extraction")
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 2b0dc1a9125d..3bd3ac3a69f2 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -995,14 +995,16 @@ pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
         // Get the falcon ucode offset that was found in setup_falcon_data.
         let falcon_ucode_offset = self.falcon_ucode_offset;
 
+        let data = self.base.data.get(falcon_ucode_offset..).ok_or(EINVAL)?;
+
         // Read the first 4 bytes to get the version.
-        let hdr_bytes: [u8; 4] = self.base.data[falcon_ucode_offset..falcon_ucode_offset + 4]
+        let hdr_bytes: [u8; 4] = data
+            .get(..4)
+            .ok_or(EINVAL)?
             .try_into()
             .map_err(|_| EINVAL)?;
         let hdr = u32::from_le_bytes(hdr_bytes);
         let ver = (hdr & 0xff00) >> 8;
-
-        let data = self.base.data.get(falcon_ucode_offset..).ok_or(EINVAL)?;
         match ver {
             2 => {
                 let v2 = FalconUCodeDescV2::from_bytes_copy_prefix(data)

-- 
2.53.0

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

[Eliot Courtney]

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@nvidia.com>
---
 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))
             .ok_or(ERANGE)
             .inspect_err(|_| {
                 dev_err!(

-- 
2.53.0

Re: [PATCH 1/5] gpu: nova-core: vbios: fix various cases of reading past `BIOS_MAX_SCAN_LEN`

[Joel Fernandes]

On 4/10/2026 4:38 AM, Eliot Courtney wrote:
> Fix various cases that allow reading past `BIOS_MAX_SCAN_LEN` when
> scanning the VBIOS.
> 
> Fix bug where `read_more_at_offset` would unnecessarily read more data.
> This happens when the window to read has some part cached and some part
> not. It would read `len` bytes instead of just the uncached portion,
> which could read past `BIOS_MAX_SCAN_LEN`.
> 
> Also add more checked arithmetic to catch potential overflows.
> `read_bios_image_at_offset` is called with a length from the VBIOS
> header, so we should be more defensive here.
> 
> Fixes: 6fda04e7f0cd ("gpu: nova-core: vbios: Add base support for VBIOS construction and iteration")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index ebda28e596c5..6de7e58e0da0 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -132,17 +132,14 @@ fn read_more(&mut self, len: usize) -> Result {
>  
>      /// Read bytes at a specific offset, filling any gap.
>      fn read_more_at_offset(&mut self, offset: usize, len: usize) -> Result {
> -        if offset > BIOS_MAX_SCAN_LEN {
> +        let end = offset.checked_add(len).ok_or(EINVAL)?;
> +
> +        if end > BIOS_MAX_SCAN_LEN {
>              dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
>              return Err(EINVAL);
>          }
>  
> -        // If `offset` is beyond current data size, fill the gap first.
> -        let current_len = self.data.len();
> -        let gap_bytes = offset.saturating_sub(current_len);
> -
> -        // Now read the requested bytes at the offset.
> -        self.read_more(gap_bytes + len)
> +        self.read_more(end.saturating_sub(self.data.len()))
>      }
>  
>      /// Read a BIOS image at a specific offset and create a [`BiosImage`] from it.
> @@ -155,8 +152,9 @@ fn read_bios_image_at_offset(
>          len: usize,
>          context: &str,
>      ) -> Result<BiosImage> {
> +        let end = offset.checked_add(len).ok_or(EINVAL)?;
>          let data_len = self.data.len();
> -        if offset + len > data_len {
> +        if end > data_len {
>              self.read_more_at_offset(offset, len).inspect_err(|e| {
>                  dev_err!(
>                      self.dev,
> @@ -167,7 +165,7 @@ fn read_bios_image_at_offset(
>              })?;
>          }
>  
> -        BiosImage::new(self.dev, &self.data[offset..offset + len]).inspect_err(|err| {
> +        BiosImage::new(self.dev, &self.data[offset..end]).inspect_err(|err| {
>              dev_err!(
>                  self.dev,
>                  "Failed to {} at offset {:#x}: {:?}\n",
> @@ -189,7 +187,7 @@ fn next(&mut self) -> Option<Self::Item> {
>              return None;
>          }
>  
> -        if self.current_offset > BIOS_MAX_SCAN_LEN {
> +        if self.current_offset >= BIOS_MAX_SCAN_LEN {
>              dev_err!(self.dev, "Error: exceeded BIOS scan limit, stopping scan\n");
>              return None;
>          }
Very nice!

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

thanks,

--
Joel Fernandes

Re: [PATCH 2/5] gpu: nova-core: vbios: limit `BitToken` entry reads

[Joel Fernandes]

On 4/10/2026 4:38 AM, 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")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  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);
> +        }
>  
>          // 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);
> +            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]]),
>                  });
>              }
>          }
> 

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

thanks,

--
Joel Fernandes

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

[Joel Fernandes]

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@nvidia.com>
> ---
>  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)?

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

If you agree with both the above suggestions:

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

thanks,

--
Joel Fernandes

Re: [PATCH 4/5] gpu: nova-core: vbios: use checked access in `FwSecBiosImage::header`

[Joel Fernandes]

On 4/10/2026 4:38 AM, Eliot Courtney wrote:
> Use checked access in `FwSecBiosImage::header` for getting the header
> version since the value is firmware derived.
> 
> Fixes: 47c4846e4319 ("gpu: nova-core: vbios: Add support for FWSEC ucode extraction")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 2b0dc1a9125d..3bd3ac3a69f2 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -995,14 +995,16 @@ pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
>          // Get the falcon ucode offset that was found in setup_falcon_data.
>          let falcon_ucode_offset = self.falcon_ucode_offset;
>  
> +        let data = self.base.data.get(falcon_ucode_offset..).ok_or(EINVAL)?;
> +
>          // Read the first 4 bytes to get the version.
> -        let hdr_bytes: [u8; 4] = self.base.data[falcon_ucode_offset..falcon_ucode_offset + 4]
> +        let hdr_bytes: [u8; 4] = data
> +            .get(..4)
> +            .ok_or(EINVAL)?
>              .try_into()
>              .map_err(|_| EINVAL)?;
>          let hdr = u32::from_le_bytes(hdr_bytes);
>          let ver = (hdr & 0xff00) >> 8;
> -
> -        let data = self.base.data.get(falcon_ucode_offset..).ok_or(EINVAL)?;
>          match ver {
>              2 => {
>                  let v2 = FalconUCodeDescV2::from_bytes_copy_prefix(data)
> 

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

thanks,

--
Joel Fernandes

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

[Joel Fernandes]

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com> , 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@nvidia.com>
> ---
>  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.

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@nvidia.com>
>> ---
>>  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@nvidia.com>
>
> thanks,
>
> --
> Joel Fernandes

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

[Eliot Courtney]

On Sat Apr 11, 2026 at 12:05 AM JST, Joel Fernandes wrote:
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com> , 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@nvidia.com>
>> ---
>>  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.

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

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@nvidia.com>
>>> ---
>>>  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.

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@nvidia.com> , 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@nvidia.com>
>>> ---
>>>  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.

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@nvidia.com>
>>>> ---
>>>>  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!