[PATCH] gpu: nova-core: fix bounds check In PmuLookupTableEntry::new, data is sliced from 2..6, but the bounds check data.len() < 5 does not satisfy those bounds.

[PATCH] gpu: nova-core: fix bounds check In PmuLookupTableEntry::new, data is sliced from 2..6, but the bounds check data.len() < 5 does not satisfy those bounds.

[Quaternions]

Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
---
 drivers/gpu/nova-core/vbios.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 663fc50e8b66..5b5d9f38cbb3 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -901,7 +901,7 @@ struct PmuLookupTableEntry {

 impl PmuLookupTableEntry {
     fn new(data: &[u8]) -> Result<Self> {
-        if data.len() < 5 {
+        if data.len() < 6 {
             return Err(EINVAL);
         }

--
2.50.1

[PATCH] gpu: nova-core: define named constants for magic numbers

[Quaternions]

Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
---
 drivers/gpu/nova-core/vbios.rs | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 5b5d9f38cbb3..d456c494374d 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -364,8 +364,9 @@ struct BitHeader {
 }

 impl BitHeader {
+    const MIN_LEN: usize = 12;
     fn new(data: &[u8]) -> Result<Self> {
-        if data.len() < 12 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }

@@ -467,8 +468,9 @@ struct PciRomHeader {
 }

 impl PciRomHeader {
+    const MIN_LEN: usize = 26;
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
-        if data.len() < 26 {
+        if data.len() < Self::MIN_LEN {
             // Need at least 26 bytes to read pciDataStrucPtr and sizeOfBlock.
             return Err(EINVAL);
         }
@@ -772,10 +774,11 @@ fn into_image(self) -> Result<BiosImage> {
         BiosImage::try_from(self)
     }

+    const MIN_LEN: usize = 26;
     /// Creates a new BiosImageBase from raw byte data.
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
         // Ensure we have enough data for the ROM header.
-        if data.len() < 26 {
+        if data.len() < Self::MIN_LEN {
             dev_err!(pdev.as_ref(), "Not enough data for ROM header\n");
             return Err(EINVAL);
         }
@@ -900,8 +903,9 @@ struct PmuLookupTableEntry {
 }

 impl PmuLookupTableEntry {
+    const MIN_LEN: usize = 6;
     fn new(data: &[u8]) -> Result<Self> {
-        if data.len() < 6 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }

@@ -928,8 +932,9 @@ struct PmuLookupTable {
 }

 impl PmuLookupTable {
+    const MIN_LEN: usize = 4;
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
-        if data.len() < 4 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }

--
2.50.1

Re: [PATCH] gpu: nova-core: fix bounds check In PmuLookupTableEntry::new, data is sliced from 2..6, but the bounds check data.len() < 5 does not satisfy those bounds.

[Greg KH]

On Fri, Jul 11, 2025 at 04:33:26AM -0700, Quaternions wrote:
> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I know I can't take patches without a changelog, but perhaps other
maintainers are more lax?

Please don't do this...

And fix your "From:" line in your email client?

thanks,

greg k-h

Re: [PATCH] gpu: nova-core: fix bounds check In PmuLookupTableEntry::new, data is sliced from 2..6, but the bounds check data.len() < 5 does not satisfy those bounds.

[Alexandre Courbot]

Thanks for the fix!

On Fri Jul 11, 2025 at 8:33 PM JST, Quaternions wrote:
> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>

You will want to split the subject to something more minimal (e.g. "gpu:
nova-core: fix bounds check In PmuLookupTableEntry::new"), and use the
rest of the sentence as the patch description (as a rule of thumb, 70
chars for the subject, and kernel patches must all have a description).

Also your "From:" field must also match your "Signed-off-by".

./scripts/checkpatch.pl will complain about these points (and check
other things as well), so please make sure to run it before submission:

    WARNING: Missing commit description - Add an appropriate one
    WARNING: From:/Signed-off-by: email name mismatch: 'From: Quaternions <krakow20@gmail.com>' != 'Signed-off-by: Rhys Lloyd <krakow20@gmail.com>'

Re: [PATCH] gpu: nova-core: fix bounds check In PmuLookupTableEntry::new, data is sliced from 2..6, but the bounds check data.len() < 5 does not satisfy those bounds.

[Miguel Ojeda]

On Fri, Jul 11, 2025 at 1:34 PM Quaternions <krakow20@gmail.com> wrote:
>
> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>

Few procedural nits:

  - The title needs to be shorter.

  - The kernel generally does not allow empty commit messages.

  - I would have expected a "From:" line, since the email header do
not match the SoB. Git should have generated this for you, normally.

  - This should have a "Fixes:" tag.

  - Ideally you would provide a base commit via `--base`.

Regarding the patch itself: is there a way this could be done better?
i.e. avoiding a literal?

I see in your other patch you create a `const`, but could we infer
that value somehow from elsewhere?

For instance, we have a range `2..6`, so can we save somewhere that
range and query its end? (it is a field)

Or, even better, can we do fallible slicing with `get()` and avoid
having a "separate" check?

Thanks!

Cheers,
Miguel

Re: [PATCH] gpu: nova-core: define named constants for magic numbers

[Alexandre Courbot]

On Fri Jul 11, 2025 at 8:33 PM JST, Quaternions wrote:
> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 5b5d9f38cbb3..d456c494374d 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -364,8 +364,9 @@ struct BitHeader {
>  }
>
>  impl BitHeader {
> +    const MIN_LEN: usize = 12;
>      fn new(data: &[u8]) -> Result<Self> {
> -        if data.len() < 12 {
> +        if data.len() < Self::MIN_LEN {
>              return Err(EINVAL);
>          }

Mmm looking at the these functions makes me think that maybe we should
rather:

- Make `BitHeader` (and the other structs affected by this patch)
  `#[repr(C)]`,
- Wait until the `FromBytes` patchset lands [1],
- ... and simply implement `FromBytes` on these structures to replace
  this code, which is basically a little-endian deserializer. Since Nova
  has no plan to run on big-endian targets at the moment, this should
  work fine, and removes the need to define constants for the sizes of
  these types and use indices that are always error-prone.

Or at the very least, we make these structs `#[repr(C)]` and change the
implementation to be done in terms of `size_of` and `offset_of`, which,
while more verbose, also promises better correctness.

[1] https://lore.kernel.org/rust-for-linux/20250624042802.105623-1-christiansantoslima21@gmail.com/

Re: [PATCH] gpu: nova-core: fix bounds check In PmuLookupTableEntry::new, data is sliced from 2..6, but the bounds check data.len() < 5 does not satisfy those bounds.

[Alexandre Courbot]

On Fri Jul 11, 2025 at 9:04 PM JST, Alexandre Courbot wrote:
> Thanks for the fix!
>
> On Fri Jul 11, 2025 at 8:33 PM JST, Quaternions wrote:
>> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
>
> You will want to split the subject to something more minimal (e.g. "gpu:
> nova-core: fix bounds check In PmuLookupTableEntry::new"), and use the
> rest of the sentence as the patch description (as a rule of thumb, 70
> chars for the subject, and kernel patches must all have a description).
>
> Also your "From:" field must also match your "Signed-off-by".
>
> ./scripts/checkpatch.pl will complain about these points (and check
> other things as well), so please make sure to run it before submission:
>
>     WARNING: Missing commit description - Add an appropriate one
>     WARNING: From:/Signed-off-by: email name mismatch: 'From: Quaternions <krakow20@gmail.com>' != 'Signed-off-by: Rhys Lloyd <krakow20@gmail.com>'

Forgot to mention: please also run "./scripts/get_maintainer.pl" on
your patches to get the list of recipients you should send them to.

[PATCH] gpu: nova-core: define named constants for magic numbers

[Rhys Lloyd]

Introduce an associated constant `MIN_LEN` for each struct that checks
the length of the input data in its constructor against a magic number.

Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
---
Changes in v2:
- Add commit description
- Fix author to match SoB
- Add base commit

---
 drivers/gpu/nova-core/vbios.rs | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 5b5d9f38cbb3..d456c494374d 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -364,8 +364,9 @@ struct BitHeader {
 }
 
 impl BitHeader {
+    const MIN_LEN: usize = 12;
     fn new(data: &[u8]) -> Result<Self> {
-        if data.len() < 12 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }
 
@@ -467,8 +468,9 @@ struct PciRomHeader {
 }
 
 impl PciRomHeader {
+    const MIN_LEN: usize = 26;
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
-        if data.len() < 26 {
+        if data.len() < Self::MIN_LEN {
             // Need at least 26 bytes to read pciDataStrucPtr and sizeOfBlock.
             return Err(EINVAL);
         }
@@ -772,10 +774,11 @@ fn into_image(self) -> Result<BiosImage> {
         BiosImage::try_from(self)
     }
 
+    const MIN_LEN: usize = 26;
     /// Creates a new BiosImageBase from raw byte data.
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
         // Ensure we have enough data for the ROM header.
-        if data.len() < 26 {
+        if data.len() < Self::MIN_LEN {
             dev_err!(pdev.as_ref(), "Not enough data for ROM header\n");
             return Err(EINVAL);
         }
@@ -900,8 +903,9 @@ struct PmuLookupTableEntry {
 }
 
 impl PmuLookupTableEntry {
+    const MIN_LEN: usize = 6;
     fn new(data: &[u8]) -> Result<Self> {
-        if data.len() < 6 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }
 
@@ -928,8 +932,9 @@ struct PmuLookupTable {
 }
 
 impl PmuLookupTable {
+    const MIN_LEN: usize = 4;
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
-        if data.len() < 4 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }
 

base-commit: 215a3f91713383a3c0d2da82d223a608a3c17ac1
prerequisite-patch-id: d80f92d314a0693d4c89ffb7810d9ab6990336fa
-- 
2.50.1

Re: [PATCH] gpu: nova-core: fix bounds check In PmuLookupTableEntry::new, data is sliced from 2..6, but the bounds check data.len() < 5 does not satisfy those bounds.

[Rhys Lloyd]

Hi, this is my first contribution so thanks for pointing out my
mistakes.  It looks like there was only one newline between the commit
message and description, so the description spilled into the message.
If `git send-email` was supposed to add a From: line, I couldn't tell
you why it didn't, but I'll change the commit author to my full name
instead of my handle.

> Regarding the patch itself: is there a way this could be done better?

As Alexandre Courbot mentioned in the other patch, the solution that
fully addresses the issue will be to use `FromBytes` once it lands for
structs with no padding holes.

I don't know if I can submit v2 in a reply, so I'll attempt another
`send-email` shortly.


On Fri, Jul 11, 2025 at 5:20 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> On Fri Jul 11, 2025 at 9:04 PM JST, Alexandre Courbot wrote:
> > Thanks for the fix!
> >
> > On Fri Jul 11, 2025 at 8:33 PM JST, Quaternions wrote:
> >> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
> >
> > You will want to split the subject to something more minimal (e.g. "gpu:
> > nova-core: fix bounds check In PmuLookupTableEntry::new"), and use the
> > rest of the sentence as the patch description (as a rule of thumb, 70
> > chars for the subject, and kernel patches must all have a description).
> >
> > Also your "From:" field must also match your "Signed-off-by".
> >
> > ./scripts/checkpatch.pl will complain about these points (and check
> > other things as well), so please make sure to run it before submission:
> >
> >     WARNING: Missing commit description - Add an appropriate one
> >     WARNING: From:/Signed-off-by: email name mismatch: 'From: Quaternions <krakow20@gmail.com>' != 'Signed-off-by: Rhys Lloyd <krakow20@gmail.com>'
>
> Forgot to mention: please also run "./scripts/get_maintainer.pl" on
> your patches to get the list of recipients you should send them to.

Re: [PATCH] gpu: nova-core: fix bounds check In PmuLookupTableEntry::new, data is sliced from 2..6, but the bounds check data.len() < 5 does not satisfy those bounds.

[Miguel Ojeda]

On Fri, Jul 11, 2025 at 4:34 PM Rhys Lloyd <krakow20@gmail.com> wrote:
>
> Hi, this is my first contribution so thanks for pointing out my
> mistakes.  It looks like there was only one newline between the commit
> message and description, so the description spilled into the message.

You're welcome!

> If `git send-email` was supposed to add a From: line, I couldn't tell
> you why it didn't, but I'll change the commit author to my full name
> instead of my handle.

Yeah, if you have the correct Git author in the commit, then it should
all work smoothly.

> I don't know if I can submit v2 in a reply, so I'll attempt another
> `send-email` shortly.

Personally I suggest to avoid nesting threads, especially when there
is a long discussion or they are a long series, and instead sending
versions as independent email threads -- it reads better in some
clients (including in lore.kernel.org, in my opinion).

As for `send-email`, yeah, I would suggest always using the same way
to submit your patches -- doing things differently is prone to
mistakes. You may also want to check `b4` for that, by the way.

I hope that helps.

Cheers,
Miguel

[PATCH] gpu: nova-core: define named constants for magic numbers

[Rhys Lloyd]

Introduce an associated constant `MIN_LEN` for each struct that checks
the length of the input data in its constructor against a magic number.

Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
---
Changes in v2:
- Add commit description
- Fix author to match SoB
- Add base commit

---
 drivers/gpu/nova-core/vbios.rs | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 5b5d9f38cbb3..d456c494374d 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -364,8 +364,9 @@ struct BitHeader {
 }
 
 impl BitHeader {
+    const MIN_LEN: usize = 12;
     fn new(data: &[u8]) -> Result<Self> {
-        if data.len() < 12 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }
 
@@ -467,8 +468,9 @@ struct PciRomHeader {
 }
 
 impl PciRomHeader {
+    const MIN_LEN: usize = 26;
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
-        if data.len() < 26 {
+        if data.len() < Self::MIN_LEN {
             // Need at least 26 bytes to read pciDataStrucPtr and sizeOfBlock.
             return Err(EINVAL);
         }
@@ -772,10 +774,11 @@ fn into_image(self) -> Result<BiosImage> {
         BiosImage::try_from(self)
     }
 
+    const MIN_LEN: usize = 26;
     /// Creates a new BiosImageBase from raw byte data.
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
         // Ensure we have enough data for the ROM header.
-        if data.len() < 26 {
+        if data.len() < Self::MIN_LEN {
             dev_err!(pdev.as_ref(), "Not enough data for ROM header\n");
             return Err(EINVAL);
         }
@@ -900,8 +903,9 @@ struct PmuLookupTableEntry {
 }
 
 impl PmuLookupTableEntry {
+    const MIN_LEN: usize = 6;
     fn new(data: &[u8]) -> Result<Self> {
-        if data.len() < 6 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }
 
@@ -928,8 +932,9 @@ struct PmuLookupTable {
 }
 
 impl PmuLookupTable {
+    const MIN_LEN: usize = 4;
     fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
-        if data.len() < 4 {
+        if data.len() < Self::MIN_LEN {
             return Err(EINVAL);
         }
 

base-commit: 215a3f91713383a3c0d2da82d223a608a3c17ac1
prerequisite-patch-id: d80f92d314a0693d4c89ffb7810d9ab6990336fa
-- 
2.50.1

Re: [PATCH] gpu: nova-core: define named constants for magic numbers

[Alexandre Courbot]

On Sun Jul 13, 2025 at 11:51 AM JST, Rhys Lloyd wrote:
> Introduce an associated constant `MIN_LEN` for each struct that checks
> the length of the input data in its constructor against a magic number.
>
> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>

As I mentioned in [1], I think this would be better addressed by working
in terms of `sizeof` upon the relevant structures, after making them
`#[repr(C)]`. It might require splitting them a bit since some contain
other data (or we can maybe turn them into DSTs).

[1] https://lore.kernel.org/rust-for-linux/DB97X8JAJFI4.3G1I8ZPC1MWLS@nvidia.com/

Re: [PATCH] gpu: nova-core: define named constants for magic numbers

[Rhys Lloyd]

On 7/13/25 8:11 PM, Alexandre Courbot wrote:
> On Sun Jul 13, 2025 at 11:51 AM JST, Rhys Lloyd wrote:
>> Introduce an associated constant `MIN_LEN` for each struct that checks
>> the length of the input data in its constructor against a magic number.
>>
>> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
> As I mentioned in [1], I think this would be better addressed by working
> in terms of `sizeof` upon the relevant structures, after making them
> `#[repr(C)]`. It might require splitting them a bit since some contain
> other data (or we can maybe turn them into DSTs).
>
> [1] https://lore.kernel.org/rust-for-linux/DB97X8JAJFI4.3G1I8ZPC1MWLS@nvidia.com/

As far as I can tell, only one of the five structs with `MIN_LEN` have 
the same layout in-memory as they do in the `data` byte slice, that 
being `BitHeader`.  Perhaps `#[repr(packed)]` could be used for 
`PmuLookupTableEntry`, sacrificing alignment, but that is undesirable as 
it comes with its own footguns such as unaligned loads.  The other 
structs include optional values and vectors which do not have the same 
encoding when reading from the `data` byte slice as they do in memory.  
I have worked with DSTs before, but I don't recommend them for 
non-library code since they are not first-class citizens in Rust.  
Notably the fat pointer is not resized when taking a reference to the 
unsized struct field, and constructing such objects is cumbersome.  
Also, in the current version of Rust (1.88), DSTs cannot yet live 
comfortably on the stack.

This patch can be dropped if it's not valuable enough to warrant the 
change, I only made it because of your comment here: 
https://gitlab.freedesktop.org/drm/nova/-/merge_requests/4#note_2999761

Re: [PATCH] gpu: nova-core: define named constants for magic numbers

[Joel Fernandes]

On 7/11/2025 8:18 AM, Alexandre Courbot wrote:
> On Fri Jul 11, 2025 at 8:33 PM JST, Quaternions wrote:
>> Signed-off-by: Rhys Lloyd <krakow20@gmail.com>
>> ---
>>  drivers/gpu/nova-core/vbios.rs | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
>> index 5b5d9f38cbb3..d456c494374d 100644
>> --- a/drivers/gpu/nova-core/vbios.rs
>> +++ b/drivers/gpu/nova-core/vbios.rs
>> @@ -364,8 +364,9 @@ struct BitHeader {
>>  }
>>
>>  impl BitHeader {
>> +    const MIN_LEN: usize = 12;
>>      fn new(data: &[u8]) -> Result<Self> {
>> -        if data.len() < 12 {
>> +        if data.len() < Self::MIN_LEN {
>>              return Err(EINVAL);
>>          }
> 
> Mmm looking at the these functions makes me think that maybe we should
> rather:
> 
> - Make `BitHeader` (and the other structs affected by this patch)
>   `#[repr(C)]`,
> - Wait until the `FromBytes` patchset lands [1],
> - ... and simply implement `FromBytes` on these structures to replace
>   this code, which is basically a little-endian deserializer. Since Nova
>   has no plan to run on big-endian targets at the moment, this should
>   work fine, and removes the need to define constants for the sizes of
>   these types and use indices that are always error-prone.
> 
> Or at the very least, we make these structs `#[repr(C)]` and change the
> implementation to be done in terms of `size_of` and `offset_of`, which,
> while more verbose, also promises better correctness.
> 
> [1] https://lore.kernel.org/rust-for-linux/20250624042802.105623-1-christiansantoslima21@gmail.com/

I agree. I wish for FromBytes to be merged soon because we are also continuing
to use our own version of it in several new and upcoming code paths in
nova-core.  A conversion to using FromBytes and simplifying the vbios.rs code
(along with repr(C)) is much appreciated. Maybe a patchset like that can
reference the in-flight FromBytes patches.

thanks,

 - Joel