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.)