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 7:
(13 comments)
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56601/comment/6249186e_387fefce PS2, Line 199: CONFIG_NO_CBFS_MCACHE=0 \
I disabled MCache for this test. It will be tested in another one. […]
Well I think file verification and metadata verification are certainly different enough things that you could have separate tests for them if you want (especially because they would likely need different mock functions and/or data). And if you do something like that more integrated overall verification test I described above, that would probably also need to be its own test file. I don't think you need to put everything "verification" into a single file -- rather, you probably want to put the things that can use the same mocks into the same file, and have stuff that needs a very different mock setup in another file.
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56601/comment/a970dfb1_d37fc54a PS7, Line 35: tests-y += cbfs-verification-no-sha512-test Should explain the rationale for testing both cases in a comment somewhere.
https://review.coreboot.org/c/coreboot/+/56601/comment/fce9d39a_6a24443e PS7, Line 188: cbfs-verification-sources := tests/lib/cbfs-verification-test.c \ I wonder if it makes sense to define a copy_test function for this sort of stuff, so that you could just do
$(call copy_test,cbfs-verification-no-sha512,cbfs-verification-has-sha512) cbfs-verification-has-sha512-config += VB2_SUPPORT_SHA512=1
to allow writing these variations with less duplication.
https://review.coreboot.org/c/coreboot/+/56601/comment/b81c125d_47b90ab1 PS7, Line 198: 3rdparty/vboot/firmware/include Shouldn't we just put these into the default include path for tests? They are in the default include path for normal coreboot, and I think tests should just use the same.
https://review.coreboot.org/c/coreboot/+/56601/comment/110d662f_d29516cb PS7, Line 219: CONFIG_CBFS_VERIFICATION=1 Wasn't the point that this should be 0 here?
File tests/lib/cbfs-verification-test.c:
https://review.coreboot.org/c/coreboot/+/56601/comment/27433d5c_d491794a PS2, Line 167: assert_null(mapping);
Done, I think. […]
Ack
https://review.coreboot.org/c/coreboot/+/56601/comment/6a9f34f0_1c407298 PS2, Line 181: assert_memory_equal(mapping, &test_data, TEST_DATA_SIZE);
Assuming we do not use cache, we can. […]
That's not how this works -- CBFS cache is used by the specific rdev's mmap() implementation, not by the core CBFS framework (at least not for this). If you're providing a mem_rdev it will just return a pointer for mmap(), not use any cache.
https://review.coreboot.org/c/coreboot/+/56601/comment/96746d5b_8a505712 PS2, Line 244: NULL
I have seen cbfs_init_boot_device(cbd, NULL) in ./src/security/vboot/vboot_loader.c, so I tought tat it would be nice to include it in the test as well.
Yeah that's a stopgap, that's why it says // TODO. It basically means CBFS_VERIFICATION is force-disabled for RW CBFSes for now. Once we implement that part, it will pass a valid hash there.
If you want to have a test for cbfs_walk() where all the vb2_digest...() stuff is mocked out you can certainly do that. But I would at some point definitely like to have a test that tests the whole verification system with real hashes and real hash functions. Call it a higher level "integration test" if you want (but I think it can use this same test framework), it's just good to test that all these things can work together to fulfill their intended purpose and don't just work in isolation (when you mock too many things, there's always a danger that you mocked them wrong and they don't represent the real case). I think I'd consider that test more important than a completely bare-bones, fully mocked-out cbfs_walk() test, but I certainly wouldn't mind having both.
File tests/lib/cbfs-verification-test.c:
https://review.coreboot.org/c/coreboot/+/56601/comment/ec7c8b23_61dd7ba2 PS7, Line 147: { Actually, I think it would be better to properly mock this too (comparing buf and size to expected values). Otherwise you're not really testing much verification, you're just kinda pretending that you always get the right result.
https://review.coreboot.org/c/coreboot/+/56601/comment/cf7f0c44_54c1bdaa PS7, Line 216: &file_valid_hash.attrs_and_data[HASH_ATTR_SIZE], HASH_ATTR_SIZE); Uhhh... wait a second... does this test succeed? Because it shouldn't succeed. When CONFIG_CBFS_VERIFICATION=0, vb2_hash_verify() should never be called. Does cmocka not check at the end of every test whether there were pending expect()s that didn't get hit (I thought it did)?
https://review.coreboot.org/c/coreboot/+/56601/comment/d6060f4c_72fe1e35 PS7, Line 243: assert_null(mapping); And this should return a valid mapping because verification is disabled. I guess you just never ran this because of that typo in the Makefile?
https://review.coreboot.org/c/coreboot/+/56601/comment/a4e74919_27dace2e PS7, Line 283: /* No cache, so no validation required */ (nit: comment still makes no sense here)
https://review.coreboot.org/c/coreboot/+/56601/comment/9e238193_659e5500 PS7, Line 288: has_mcache Well, you can either define NO_CBFS_MCACHE, or you can test mcache stuff here. Not both.
-
Julius Werner (Code Review)