Attention is currently required from: Angel Pons, Light, Nikolai Artemiev.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62733 )
Change subject: writeprotect_ranges.c: Use 1UL if left shifting more than 32bits
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
Heh, a third solution comes to mind. Sorry if this is getting too
theoretical. Just ignore this comment at your convenience. ;)
Prove that it can't happen and ignore the tool. I think that is
possible. My reasoning:
* If `bp_bit_count == 0`, then `bp == 0` and the error path isn't taken.
* If `bp_bit_count == 1`, then either `bp == 0` or `bp == 1 == bp_max` and the error path isn't taken.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62733
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2766ef8e34f7121dad746e5f32a835d480ae1cad
Gerrit-Change-Number: 62733
Gerrit-PatchSet: 4
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 18:16:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Light, Nikolai Artemiev.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62733 )
Change subject: writeprotect_ranges.c: Use 1UL if left shifting more than 32bits
......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62733/comment/ca7449dd_562a8093
PS2, Line 12: Still the bug is not fixed :(
> I don't think it would work as the max value that bp_max can take is 15 since the array bits->bp is […]
The types can't help us because an underflow or a negative `bp_max - 2` both
would result in undefined behavior. From C11 about the shift operators:
"If the value of the right operand is negative or is greater than
or equal to the width of the promoted left operand, the behavior
is undefined."
As far as I can see, the bug is only a theoretical one that depends on the
arguments passed to this function (i.e. `bits->bp_bit_count`). I see two
potential solutions: 1. Either add an assert() that would provide a contract
how this function should be called, e.g. on the top of the function:
assert(bits->bp_bit_count > 1);
2. Defensive programming. We could add an `if` that returns an error if
this happens.
Personally, I'm much a fan of 1. But I am not sure if it will be accepted
by scan-build.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62733
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2766ef8e34f7121dad746e5f32a835d480ae1cad
Gerrit-Change-Number: 62733
Gerrit-PatchSet: 4
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 17:59:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons.
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/62733
to look at the new patch set (#4).
Change subject: writeprotect_ranges.c: Use 1UL if left shifting more than 32bits
......................................................................
writeprotect_ranges.c: Use 1UL if left shifting more than 32bits
size_t is of size 64 bits, while 1 (int) is of 32 bits. Left shifting 1
by more than 32 will lead to shift-count-overflow bug. So use 1UL or
1ULL to make 1 of type unsigned long or unsigned long long respectively.
Change-Id: I2766ef8e34f7121dad746e5f32a835d480ae1cad
Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
---
M writeprotect_ranges.c
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/62733/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/62733
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2766ef8e34f7121dad746e5f32a835d480ae1cad
Gerrit-Change-Number: 62733
Gerrit-PatchSet: 4
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Angel Pons.
Light has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62733 )
Change subject: writeprotect_ranges.c: Use 1UL if left shifting more than 32bits
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62733/comment/54d8f9a0_92fec78a
PS2, Line 12: Still the bug is not fixed :(
> I wonder if casting the `1` to `size_t` would work: […]
I don't think it would work as the max value that bp_max can take is 15 since the array bits->bp is of length MAX_BP_BITS which is 4. The error arises when bp_max takes values lower than 2. Then bp_max-2 becomes very large due to underflow error since size_t is unsigned type.
Any suggestions to solve this error?
--
To view, visit https://review.coreboot.org/c/flashrom/+/62733
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2766ef8e34f7121dad746e5f32a835d480ae1cad
Gerrit-Change-Number: 62733
Gerrit-PatchSet: 3
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 16:52:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Light.
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/62733
to look at the new patch set (#3).
Change subject: writeprotect_ranges.c: Use 1UL if left shifting more than 32bits
......................................................................
writeprotect_ranges.c: Use 1UL if left shifting more than 32bits
size_t is of size 64 bits, while 1 (int) is of 32 bits. Left shifting 1
by more than 32 will lead to shift-count-overflow bug. So use 1UL or
1ULL to make 1 of type unsigned long or unsigned long long respectively.
Change-Id: I2766ef8e34f7121dad746e5f32a835d480ae1cad
Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
---
M writeprotect_ranges.c
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/62733/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/62733
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2766ef8e34f7121dad746e5f32a835d480ae1cad
Gerrit-Change-Number: 62733
Gerrit-PatchSet: 3
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Light.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62733 )
Change subject: writeprotect_ranges.c: Use 1UL if left shifting more than 32bits
......................................................................
Patch Set 2:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62733/comment/3cf38b51_f8c30c2a
PS2, Line 9: size_t is of size 64 bits
Not always, it could be 32 bits wide on 32-bit machines
https://review.coreboot.org/c/flashrom/+/62733/comment/84c69a7e_16ac5511
PS2, Line 12: Still the bug is not fixed :(
I wonder if casting the `1` to `size_t` would work:
(size_t)1 << foo
File tags:
PS2:
Did you mean to add this file?
--
To view, visit https://review.coreboot.org/c/flashrom/+/62733
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2766ef8e34f7121dad746e5f32a835d480ae1cad
Gerrit-Change-Number: 62733
Gerrit-PatchSet: 2
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 15:25:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Light.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62724 )
Change subject: pony_spi.c: Fix memory leak in function pony_init_spi
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62724/comment/2c19ea3f_97fec297
PS3, Line 11: ii
nit: just one `i`
--
To view, visit https://review.coreboot.org/c/flashrom/+/62724
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7910db94f63693e7f131836d4963e88cfdbec301
Gerrit-Change-Number: 62724
Gerrit-PatchSet: 3
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 15:18:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Light.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62724 )
Change subject: pony_spi.c: Fix memory leak in function pony_init_spi
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS2:
> Thanks for pointing out the mistakes. I have made the changes accordingly. […]
Hi, it's what I described here: https://review.coreboot.org/c/flashrom/+/62724/1..3/pony_spi.c#b167
In a nutshell, `register_shutdown(pony_spi_shutdown, data)` registers the shutdown function *and also* stores a copy of the pointer to the data to be freed. I didn't explicitly mention this second part in my other comment.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62724
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7910db94f63693e7f131836d4963e88cfdbec301
Gerrit-Change-Number: 62724
Gerrit-PatchSet: 3
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 15:17:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com>
Gerrit-MessageType: comment
Patrick Rudolph has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/62735 )
Change subject: cli_classic: Fix compilation on 32bits
......................................................................
Abandoned
Duplicate of https://review.coreboot.org/62707
--
To view, visit https://review.coreboot.org/c/flashrom/+/62735
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifb25cb8b860e1253ea34343fbe0c82977beae5f5
Gerrit-Change-Number: 62735
Gerrit-PatchSet: 1
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: abandon