diff options
author | Julius Werner <jwerner@chromium.org> | 2020-01-23 14:52:59 -0800 |
---|---|---|
committer | Commit Bot <commit-bot@chromium.org> | 2020-01-28 02:32:08 +0000 |
commit | 0e97e25e85f0499e23b09a31a2c7116759f191d5 (patch) | |
tree | d990e6cc56eeab048a96de48cefdacc10e0c13b3 | |
parent | f57ad98c29072624bf0977ab972201595efd2b38 (diff) | |
download | vboot-0e97e25e85f0499e23b09a31a2c7116759f191d5.tar.gz |
2lib: Fix struct vb2_hash the way it was meant to bestabilize-quickfix-12871.27.Bstabilize-12871.91.Bstabilize-12871.65.Bstabilize-12871.57.Bstabilize-12871.253.Bstabilize-12871.24.Bstabilize-12871.103.Bstabilize-12871.102.Brelease-R81-12871.B
My goal in CL:1963614 was to write struct vb2_hash such that it can
match the exisiting binary representation of the CBFS hash attribute,
but no longer be dependent on endianness. Unfortunately I screwed up...
if you want to match the binary representation of a big-endian integer
for small numbers, the important byte you're interested in is the *last*
one, not the first.
Thankfully we still have time to fix the issue before this struct is
really used anywhere, so this patch does that and adds a test to double
check I got it right this time.
Also clarify comments about how vboot is allowed to use this struct a
bit to match the indended usage I'm planning in coreboot. In doing that
I realized that you actually don't want to make it easy to sizeof() the
|bytes| portion of the struct (because functions shouldn't rely on that
size anyway, they should only touch what's valid for a given hash
algorithm), so taking that out which also makes it a little more
comfortable to work with the struct.
BRANCH=none
BUG=none
TEST=make runtests
Change-Id: I7e1a19f36d75acb69e5d1bfa79700c9d878f9703
Signed-off-by: Julius Werner <jwerner@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2019952
-rw-r--r-- | firmware/2lib/2sha_utility.c | 2 | ||||
-rw-r--r-- | firmware/2lib/include/2sha.h | 22 | ||||
-rw-r--r-- | tests/vb2_sha_api_tests.c | 31 |
3 files changed, 38 insertions, 17 deletions
diff --git a/firmware/2lib/2sha_utility.c b/firmware/2lib/2sha_utility.c index 8c6f4b80..378f4c8a 100644 --- a/firmware/2lib/2sha_utility.c +++ b/firmware/2lib/2sha_utility.c @@ -221,7 +221,7 @@ vb2_error_t vb2_hash_verify(const void *buf, uint32_t size, hash_buf, hash_size); if (rv) return rv; - if (memcmp(hash_buf, hash->bytes.raw, hash_size)) + if (memcmp(hash_buf, hash->raw, hash_size)) return VB2_ERROR_SHA_MISMATCH; else return VB2_SUCCESS; diff --git a/firmware/2lib/include/2sha.h b/firmware/2lib/include/2sha.h index 32af9f74..95f85804 100644 --- a/firmware/2lib/include/2sha.h +++ b/firmware/2lib/include/2sha.h @@ -100,15 +100,17 @@ struct vb2_digest_context { /* * Serializable data structure that can store any vboot hash. Layout used in * CBFS attributes that need to be backwards-compatible -- do not change! - * When serializing/deserizaling this, you should store/load (offsetof(bytes) + - * vb2_digest_size(algo)), not the full size of this structure. + * When serializing/deserizaling this, you should store/load (offsetof(raw) + + * vb2_digest_size(algo)), not the full size of this structure. vboot functions + * taking a pointer to this should only access the |raw| array up to + * vb2_digest_size(algo) and not assume that the whole structure is accessible. */ struct vb2_hash { - /* enum vb2_hash_algorithm. Fixed width for serialization. - Single byte to avoid endianness issues. */ - uint8_t algo; - /* Padding to align and to match existing CBFS attribute. */ + /* Padding to match existing 4-byte big-endian from CBFS. + Could be reused for other stuff later (e.g. flags or something). */ uint8_t reserved[3]; + /* enum vb2_hash_algorithm. Single byte to avoid endianness issues. */ + uint8_t algo; /* The actual digest. Can add new types here as required. */ union { uint8_t raw[0]; @@ -121,10 +123,10 @@ struct vb2_hash { #if VB2_SUPPORT_SHA512 uint8_t sha512[VB2_SHA512_DIGEST_SIZE]; #endif - } bytes; /* This has a name so that it's easy to sizeof(). */ + }; }; -_Static_assert(sizeof(((struct vb2_hash *)0)->bytes) <= VB2_MAX_DIGEST_SIZE, - "Must update VB2_MAX_DIGEST_SIZE for new digests!"); +_Static_assert(sizeof(struct vb2_hash) - offsetof(struct vb2_hash, raw) + <= VB2_MAX_DIGEST_SIZE, "Update VB2_MAX_DIGEST_SIZE for new digests!"); _Static_assert(VB2_HASH_ALG_COUNT <= UINT8_MAX, "vb2_hash.algo overflow!"); /** @@ -270,7 +272,7 @@ static inline vb2_error_t vb2_hash_calculate(const void *buf, uint32_t size, struct vb2_hash *hash) { hash->algo = algo; - return vb2_digest_buffer(buf, size, algo, hash->bytes.raw, + return vb2_digest_buffer(buf, size, algo, hash->raw, vb2_digest_size(algo)); } diff --git a/tests/vb2_sha_api_tests.c b/tests/vb2_sha_api_tests.c index 6a23dcee..cb58f7bc 100644 --- a/tests/vb2_sha_api_tests.c +++ b/tests/vb2_sha_api_tests.c @@ -54,13 +54,30 @@ vb2_error_t vb2_digest_finalize(struct vb2_digest_context *dc, uint8_t *digest, return mock_finalize_rv; } +static void vb2_hash_cbfs_compatibility_test(void) +{ + /* 'algo' used to be represented as a 4-byte big-endian in CBFS. Confirm + that the new representation is binary compatible for small values. */ + union { + struct vb2_hash hash; + struct { + uint32_t be32; + uint8_t bytes[0]; + }; + } test = {0}; + + test.be32 = htobe32(0xa5); + TEST_EQ(test.hash.algo, 0xa5, "vb2_hash algo compatible to CBFS attr"); + TEST_PTR_EQ(&test.hash.raw, &test.bytes, " digest offset matches"); +} + static void vb2_hash_calculate_tests(void) { reset_common_data(); TEST_SUCC(vb2_hash_calculate(&mock_buffer, sizeof(mock_buffer), VB2_HASH_SHA1, &mock_hash), "hash_calculate success"); - TEST_SUCC(memcmp(mock_hash.bytes.sha1, mock_sha1, sizeof(mock_sha1)), + TEST_SUCC(memcmp(mock_hash.sha1, mock_sha1, sizeof(mock_sha1)), " got the right hash"); TEST_EQ(mock_hash.algo, VB2_HASH_SHA1, " set algo correctly"); @@ -87,19 +104,19 @@ static void vb2_hash_verify_tests(void) { reset_common_data(); - memcpy(mock_hash.bytes.sha1, mock_sha1, sizeof(mock_sha1)); + memcpy(mock_hash.sha1, mock_sha1, sizeof(mock_sha1)); mock_hash.algo = VB2_HASH_SHA1; TEST_SUCC(vb2_hash_verify(mock_buffer, sizeof(mock_buffer), &mock_hash), "hash_verify success"); - memcpy(mock_hash.bytes.sha1, mock_sha1, sizeof(mock_sha1)); + memcpy(mock_hash.sha1, mock_sha1, sizeof(mock_sha1)); mock_hash.algo = VB2_HASH_SHA256; TEST_EQ(vb2_hash_verify(mock_buffer, sizeof(mock_buffer), &mock_hash), VB2_ERROR_MOCK, "hash_verify wrong algo"); - memcpy(mock_hash.bytes.sha1, mock_sha1, sizeof(mock_sha1)); - mock_hash.bytes.sha1[5] = 0xfe; + memcpy(mock_hash.sha1, mock_sha1, sizeof(mock_sha1)); + mock_hash.sha1[5] = 0xfe; mock_hash.algo = VB2_HASH_SHA1; TEST_EQ(vb2_hash_verify(mock_buffer, sizeof(mock_buffer), &mock_hash), VB2_ERROR_SHA_MISMATCH, @@ -108,9 +125,11 @@ static void vb2_hash_verify_tests(void) int main(int argc, char *argv[]) { - TEST_EQ(sizeof(mock_hash.bytes), VB2_SHA512_DIGEST_SIZE, + TEST_EQ(sizeof(mock_hash), + offsetof(struct vb2_hash, raw) + VB2_SHA512_DIGEST_SIZE, "tests run with all SHA algorithms enabled"); + vb2_hash_cbfs_compatibility_test(); vb2_hash_calculate_tests(); vb2_hash_verify_tests(); |