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.
Subsystems (4)
PATCH 1/1 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.
--- drivers/gpu/nova-core/vbios.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/nova-core/vbios.rs +1 -1
@@ -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 1/1 gpu: nova-core: define named constants for magic numbers
--- drivers/gpu/nova-core/vbios.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/gpu/nova-core/vbios.rs +10 -5
@@ -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
Alexandre Courbot
maintainerRe: [PATCH] gpu: nova-core: define named constants for magic numbers
Quoted 10 lines · Quaternions
Signed-off-by Rhys Lloyd <[email protected]>--- drivers/gpu/nova-core/vbios.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)drivers/gpu/nova-core/vbios.rs +0
-0
drivers/gpu/nova-core/vbios.rs struct BitHeader { · line 371
Quoted 9 lines
drivers/gpu/nova-core/vbios.rs +2
-1@@ -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/[email protected]/
Re: [PATCH] gpu: nova-core: define named constants for magic numbers
Quoted 33 lines · Alexandre Courbot
Quoted 10 lines · Quaternions
Signed-off-by Rhys Lloyd <[email protected]>--- drivers/gpu/nova-core/vbios.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)drivers/gpu/nova-core/vbios.rs +0
-0drivers/gpu/nova-core/vbios.rs struct BitHeader { · line 371
Quoted 9 lines
drivers/gpu/nova-core/vbios.rs +2
-1@@ -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 theFromBytespatchset lands [1],
- ... and simply implementFromByteson 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.
Quoted 5 lines
Or at the very least, we make these structs
#[repr(C)]and change the implementation to be done in terms ofsize_ofandoffset_of, which, while more verbose, also promises better correctness.[1] https://lore.kernel.org/rust-for-linux/[email protected]/
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
Quoted 4 lines · Quaternions
Signed-off-by Rhys Lloyd <[email protected]>--- 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
Alexandre Courbot
maintainerThanks for the fix!
Quoted 1 line · Quaternions
Signed-off-by Rhys Lloyd <[email protected]>
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 <[email protected]>' != 'Signed-off-by: Rhys Lloyd <[email protected]>'
Alexandre Courbot
maintainerQuoted 10 lines · Alexandre Courbot
Thanks for the fix!
Quoted 1 line · Quaternions
Signed-off-by Rhys Lloyd <[email protected]>You will want to split the subject to something more minimal (e.g. "gpu:
nova-core: fix bounds check InPmuLookupTableEntry::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).
Quoted 7 lines
Also your "From:" field must also match your "Signed-off-by".
./scripts/checkpatch.plwill 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 <[email protected]>' != 'Signed-off-by: Rhys Lloyd <[email protected]>'
Forgot to mention: please also run "./scripts/get_maintainer.pl" on your patches to get the list of recipients you should send them to.
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.
Quoted 1 line
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.
Quoted 22 lines · Alexandre Courbot
Quoted 10 lines · Alexandre Courbot
Thanks for the fix!
Quoted 1 line · Quaternions
Signed-off-by Rhys Lloyd <[email protected]>You will want to split the subject to something more minimal (e.g. "gpu:
nova-core: fix bounds check InPmuLookupTableEntry::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).Quoted 7 lines
Also your "From:" field must also match your "Signed-off-by".
./scripts/checkpatch.plwill 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 <[email protected]>' != 'Signed-off-by: Rhys Lloyd <[email protected]>'Forgot to mention: please also run "
./scripts/get_maintainer.pl" on your patches to get the list of recipients you should send them to.
Quoted 4 lines · 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.
You're welcome!
Quoted 3 lines
If
git send-emailwas 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.
Quoted 2 lines
I don't know if I can submit v2 in a reply, so I'll attempt another
send-emailshortly.
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
Quoted 2 lines · Quaternions
Signed-off-by Rhys Lloyd <[email protected]>
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