| Message ID | 20180914204749.11014-1-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [cbootimage] Fix various abort(), crashes, and memory errors | expand |
On Fri, Sep 14, 2018 at 02:47:49PM -0600, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > cbootimage doesn't have extensive error-checking of the input files. Thus > it's easy to trigger aborts (which in turn segfault to exit the app) and > bad memory accesses by providing under-sized binary input files or > configuration files with missing required statements. Add a bit more > error-checking to clean up some of these cases. No doubt there are more, > but this change only fixes those that have been reported. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > src/cbootimage.c | 15 ++++++++++++++- > src/data_layout.c | 11 +++++++++-- > src/parse.c | 2 +- > 3 files changed, 24 insertions(+), 4 deletions(-) Reviewed-by: Thierry Reding <treding@nvidia.com>
On 09/17/2018 03:59 AM, Thierry Reding wrote: > On Fri, Sep 14, 2018 at 02:47:49PM -0600, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> cbootimage doesn't have extensive error-checking of the input files. Thus >> it's easy to trigger aborts (which in turn segfault to exit the app) and >> bad memory accesses by providing under-sized binary input files or >> configuration files with missing required statements. Add a bit more >> error-checking to clean up some of these cases. No doubt there are more, >> but this change only fixes those that have been reported. >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> --- >> src/cbootimage.c | 15 ++++++++++++++- >> src/data_layout.c | 11 +++++++++-- >> src/parse.c | 2 +- >> 3 files changed, 24 insertions(+), 4 deletions(-) > > Reviewed-by: Thierry Reding <treding@nvidia.com> Applied.
diff --git a/src/cbootimage.c b/src/cbootimage.c index ca781fa6dab1..e728c8b06801 100644 --- a/src/cbootimage.c +++ b/src/cbootimage.c @@ -239,7 +239,7 @@ main(int argc, char *argv[]) /* Get BCT_SIZE from input image file */ bct_size = get_bct_size_from_image(&context); - if (bct_size < 0) { + if (bct_size <= 0) { printf("Error: Invalid input image file %s\n", context.input_image_filename); goto fail; @@ -301,6 +301,19 @@ main(int argc, char *argv[]) goto fail; } + if (!context.bct_init) { + e = 1; + printf("No BCT file has been read or generated.\n"); + printf("This is likely due to an incomplete config file.\n"); + goto fail; + } + if (!context.memory) { + e = 1; + printf("No output data generated.\n"); + printf("This is likely due to an incomplete config file.\n"); + goto fail; + } + /* Peform final signing & encryption of bct. */ e = sign_bct(&context, context.bct); if (e != 0) { diff --git a/src/data_layout.c b/src/data_layout.c index 2ed486bf12b5..8301aec59b03 100644 --- a/src/data_layout.c +++ b/src/data_layout.c @@ -218,8 +218,10 @@ write_page(build_image_context *context, return -ENOMEM; if (block->data == NULL) return -ENOMEM; - assert(((page_number + 1) * context->page_size) - <= context->block_size); + if (((page_number + 1) * context->page_size) > context->block_size) { + printf("Page number outside block; likely config file error.\n"); + return -ENOMEM; + } if (block->pages_used != page_number) { printf("Warning: Writing page in block out of order.\n"); @@ -838,6 +840,11 @@ begin_update(build_image_context *context) assert(context); + if (context->page_size_log2 < NVBOOT_AES_BLOCK_SIZE_LOG2) { + printf("Page size is too small; likely config file error\n"); + return 1; + } + /* Ensure that the BCT block & page data is current. */ if (enable_debug) { uint32_t block_size_log2; diff --git a/src/parse.c b/src/parse.c index 99cb428b26fd..10060936c529 100644 --- a/src/parse.c +++ b/src/parse.c @@ -249,7 +249,7 @@ parse_filename(char *str, char *name, int chars_remaining) * Check if the filename buffer is out of space, preserving one * character to null terminate the string. */ - while (isalnum(*str) || strchr("\\/~_-+:.@", *str)) { + while (*str && (isalnum(*str) || strchr("\\/~_-+:.@", *str))) { chars_remaining--;