Paul Fagerburg has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/googl/hatch: update CRC calculation for correctness ......................................................................
util/mb/googl/hatch: update CRC calculation for correctness
The CRC result is treated as a signed value, and so in certain situations, the calculated value for the last four digits will not be correct. Ensure that the CRC is treated as an unsigned 32-bit value prior to converting the last 4 decimal digits to a string.
Signed-off-by: Paul Fagerburg pfagerburg@chromium.org Change-Id: I92f9ce1ceb7450f90b89c94e0ace6f79a9419b42 --- M util/mainboard/google/hatch/kconfig.py 1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35604/1
diff --git a/util/mainboard/google/hatch/kconfig.py b/util/mainboard/google/hatch/kconfig.py index ecc24ee..2a2972d 100755 --- a/util/mainboard/google/hatch/kconfig.py +++ b/util/mainboard/google/hatch/kconfig.py @@ -64,8 +64,9 @@ converted to all uppercase as part of this function.""" hwid = variant_name + ' test' upperhwid = hwid.upper() - suffix = zlib.crc32(upperhwid.encode('UTF-8')) % 10000 - gbb_hwid = upperhwid + ' ' + str(suffix).zfill(4) + # Force conversion to unsigned by bitwise AND with (2^32)-1 + crc = zlib.crc32(upperhwid.encode('UTF-8')) & 0xffffffff + gbb_hwid = upperhwid + ' ' + str(crc % 10000).zfill(4) return gbb_hwid
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/googl/hatch: update CRC calculation for correctness ......................................................................
Patch Set 1:
(1 comment)
Interesting catch, looks like it's only an issue for older versions of the library, https://docs.python.org/3/library/zlib.html
https://review.coreboot.org/c/coreboot/+/35604/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35604/1//COMMIT_MSG@7 PS1, Line 7: googl google
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/googl/hatch: update CRC calculation for correctness ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
Interesting catch, looks like it's only an issue for older versions of the library, https://docs.python.org/3/library/zlib.html
Given that the GBB_HWID code may be used outside the chroot (for example, to check the validity of GBB_HWID strings, see b/140067412), it seemed like a good idea to put the fix in even though it's correct in the chroot.
Hello Andrew McRae, Edward O'Callaghan, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35604
to look at the new patch set (#2).
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
util/mb/google/hatch: update CRC calculation for correctness
The CRC result is treated as a signed value, and so in certain situations, the calculated value for the last four digits will not be correct. Ensure that the CRC is treated as an unsigned 32-bit value prior to converting the last 4 decimal digits to a string.
Signed-off-by: Paul Fagerburg pfagerburg@chromium.org Change-Id: I92f9ce1ceb7450f90b89c94e0ace6f79a9419b42 --- M util/mainboard/google/hatch/kconfig.py 1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35604/2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 2: Code-Review+2
Andrew McRae has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35604/2/util/mainboard/google/hatch... File util/mainboard/google/hatch/kconfig.py:
https://review.coreboot.org/c/coreboot/+/35604/2/util/mainboard/google/hatch... PS2, Line 68: . I am not sure what the original intended rules are or how the 'correct' value is determined. To me, it would seem natural to use abs() rather than a mask, and I note there are potentially 3 different values that could result: python
a = -12345678 print a % 10000
4322
print (a & 0xffffffff) % 10000
1618
print abs(a) % 10000
5678
Can you add a link to a reference that explains the definitive calculation? It seems that any previous calculations would have given a different result with a -ve value, so if it is going to change anyway, maybe using abs() is better? As a nit, the conversion isn't to an unsigned 32 bit value, but to a 31 bit value with a cleared sign bit (which is the same I guess, but if you referenced it in a language like C that allowed unsigned 32 bit values, you are only using 31 bits). Rambling on, you could also call it a signed 32 bit value that is always positive :-)
Andrew McRae has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 2:
Patch Set 2:
(1 comment)
Ah, I see the discussion reference to https://docs.python.org/3/library/zlib.html Please put this link in for context.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
Patch Set 2:
(1 comment)
Ah, I see the discussion reference to https://docs.python.org/3/library/zlib.html Please put this link in for context.
Link and minimal context added.
https://review.coreboot.org/c/coreboot/+/35604/2/util/mainboard/google/hatch... File util/mainboard/google/hatch/kconfig.py:
https://review.coreboot.org/c/coreboot/+/35604/2/util/mainboard/google/hatch... PS2, Line 68: .
I am not sure what the original intended rules are or how the 'correct' value is determined. […]
The second option (treat as unsigned) is what we've been doing in the manual process for v2 HWIDs, so that's what the code will automatically do now.
Hello Andrew McRae, Edward O'Callaghan, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35604
to look at the new patch set (#3).
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
util/mb/google/hatch: update CRC calculation for correctness
The CRC result is treated as a signed value, and so in certain situations, the calculated value for the last four digits will not be correct. Ensure that the CRC is treated as an unsigned 32-bit value prior to converting the last 4 decimal digits to a string.
Signed-off-by: Paul Fagerburg pfagerburg@chromium.org Change-Id: I92f9ce1ceb7450f90b89c94e0ace6f79a9419b42 --- M util/mainboard/google/hatch/kconfig.py 1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35604/3
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35604/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35604/1//COMMIT_MSG@7 PS1, Line 7: googl
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 4: Code-Review+1
Andrew McRae has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 5: Code-Review+1
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
Patch Set 5:
I'll get it in to fix the calculation, but Hung-te's work on changing the default behavior (fill in a v2 ID if empty) makes the entire calculation kinda obsolete, right?
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35604 )
Change subject: util/mb/google/hatch: update CRC calculation for correctness ......................................................................
util/mb/google/hatch: update CRC calculation for correctness
The CRC result is treated as a signed value, and so in certain situations, the calculated value for the last four digits will not be correct. Ensure that the CRC is treated as an unsigned 32-bit value prior to converting the last 4 decimal digits to a string.
Signed-off-by: Paul Fagerburg pfagerburg@chromium.org Change-Id: I92f9ce1ceb7450f90b89c94e0ace6f79a9419b42 Reviewed-on: https://review.coreboot.org/c/coreboot/+/35604 Reviewed-by: Andrew McRae amcrae@chromium.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/mainboard/google/hatch/kconfig.py 1 file changed, 5 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, but someone else must approve Edward O'Callaghan: Looks good to me, approved Andrew McRae: Looks good to me, but someone else must approve
diff --git a/util/mainboard/google/hatch/kconfig.py b/util/mainboard/google/hatch/kconfig.py index ecc24ee..891714b 100755 --- a/util/mainboard/google/hatch/kconfig.py +++ b/util/mainboard/google/hatch/kconfig.py @@ -64,8 +64,11 @@ converted to all uppercase as part of this function.""" hwid = variant_name + ' test' upperhwid = hwid.upper() - suffix = zlib.crc32(upperhwid.encode('UTF-8')) % 10000 - gbb_hwid = upperhwid + ' ' + str(suffix).zfill(4) + # Force conversion to unsigned by bitwise AND with (2^32)-1. + # See the docs for crc32 at https://docs.python.org/3/library/zlib.html + # for why '& 0xffffffff' is necessary. + crc = zlib.crc32(upperhwid.encode('UTF-8')) & 0xffffffff + gbb_hwid = upperhwid + ' ' + str(crc % 10000).zfill(4) return gbb_hwid
-
Andrew McRae (Code Review) -
Edward O'Callaghan (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Fagerburg (Code Review)
-
Tim Wawrzynczak (Code Review)