Change in coreboot[master]: cbfstool: Hide hash printing behind -v and add to parseable output
Hello Patrick Georgi, Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41119
to review the following change.
Change subject: cbfstool: Hide hash printing behind -v and add to parseable output ......................................................................
cbfstool: Hide hash printing behind -v and add to parseable output
With the upcoming introduction of CBFS verification, a lot more CBFS files will have hashes. The current cbfstool default of always printing hash attributes when they exist will make cbfstool print very messy. Therefore, hide hash attribute output unless the user passed -v.
It would also be useful to be able to get file attributes like hashes in machine parseable output. Unfortunately, our machine parseable format (-k) doesn't really seem designed to be extensible. To avoid breaking older parsers, this patch adds new attribute output behind -v (which hopefully no current users pass since it doesn't change anything for -k at the moment). With this patch cbfstool print -k -v may print an arbitrary amount of extra tokens behind the predefined ones on a file line. Tokens always begin with an identifying string (e.g. 'hash'), followed by extra fields that should be separated by colons. Multiple tokens are separated by the normal separator character (tab).
cbfstool print -k -v may also print additional information that applies to the whole CBFS on separate lines. These lines will always begin with a '[' (which hopefully nobody would use as a CBFS filename character although we technically have no restrictions at the moment).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I9e16cda393fa0bc1d8734d4b699e30e2ae99a36d --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfs_image.h M util/cbfstool/cbfstool.c 3 files changed, 41 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/41119/1
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 7942b27..3a83694 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -1503,6 +1503,9 @@ decompressed_size );
+ if (!verbose) + return 0; + struct cbfs_file_attr_hash *attr = NULL; while ((attr = cbfs_file_get_next_hash(entry, attr)) != NULL) { size_t hash_len = vb2_digest_size(attr->hash.algo); @@ -1522,9 +1525,6 @@ free(hash_str); }
- if (!verbose) - return 0; - DEBUG(" cbfs_file=0x%x, offset=0x%x, content_address=0x%x+0x%x\n", cbfs_get_entry_addr(image, entry), ntohl(entry->offset), cbfs_get_entry_addr(image, entry) + ntohl(entry->offset), @@ -1587,21 +1587,45 @@ fprintf(fp, "%s%s", type, sep); fprintf(fp, "0x%zx%s", metadata_size, sep); fprintf(fp, "0x%zx%s", data_size, sep); - fprintf(fp, "0x%zx\n", metadata_size + data_size); + fprintf(fp, "0x%zx", metadata_size + data_size); + + if (verbose) { + unsigned int decompressed_size = 0; + unsigned int compression = cbfs_file_get_compression_info(entry, + &decompressed_size); + if (compression != CBFS_COMPRESS_NONE) + fprintf(fp, "%scomp:%s:0x%x", sep, lookup_name_by_type( + types_cbfs_compression, compression, "????"), + decompressed_size); + + struct cbfs_file_attr_hash *attr = NULL; + while ((attr = cbfs_file_get_next_hash(entry, attr)) != NULL) { + size_t hash_len = vb2_digest_size(attr->hash.algo); + if (!hash_len) + continue; + char *hash_str = bintohex(attr->hash.raw, hash_len); + int valid = vb2_hash_verify(CBFS_SUBHEADER(entry), + ntohl(entry->len), &attr->hash) == VB2_SUCCESS; + fprintf(fp, "%shash:%s:%s:%s", sep, + vb2_get_hash_algorithm_name(attr->hash.algo), + hash_str, valid ? "valid" : "invalid"); + free(hash_str); + } + } + fprintf(fp, "\n");
return 0; }
-int cbfs_print_directory(struct cbfs_image *image) +void cbfs_print_directory(struct cbfs_image *image) { if (cbfs_is_legacy_cbfs(image)) cbfs_print_header_info(image); printf("%-30s %-10s %-12s Size Comp\n", "Name", "Offset", "Type"); cbfs_legacy_walk(image, cbfs_print_entry_info, NULL); - return 0; }
-int cbfs_print_parseable_directory(struct cbfs_image *image) +void cbfs_print_parseable_directory(struct cbfs_image *image) { size_t i; const char *header[] = { @@ -1618,7 +1642,6 @@ fprintf(stdout, "%s%s", header[i], sep); fprintf(stdout, "%s\n", header[i]); cbfs_legacy_walk(image, cbfs_print_parseable_entry_info, stdout); - return 0; }
int cbfs_merge_empty_entry(struct cbfs_image *image, struct cbfs_file *entry, diff --git a/util/cbfstool/cbfs_image.h b/util/cbfstool/cbfs_image.h index a3a6fb9..d633fee 100644 --- a/util/cbfstool/cbfs_image.h +++ b/util/cbfstool/cbfs_image.h @@ -174,8 +174,8 @@ int cbfs_is_valid_entry(struct cbfs_image *image, struct cbfs_file *entry);
/* Print CBFS component information. */ -int cbfs_print_directory(struct cbfs_image *image); -int cbfs_print_parseable_directory(struct cbfs_image *image); +void cbfs_print_directory(struct cbfs_image *image); +void cbfs_print_parseable_directory(struct cbfs_image *image); int cbfs_print_header_info(struct cbfs_image *image); int cbfs_print_entry_info(struct cbfs_image *image, struct cbfs_file *entry, void *arg); diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index f97745e..6015603 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -1087,12 +1087,16 @@ if (cbfs_image_from_buffer(&image, param.image_region, param.headeroffset)) return 1; - if (param.machine_parseable) - return cbfs_print_parseable_directory(&image); - else { + if (param.machine_parseable) { + if (verbose) + printf("[FMAP REGION]\t%s\n", param.region_name); + cbfs_print_parseable_directory(&image); + } else { printf("FMAP REGION: %s\n", param.region_name); - return cbfs_print_directory(&image); + cbfs_print_directory(&image); } + + return 0; }
static int cbfs_extract(void)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41119 )
Change subject: cbfstool: Hide hash printing behind -v and add to parseable output ......................................................................
Patch Set 5: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41119 )
Change subject: cbfstool: Hide hash printing behind -v and add to parseable output ......................................................................
cbfstool: Hide hash printing behind -v and add to parseable output
With the upcoming introduction of CBFS verification, a lot more CBFS files will have hashes. The current cbfstool default of always printing hash attributes when they exist will make cbfstool print very messy. Therefore, hide hash attribute output unless the user passed -v.
It would also be useful to be able to get file attributes like hashes in machine parseable output. Unfortunately, our machine parseable format (-k) doesn't really seem designed to be extensible. To avoid breaking older parsers, this patch adds new attribute output behind -v (which hopefully no current users pass since it doesn't change anything for -k at the moment). With this patch cbfstool print -k -v may print an arbitrary amount of extra tokens behind the predefined ones on a file line. Tokens always begin with an identifying string (e.g. 'hash'), followed by extra fields that should be separated by colons. Multiple tokens are separated by the normal separator character (tab).
cbfstool print -k -v may also print additional information that applies to the whole CBFS on separate lines. These lines will always begin with a '[' (which hopefully nobody would use as a CBFS filename character although we technically have no restrictions at the moment).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I9e16cda393fa0bc1d8734d4b699e30e2ae99a36d Reviewed-on: https://review.coreboot.org/c/coreboot/+/41119 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M util/cbfstool/cbfs_image.c M util/cbfstool/cbfs_image.h M util/cbfstool/cbfstool.c 3 files changed, 41 insertions(+), 14 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/util/cbfstool/cbfs_image.c b/util/cbfstool/cbfs_image.c index 5d95814..05fdc8a 100644 --- a/util/cbfstool/cbfs_image.c +++ b/util/cbfstool/cbfs_image.c @@ -1488,6 +1488,9 @@ decompressed_size );
+ if (!verbose) + return 0; + struct cbfs_file_attr_hash *attr = NULL; while ((attr = cbfs_file_get_next_hash(entry, attr)) != NULL) { size_t hash_len = vb2_digest_size(attr->hash.algo); @@ -1507,9 +1510,6 @@ free(hash_str); }
- if (!verbose) - return 0; - DEBUG(" cbfs_file=0x%x, offset=0x%x, content_address=0x%x+0x%x\n", cbfs_get_entry_addr(image, entry), ntohl(entry->offset), cbfs_get_entry_addr(image, entry) + ntohl(entry->offset), @@ -1572,21 +1572,45 @@ fprintf(fp, "%s%s", type, sep); fprintf(fp, "0x%zx%s", metadata_size, sep); fprintf(fp, "0x%zx%s", data_size, sep); - fprintf(fp, "0x%zx\n", metadata_size + data_size); + fprintf(fp, "0x%zx", metadata_size + data_size); + + if (verbose) { + unsigned int decompressed_size = 0; + unsigned int compression = cbfs_file_get_compression_info(entry, + &decompressed_size); + if (compression != CBFS_COMPRESS_NONE) + fprintf(fp, "%scomp:%s:0x%x", sep, lookup_name_by_type( + types_cbfs_compression, compression, "????"), + decompressed_size); + + struct cbfs_file_attr_hash *attr = NULL; + while ((attr = cbfs_file_get_next_hash(entry, attr)) != NULL) { + size_t hash_len = vb2_digest_size(attr->hash.algo); + if (!hash_len) + continue; + char *hash_str = bintohex(attr->hash.raw, hash_len); + int valid = vb2_hash_verify(CBFS_SUBHEADER(entry), + ntohl(entry->len), &attr->hash) == VB2_SUCCESS; + fprintf(fp, "%shash:%s:%s:%s", sep, + vb2_get_hash_algorithm_name(attr->hash.algo), + hash_str, valid ? "valid" : "invalid"); + free(hash_str); + } + } + fprintf(fp, "\n");
return 0; }
-int cbfs_print_directory(struct cbfs_image *image) +void cbfs_print_directory(struct cbfs_image *image) { if (cbfs_is_legacy_cbfs(image)) cbfs_print_header_info(image); printf("%-30s %-10s %-12s Size Comp\n", "Name", "Offset", "Type"); cbfs_legacy_walk(image, cbfs_print_entry_info, NULL); - return 0; }
-int cbfs_print_parseable_directory(struct cbfs_image *image) +void cbfs_print_parseable_directory(struct cbfs_image *image) { size_t i; const char *header[] = { @@ -1603,7 +1627,6 @@ fprintf(stdout, "%s%s", header[i], sep); fprintf(stdout, "%s\n", header[i]); cbfs_legacy_walk(image, cbfs_print_parseable_entry_info, stdout); - return 0; }
int cbfs_merge_empty_entry(struct cbfs_image *image, struct cbfs_file *entry, diff --git a/util/cbfstool/cbfs_image.h b/util/cbfstool/cbfs_image.h index 74c35c9..664d146 100644 --- a/util/cbfstool/cbfs_image.h +++ b/util/cbfstool/cbfs_image.h @@ -162,8 +162,8 @@ int cbfs_is_valid_entry(struct cbfs_image *image, struct cbfs_file *entry);
/* Print CBFS component information. */ -int cbfs_print_directory(struct cbfs_image *image); -int cbfs_print_parseable_directory(struct cbfs_image *image); +void cbfs_print_directory(struct cbfs_image *image); +void cbfs_print_parseable_directory(struct cbfs_image *image); int cbfs_print_header_info(struct cbfs_image *image); int cbfs_print_entry_info(struct cbfs_image *image, struct cbfs_file *entry, void *arg); diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 2fd8eaa..6936d57 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -1084,12 +1084,16 @@ if (cbfs_image_from_buffer(&image, param.image_region, param.headeroffset)) return 1; - if (param.machine_parseable) - return cbfs_print_parseable_directory(&image); - else { + if (param.machine_parseable) { + if (verbose) + printf("[FMAP REGION]\t%s\n", param.region_name); + cbfs_print_parseable_directory(&image); + } else { printf("FMAP REGION: %s\n", param.region_name); - return cbfs_print_directory(&image); + cbfs_print_directory(&image); } + + return 0; }
static int cbfs_extract(void)
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)