Attention is currently required from: Ricardo Quesada, Paul Menzel, Tim Wawrzynczak, Patrick Rudolph.
Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56403 )
Change subject: Move post_codes.h to commonlib/console/
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/56403
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie48c4b1d01474237d007c47832613cf1d4a86ae1
Gerrit-Change-Number: 56403
Gerrit-PatchSet: 3
Gerrit-Owner: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Ricardo Quesada <ricardoq(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Wed, 28 Jul 2021 23:07:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Patrick Rudolph.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56343 )
Change subject: security/lockdown: Allow to lock the controller's full address space
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> It seems (I only had a quick look at the code) that CONFIG_ROM_SIZE is taken into account. What happens if that Kconfig is not correct?
The underlying rdevs for boot_device_ro() and _rw() are defined via CONFIG_ROM_SIZE, so if it is defined incorrectly and part of the flash isn't protected, at least coreboot shouldn't be able to access those parts (the rdev API enforces range checks on everything) and thus shouldn't be vulnerable to it. spi_flash_probe() will also yell if the detected SPI part size doesn't match CONFIG_ROM_SIZE.
> What happens if the bootmedia code changes and some other mechanism is used?
I mean... I think you can ask that about any piece of code anywhere. Theoretically anything could break. We just have to use good engineering practices and unit tests and whatever to try to prevent that.
When some people want features and some people want simplicity, you always have to meet somewhere in the middle. Where it's possible to cleanly isolate the more complicated features so that they can be turned off and only leave the simpler parts behind we should do that, but in this case I don't really see a better way to design it. (I'd say this patch as written doesn't fit cleanly... as I mentioned I don't think modeling this in the "protection mode" parameter makes much sense. Also, the assumption that 0x0 to 0xffffffff are valid bounds may work on the Intel controllers but if we ever have other platforms with controller-based protection, they might insist on bounds that actually match the flash range and then you have to deal with that abstraction break again.)
> I see two options: 1. Just turn the LOCK_WHOLE_RO behavior into what we need; i.e. not taking the flash size into account. 2. keep our solution downstream or move it into the mainboard ports.
Yeah, I mean, like I said... if you really care I'd be okay with hacking this directly into fast_spi_flash.c (mostly because it's an x86-specific file and I don't personally care as much about those). I just don't think we should spread it out all across the code like this.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56343
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib4193e37b072b0f9e4ce69fa6ae15b6bcf26eec2
Gerrit-Change-Number: 56343
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Gröber <dxld(a)darkboxed.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Wed, 28 Jul 2021 23:06:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/56643 )
Change subject: util/xcompile: Allow overriding default compiler path
......................................................................
util/xcompile: Allow overriding default compiler path
When looking for C compilers, xcompile uses the "" prefix to "gcc" and
"clang" as a last-resort option. This fails in environments where such
default names are blocked to prevent "unclean" builds - such as Chrome
OS.
Allow overriding this prefix using the GENERIC_COMPILER_PREFIX variable
that is hopefully both descriptive enough to suggest what it is for and
unusual enough to not trigger by chance.
Change-Id: I16239f66730f1dbcb7482f223cea4ee5957af10c
Signed-off-by: Patrick Georgi <pgeorgi(a)google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/56643
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Martin Roth <martinroth(a)google.com>
---
M util/xcompile/xcompile
1 file changed, 4 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Martin Roth: Looks good to me, approved
diff --git a/util/xcompile/xcompile b/util/xcompile/xcompile
index 9fdda03..4950a9e 100755
--- a/util/xcompile/xcompile
+++ b/util/xcompile/xcompile
@@ -10,6 +10,8 @@
set -x
fi
+# GENERIC_COMPILER_PREFIX defaults to empty but can be used to override
+# compiler search behavior
TMPFILE=""
XGCCPATH=$1
@@ -421,7 +423,7 @@
# Search toolchain by checking assembler capability.
for TBFDARCH in $TBFDARCHS; do
- for gccprefix in $search ""; do
+ for gccprefix in $search "$GENERIC_COMPILER_PREFIX"; do
program_exists "${gccprefix}as" || continue
for endian in $TENDIAN ""; do
{ testas "$gccprefix" "$TWIDTH" "$TBFDARCH" \
@@ -439,7 +441,7 @@
fi
for clang_arch in $TCLIST invalid; do
- for clang_prefix in $search $XGCCPATH ""; do
+ for clang_prefix in $search $XGCCPATH "$GENERIC_COMPILER_PREFIX"; do
testcc "${clang_prefix}clang" "-target ${clang_arch}-$TABI -c" && break 2
done
done
--
To view, visit https://review.coreboot.org/c/coreboot/+/56643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I16239f66730f1dbcb7482f223cea4ee5957af10c
Gerrit-Change-Number: 56643
Gerrit-PatchSet: 2
Gerrit-Owner: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: merged
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/56630 )
Change subject: mb/google/brya/var/kano: Generate SPD ID for supported parts
......................................................................
mb/google/brya/var/kano: Generate SPD ID for supported parts
Add supported memory parts in mem_parts_used.txt, and generate
SPD id for these parts.
MT53E512M32D1NP-046 WT:B
MT53E1G32D2NP-046 WT:B
H54G46CYRBX267
H54G56CYRBX247
K4U6E3S4AB-MGCL
K4UBE3D4AB-MGCL
BUG=b:194766276 b:194686484 b:194765811
TEST=build
Signed-off-by: David Wu <david_wu(a)quanta.corp-partner.google.com>
Change-Id: Iba019c50224be8322865eee7baf81e3a574ff9a4
Reviewed-on: https://review.coreboot.org/c/coreboot/+/56630
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Reviewed-by: Nick Vaccaro <nvaccaro(a)google.com>
---
M src/mainboard/google/brya/variants/kano/memory/Makefile.inc
M src/mainboard/google/brya/variants/kano/memory/dram_id.generated.txt
M src/mainboard/google/brya/variants/kano/memory/mem_parts_used.txt
3 files changed, 15 insertions(+), 13 deletions(-)
Approvals:
build bot (Jenkins): Verified
Nick Vaccaro: Looks good to me, approved
Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/google/brya/variants/kano/memory/Makefile.inc b/src/mainboard/google/brya/variants/kano/memory/Makefile.inc
index b0ca222..a81fb3b 100644
--- a/src/mainboard/google/brya/variants/kano/memory/Makefile.inc
+++ b/src/mainboard/google/brya/variants/kano/memory/Makefile.inc
@@ -1,5 +1,6 @@
## SPDX-License-Identifier: GPL-2.0-or-later
## This is an auto-generated file. Do not edit!!
-## Add memory parts in mem_parts_used.txt and run spd_tools to regenerate.
-SPD_SOURCES = placeholder.spd.hex
+SPD_SOURCES =
+SPD_SOURCES += lp4x-spd-1.hex # ID = 0(0b0000) Parts = MT53E512M32D1NP-046 WT:B, H54G46CYRBX267, K4U6E3S4AB-MGCL
+SPD_SOURCES += lp4x-spd-3.hex # ID = 1(0b0001) Parts = MT53E1G32D2NP-046 WT:B, H54G56CYRBX247, K4UBE3D4AB-MGCL
diff --git a/src/mainboard/google/brya/variants/kano/memory/dram_id.generated.txt b/src/mainboard/google/brya/variants/kano/memory/dram_id.generated.txt
index fa24790..ed9007d 100644
--- a/src/mainboard/google/brya/variants/kano/memory/dram_id.generated.txt
+++ b/src/mainboard/google/brya/variants/kano/memory/dram_id.generated.txt
@@ -1 +1,7 @@
DRAM Part Name ID to assign
+MT53E512M32D1NP-046 WT:B 0 (0000)
+MT53E1G32D2NP-046 WT:B 1 (0001)
+H54G46CYRBX267 0 (0000)
+H54G56CYRBX247 1 (0001)
+K4U6E3S4AB-MGCL 0 (0000)
+K4UBE3D4AB-MGCL 1 (0001)
diff --git a/src/mainboard/google/brya/variants/kano/memory/mem_parts_used.txt b/src/mainboard/google/brya/variants/kano/memory/mem_parts_used.txt
index 9cff262..a188982 100644
--- a/src/mainboard/google/brya/variants/kano/memory/mem_parts_used.txt
+++ b/src/mainboard/google/brya/variants/kano/memory/mem_parts_used.txt
@@ -1,11 +1,6 @@
-# This is a CSV file containing a list of memory parts used by this variant.
-# One part per line with an optional fixed ID in column 2.
-# Only include a fixed ID if it is required for legacy reasons!
-# Generated IDs are dependent on the order of parts in this file,
-# so new parts must always be added at the end of the file!
-#
-# Generate an updated Makefile.inc and dram_id.generated.txt by running the
-# gen_part_id tool from util/spd_tools/lp4x.
-# See util/spd_tools/lp4x/README.md for more details and instructions.
-
-# Part Name
+MT53E512M32D1NP-046 WT:B
+MT53E1G32D2NP-046 WT:B
+H54G46CYRBX267
+H54G56CYRBX247
+K4U6E3S4AB-MGCL
+K4UBE3D4AB-MGCL
--
To view, visit https://review.coreboot.org/c/coreboot/+/56630
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iba019c50224be8322865eee7baf81e3a574ff9a4
Gerrit-Change-Number: 56630
Gerrit-PatchSet: 3
Gerrit-Owner: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/56500 )
Change subject: mb/google/volteer/variants/drobit: Add Charger Performance Control table TCHG for DPTF setting.
......................................................................
mb/google/volteer/variants/drobit: Add Charger Performance Control table TCHG for DPTF setting.
Add Charger Performance Control table TCHG for DPTF setting.
BUG=b:194256990
BRANCH=firmware-volteer-13672.B
TEST=build test firmware and verified by thermal team.
Change-Id: I9dba3f0e75d07d8ee9656bd1ee8d6de2d3b8c152
Signed-off-by: Wayne3 Wang <Wayne3_Wang(a)pegatron.corp-partner.google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/56500
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Ariel Chang <ariel_chang(a)pegatron.corp-partner.google.com>
Reviewed-by: Paul F Yang <paul.f.yang(a)intel.com>
Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Reviewed-by: Wayne3 Wang <wayne3_wang(a)pegatron.corp-partner.google.com>
---
M src/mainboard/google/volteer/variants/drobit/overridetree.cb
1 file changed, 8 insertions(+), 0 deletions(-)
Approvals:
build bot (Jenkins): Verified
Tim Wawrzynczak: Looks good to me, approved
Wayne3 Wang: Looks good to me, but someone else must approve
Ariel Chang: Looks good to me, but someone else must approve
Paul F Yang: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/volteer/variants/drobit/overridetree.cb b/src/mainboard/google/volteer/variants/drobit/overridetree.cb
index 8ae198c..d779a3c 100644
--- a/src/mainboard/google/volteer/variants/drobit/overridetree.cb
+++ b/src/mainboard/google/volteer/variants/drobit/overridetree.cb
@@ -102,6 +102,14 @@
register "options.fan.fine_grained_control" = "1"
register "options.fan.step_size" = "2"
+ ## Charger Performance Control (Control, mA)
+ register "controls.charger_perf" = "{
+ [0] = { 32, 2048 },
+ [1] = { 29, 1856 },
+ [2] = { 26, 1664 },
+ [3] = { 23, 1472 }
+ }"
+
device generic 0 on end
end
end # DPTF
--
To view, visit https://review.coreboot.org/c/coreboot/+/56500
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9dba3f0e75d07d8ee9656bd1ee8d6de2d3b8c152
Gerrit-Change-Number: 56500
Gerrit-PatchSet: 3
Gerrit-Owner: Wayne3 Wang <wayne3_wang(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Ariel Chang <ariel_chang(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Hao Chou <hao_chou(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Ken Lu <ken_lu(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Paul F Yang <paul.f.yang(a)intel.com>
Gerrit-Reviewer: Paul2 Huang <paul2_huang(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Wayne3 Wang <wayne3_wang(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Alex1 Kao <alex1_kao(a)pegatron.corp-partner.google.com>
Gerrit-CC: Kirk Wang <kirk_wang(a)pegatron.corp-partner.google.com>
Gerrit-MessageType: merged
Attention is currently required from: Jakub Czapiga, Jan Dabros.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56601 )
Change subject: tests: Add lib/cbfs-verification-test test case
......................................................................
Patch Set 2:
(11 comments)
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56601/comment/4ad10e5e_8aef14d5
PS2, Line 198: CONFIG_CBFS_VERIFICATION=1 \
It would be great if you could build this test twice, once with CONFIG_CBFS_VERIFICATION=1 and once with CONFIG_CBFS_VERIFICATION=0. Then use `if CONFIG(CBFS_VERIFICATION)` in the test file to change the expected result for the two, making sure that all the cases work both with and without the feature enabled (basically, all cases should succeed with verification disabled, regardless of whether there is no hash, a good hash or a bad hash).
https://review.coreboot.org/c/coreboot/+/56601/comment/8567ed82_d022a484
PS2, Line 199: CONFIG_NO_CBFS_MCACHE=0 \
It's a bit odd that you enable the MCACHE here but leave it practically disabled in most of the tests (by having mcache_size = 0), which always makes the code go into the "fallback" path (and there would be angry warning messages for every file access if the test framework didn't swallow them). I think it would make more sense to either actually use the mcache, or disable it completely. (There should probably also be tests specific to the mcache behavior somewhere that check all the edge cases of mcache enabled, mcache disabled, mcache enabled but varying degrees of too small, etc., but that's probably separate enough from verification to warrant a separate test.)
I guess you have this for the cbfs_init_boot_device() stuff... maybe it makes more sense to move that to a separate test?
https://review.coreboot.org/c/coreboot/+/56601/comment/0823ed94_dd5d781c
PS2, Line 202: VB2_SUPPORT_SHA512=0
Are these necessary? By default vboot assumes that all three are enabled and that's how coreboot uses it. (This will mean that struct vb2_hash is longer than it needs to be for a SHA256, and that is also how coreboot uses it, so it would be good to test that. Be aware that the CBFS_FILE_ATTR_TAG_HASH CBFS attribute is variable length and will always be exactly the length needed to store the embedded hash. Code should never use sizeof() on a struct cbfs_file_attr_tag_hash, because the size of that struct may not match the size of the actual attribute in flash.)
Actually, now that I think of it, would probably be a good idea to build this test twice (once with SHA512 enabled and once with it disabled) to test that the difference in struct size has no impact on the code and it handles both cases correctly (as it was written to do).
File tests/lib/cbfs-verification-test.c:
https://review.coreboot.org/c/coreboot/+/56601/comment/b5a52a7d_cac6c024
PS2, Line 4: #define __noreturn
What's this? __noreturn comes from src/commonlib/bsd/include/commonlib/bsd/compiler.h which tests/Makefile.inc is already including, so it should be available here.
https://review.coreboot.org/c/coreboot/+/56601/comment/a407fbba_0f8a20cc
PS2, Line 38: #define HASH_ATTR_SIZE (sizeof(struct cbfs_file_attr_hash))
Oh, this is the VB2_SUPPORT_SHA512 issue. Okay, yeah, this needs to be defined differently. I'd suggest (offsetof(struct cbfs_file_attr_hash, hash.raw) + VB2_SHA256_DIGEST_SIZE).
https://review.coreboot.org/c/coreboot/+/56601/comment/a3816662_768e46cc
PS2, Line 121: {
These should also use the check...() functions to make sure they're only called when intended. (Or, if this should not get called at all, just make it call fail().)
https://review.coreboot.org/c/coreboot/+/56601/comment/cde3db9e_cd30c1cf
PS2, Line 167: assert_null(mapping);
It would kinda be nice if we could check the exact error code returned from cbfs_lookup() for these... is there any way to do that with Cmocka, but still run the underlying code? I guess with the way the objcopy weaking mocks work now, you can't both mock a function and still call the underlying to make a little wrapper that checks arguments? Maybe it would be nice to add that, not sure if we can come up with any further objcopy tricks to make that work? Maybe using --add-symbol to define a (non-weak) __real_<functionname> symbol for the same address as each <functionname> that's marked as mockable?
https://review.coreboot.org/c/coreboot/+/56601/comment/87613b47_c0d1a30f
PS2, Line 181: assert_memory_equal(mapping, &test_data, TEST_DATA_SIZE);
nit: Feels a bit odd that you test pointer equality above and only content equality down here. Not really sure which one is better (I guess pointer equality is stricter and since we know we have a mem_rdev we can do it?), but I'd make it consistent (i.e. either change it to assert_ptr_equal() down here or expect_memory() above).
https://review.coreboot.org/c/coreboot/+/56601/comment/72eb0178_02a414ac
PS2, Line 235: assert_int_equal(CB_SUCCESS, cbfs_init_boot_device(&cbd, &hash));
Oh, and this is the one where you test it with a metadata_hash. Okay. (Sorry, I wrote the comments below first.)
The name invalid_mcache_size is quite confusing here, I would rather call it test_init_boot_device_verify_no_mcache() and the other test_init_boot_device_no_verify_has_mcache(), because that's sort of the relevant difference here. And then of course it's kinda weird that you're missing the complementary cases test_init_boot_device_no_verify_no_mcache() and test_init_boot_device_verify_has_mcache().
https://review.coreboot.org/c/coreboot/+/56601/comment/d242fd95_0d8f6523
PS2, Line 243: /* No cache, so no validation required */
Not really sure what these comments mean. When CBFS_VERIFICATION is enabled then verification is always required. The mcache caches the metadata, so when an mcache is used the metadata is only verified once when it is initially being read into the cache, whereas with no mcache the metadata of the whole CBFS is verified every time a file is loaded. But it must always be verified at least once.
If the idea here is to validate that the mcache is built correctly (although that would be more of an mcache test than a verification test at that point), I'd assert_memory_equal() the generated mcache with what's expected at the end (although that test is pretty boring with a single CBFS file, we'd probably want to put that into a different test that mocks a larger CBFS).
https://review.coreboot.org/c/coreboot/+/56601/comment/93c0097f_eda03936
PS2, Line 244: NULL
The real reason this succeeds without verification is because you're passing NULL here. Passing NULL as the metadata_hash to cbfs_walk() means "don't hash the metadata"). The real lib/cbfs.c should never pass NULL to cbfs_init_boot_device() when CBFS_VERIFICATION is enabled.
Anyway, I'm not really clear on what you're trying to do here... I thought this was just focused on testing file verification in isolation first. If you want to test metadata verification as well that's great too, but then you'll need to have a valid metadata hash too. You could use cbfs_walk() with CBFS_WALK_WRITEBACK_HASH (like in util/cbfstool/cbfstool.c:maybe_update_metadata_hash()) to generate that on the fly. (I'm not really sure how to best test that, it probably makes sense to break some of that out into a separate test that's built with slightly different mocks. Since this test stubs out vb2_hash_verify() anyway, there's little reason to generate valid hashes in here. This is more about confirming that the right data buffers are passed into vb2_hash_verify() in the right order. It would probably be good to have a separate, similar test where vb2_hash_verify() isn't stubbed out, and all the mock SHA256s for both metadata and files are real, and then you start randomly flipping some bits in different parts of the data to ensure that the verification catches each of them.)
--
To view, visit https://review.coreboot.org/c/coreboot/+/56601
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1d8cbb1c2d0a9db3236de065428b70a9c2a66330
Gerrit-Change-Number: 56601
Gerrit-PatchSet: 2
Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Reviewer: Jan Dabros <jsd(a)semihalf.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Attention: Jan Dabros <jsd(a)semihalf.com>
Gerrit-Comment-Date: Wed, 28 Jul 2021 21:58:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment