Attention is currently required from: Ricardo Quesada. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56406 )
Change subject: util/elogtool: add tool to print elog events ......................................................................
Patch Set 9:
(6 comments)
Patchset:
PS9: Apologies for being too late on the review, but please consider these for a future fixup CL.
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/56406/comment/697aff4e_32ab3087 PS9, Line 160: ELOGTOOL_EXIT_SUCCESS Doesn't reaching this point mean BAD_ARGS (i.e. the command was not recognized)?
File util/cbfstool/eventlog.c:
https://review.coreboot.org/c/coreboot/+/56406/comment/5a4673e7_3254d539 PS9, Line 80: strftime(tm_string, sizeof(tm_string), "%Y-%m-%d %H:%M:%S", localtime(&time)); nit: I know it didn't use to do this, but should we maybe consider adding the time zone (%z) here? We have some logs in UTC and some in local, it's always super confusing to figure out which is which when it doesn't say so explicitly.
https://review.coreboot.org/c/coreboot/+/56406/comment/61e79a7f_21630a00 PS9, Line 355: {VB2_RECOVERY_LEGACY, "Legacy Utility"}, Now that this is linking vboot it should just call vb2_get_recovery_reason_string() rather than maintaining a separate list.
https://review.coreboot.org/c/coreboot/+/56406/comment/b3273c94_5ca06bdd PS9, Line 550: eventlog_printf("%s", val2str(*code, coreboot_post_codes)); There needs to be more range checking in various parts of this, you're just printing a potentially unbounded string from an untrusted source here. This should be written to always keep track of how much eventlog data was loaded in total (e.g. by using the cbfstool buffer_xxx() APIs) and then for each parsing operation make sure that it cannot run over the end of that without blindly trusting the data.
https://review.coreboot.org/c/coreboot/+/56406/comment/0bc24f7c_884ff5e1 PS9, Line 626: eventlog_printf_ignore_separator_once = 1; nit: Maybe it's just me but this printf API that passes state through a global seems pretty horrible. Can't we just insert the printf(" | ") manually where it's needed?
-
Julius Werner (Code Review)