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

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

Change in flashrom[master]: flashrom.c, sfdp.c: Initialize dynamically allocated memory using calloc
by Nico Huber (Code Review) 26 Mar '22

26 Mar '22

Attention is currently required from: Felix Singer, Light, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62747 ) Change subject: flashrom.c, sfdp.c: Initialize dynamically allocated memory using calloc ...................................................................... Patch Set 12: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/62747/comment/779a76fd_4f8b23f6 PS9, Line 11: when later used in need_erase could result in undefined behaviour. > Maybe intitializing them with zeroes using calloc and then onlycopying "flashrom bug!" once should b […] That would depend much on the kind of bug we are trying to catch, i.e. do we expect flashrom could accidentally write the first bytes of a chip or accidentally write any bytes? -- To view, visit https://review.coreboot.org/c/flashrom/+/62747 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6b9269129968fb3b55b0d2a2e384c8a1aeba43ab Gerrit-Change-Number: 62747 Gerrit-PatchSet: 12 Gerrit-Owner: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 26 Mar 2022 19:28:50 +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

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

26 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: Assert unsigned types >=0 on to prevent underflow error ...................................................................... Patch Set 12: Code-Review+1 -- 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: 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: Sat, 26 Mar 2022 19:24:48 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

26 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: Assert unsigned types >=0 on to prevent underflow error ...................................................................... Patch Set 12: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/8a2a5728_fe226634 PS6, Line 507: assert(j >= 12); > One difference that I see, is for Sunrise Point: `num_regions` is 10 (assigned at line 492 in curren […] If `num_regions` is 10, the second loop never runs. This doesn't change because `j = 12;` can only increase `j`. -- 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> 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: Sat, 26 Mar 2022 19:24:41 +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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2022