March 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow ...
by Anastasia Klimchuk (Code Review) 28 Mar '22

28 Mar '22

Attention is currently required from: Angel Pons, Light. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62764 ) Change subject: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow error ...................................................................... Patch Set 14: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/62764/comment/7a02235b_7a29ee85 PS12, Line 7: ich_descriptors.c: Assert unsigned types >=0 on to prevent underflow error > Done The second part is left: to fit commit description into 72 chars, and it is 74 at the moment. I think you can just drop the last word (error), so that will be "to prevent underflow". Patchset: PS14: One more thing I wanted to say: at the moment this patch depends on the previous one, because they are in the chain and modifying the same file. But technically the diffs are independent of each other. This one seems almost ready, the previous one has an open question. So, if you want, you can either swap this patch with previous in the chain, or move it out of the chain entirely. Then it can be merged independently when it's ready. Currently, one comment left :) -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 14 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Mon, 28 Mar 2022 01:08:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: Fix voltage ranges for MX23L1654 and MX23L6454
by Felix Singer (Code Review) 27 Mar '22

27 Mar '22

Felix Singer has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/63124 ) Change subject: flashchips.c: Fix voltage ranges for MX23L1654 and MX23L6454 ...................................................................... Abandoned According to the newest version of their datasheets (v1.5 for MX23L1654 and v1.8 MX23L6454), the voltage ranges are correct. -- To view, visit https://review.coreboot.org/c/flashrom/+/63124 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I91dd6058d5951868b154c8bc8818cb0c412f2499 Gerrit-Change-Number: 63124 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

Change in flashrom[master]: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow ...
by Nico Huber (Code Review) 27 Mar '22

27 Mar '22

Attention is currently required from: Angel Pons, Light, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62764 ) Change subject: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow error ...................................................................... Patch Set 14: Code-Review+1 (2 comments) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/f097a859_1613cb06 PS6, Line 507: assert(j >= 12); > Oh yes, sorry that's all good, I missed! I am marking this as resolved then? Ack File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/46622ef5_3c079b6c PS12, Line 505: ++j > Done Sorry, my bad, I put the `++j` in a comment (it feels more natural to me). -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 14 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 27 Mar 2022 12:41:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c: Initialize structures
by Nico Huber (Code Review) 27 Mar '22

27 Mar '22

Attention is currently required from: Felix Singer, Angel Pons, Light, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62763 ) Change subject: ich_descriptors.c: Initialize structures ...................................................................... Patch Set 10: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62763/comment/f773ac74_c5f71eca PS7, Line 1364: int ret = read_ich_descriptors_from_dump(dump, len, &cs, &desc); > oh... […] It could be a false-positive (in a project where tools like scan-build find little to complain about most findings are). If it's not a false-positive, this change would cover up a bug in read_ich_descriptors_from_dump(). -- To view, visit https://review.coreboot.org/c/flashrom/+/62763 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6e5bc84c6199a0731db0a9c8ef56f1215686dab2 Gerrit-Change-Number: 62763 Gerrit-PatchSet: 10 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 27 Mar 2022 12:33:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow ...
by Light (Code Review) 27 Mar '22

27 Mar '22

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Light has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62764 ) Change subject: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow error ...................................................................... Patch Set 14: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/62764/comment/9ce5a189_ea46c314 PS12, Line 7: ich_descriptors.c: Assert unsigned types >=0 on to prevent underflow error > We need to fit this into 72 char, and it is 74 at the moment :) […] Done File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/d96c6122_12c493d3 PS12, Line 505: ++j > Is there a reason for this change (from `j++` to `++j`) ? If not (not an intentional change), then b […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 14 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 27 Mar 2022 08:58:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow ...
by Light (Code Review) 27 Mar '22

27 Mar '22

Attention is currently required from: Nico Huber, Angel Pons, Light. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62764 to look at the new patch set (#14). Change subject: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow error ...................................................................... ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow error Unsigned types show undefined behaviour if they are subtracted by a value greater than their own (mostly it wraps to the max value). Using this value for left shifting could be even more dangerous. Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com> --- M ich_descriptors.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/62764/14 -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 14 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ich_descriptors.c: Assert unsigned types >=0 on to prevent underflow ...
by Light (Code Review) 27 Mar '22

27 Mar '22

Attention is currently required from: Nico Huber, Angel Pons, Light. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62764 to look at the new patch set (#13). Change subject: ich_descriptors.c: Assert unsigned types >=0 on to prevent underflow error ...................................................................... ich_descriptors.c: Assert unsigned types >=0 on to prevent underflow error Unsigned types show undefined behaviour if they are subtracted by a value greater than their own (mostly it wraps to the max value). Using this value for left shifting could be even more dangerous. Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com> --- M ich_descriptors.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/62764/13 -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 13 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ich_descriptors.c: Initialize structures
by Anastasia Klimchuk (Code Review) 27 Mar '22

27 Mar '22

Attention is currently required from: Felix Singer, Nico Huber, Angel Pons, Light. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62763 ) Change subject: ich_descriptors.c: Initialize structures ...................................................................... Patch Set 10: Code-Review+2 (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62763/comment/18b891e9_0280ab9c PS7, Line 1364: int ret = read_ich_descriptors_from_dump(dump, len, &cs, &desc); > Hmmm, that matches what I suspected. Doesn't say we are correct, though ;) […] oh... do you mean this is false-positive? and as of right now, everything is initialised? -- To view, visit https://review.coreboot.org/c/flashrom/+/62763 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6e5bc84c6199a0731db0a9c8ef56f1215686dab2 Gerrit-Change-Number: 62763 Gerrit-PatchSet: 10 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sun, 27 Mar 2022 07:18:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c: Assert unsigned types >=0 on to prevent underflow ...
by Anastasia Klimchuk (Code Review) 27 Mar '22

27 Mar '22

Attention is currently required from: Nico Huber, Angel Pons, Light. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62764 ) Change subject: ich_descriptors.c: Assert unsigned types >=0 on to prevent underflow error ...................................................................... Patch Set 12: (2 comments) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/06fb063e_67b36cee PS6, Line 507: assert(j >= 12); > If `num_regions` is 10, the second loop never runs. This doesn't change […] Oh yes, sorry that's all good, I missed! I am marking this as resolved then? File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/0f3da6d7_81b9244a PS12, Line 505: ++j Is there a reason for this change (from `j++` to `++j`) ? If not (not an intentional change), then better return back as it was. Thanks! -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 12 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sun, 27 Mar 2022 07:16:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c: Initialize structures
by Nico Huber (Code Review) 26 Mar '22

26 Mar '22

Attention is currently required from: Felix Singer, Angel Pons, Light. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62763 ) Change subject: ich_descriptors.c: Initialize structures ...................................................................... Patch Set 10: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62763/comment/7e7e66f1_0981a506 PS7, Line 1364: int ret = read_ich_descriptors_from_dump(dump, len, &cs, &desc); > As far as I can see, all fields are getting filled before returning 0. Hmmm, that matches what I suspected. Doesn't say we are correct, though ;) My concern here is that scan-build might only complain because it isn't smart enough to see this. And if that's the case and we zero initialize the struct now, there wouldn't be anything to see if read_ich_descriptors_from_dump() would break in the future. So a potential, future, smarter scan-build or similar tool might be able to warn us, but only if we don't initialize the struct. -- To view, visit https://review.coreboot.org/c/flashrom/+/62763 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6e5bc84c6199a0731db0a9c8ef56f1215686dab2 Gerrit-Change-Number: 62763 Gerrit-PatchSet: 10 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 26 Mar 2022 19:35:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2022