On 02.12.21 09:59, Michael Adler wrote: > This fixes a 4 year old (non-deterministic) bug in > probe_config_partitions. > > How to reproduce (assuming ENV_NUM_CONFIG_PARTS=2): > > 1) Compile EBG with support for 2 config partitions > 2) Run bg_printenv in an environment with 3 config partitions > 3) If no segfault, go back to 1). > > Note: There is *no guarantee* that you can trigger the bug by following > the above steps due to undefined behavior. > > Analysis: > > The argument `CONFIG_PART *cfgpart` is an array of size > `ENV_NUM_CONFIG_PARTS`. > If there are more than `ENV_NUM_CONFIG_PARTS` config partitions, the > element `cfgpart[ENV_NUM_CONFIG_PARTS]` is used in probe_config_file. > Although there is a NULL check in the latter function, the memory > content of `cfgpart[ENV_NUM_CONFIG_PARTS]` is undefined since it is > out-of-bounds. It may be NULL, in which case everything is fine - but it > may also be some other (garbage) value... This is dangerous territory. > > For reference, below is a backtrace based on 55a1e3e (note: this is > *not* the culprit commit): > > 0 0x00005626dd6d8aa7 in probe_config_file > (cfgpart=cfgpart@entry=0x5626dd720ff0) at env/env_config_file.c:69 > 1 0x00005626dd6d8c10 in probe_config_partitions > (cfgpart=cfgpart@entry=0x5626dd720fc0 <config_parts>) at > env/env_config_partitions.c:61 > 2 0x00005626dd6d7c5f in bgenv_init () at env/env_api_fat.c:141 > 3 0x00005626dd6d70d6 in bg_printenv (argc=<optimized out>, argv=<optimized > out>) at tools/bg_printenv.c:347 > > Signed-off-by: Michael Adler <[email protected]> > --- > env/env_config_partitions.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/env/env_config_partitions.c b/env/env_config_partitions.c > index db3e08f..a67c468 100644 > --- a/env/env_config_partitions.c > +++ b/env/env_config_partitions.c > @@ -53,19 +53,18 @@ bool probe_config_partitions(CONFIG_PART *cfgpart) > (void)snprintf(devpath, 4096, "%s%u", > dev->path, part->num); > } > - cfgpart[count].devpath = strdup(devpath); > - if (!cfgpart[count].devpath) { > + > + CONFIG_PART tmp = {.devpath = strdup(devpath)}; > + if (!tmp.devpath) { > VERBOSE(stderr, "Out of memory."); > return false; > } > - if (probe_config_file(&cfgpart[count])) { > + if (probe_config_file(&tmp)) { > printf_debug("%s", "Environment file found.\n"); > - if (count >= ENV_NUM_CONFIG_PARTS) { > - VERBOSE(stderr, "Error, there are " > - "more than %d config " > - "partitions.\n", > - ENV_NUM_CONFIG_PARTS); > - return false; > + if (count < ENV_NUM_CONFIG_PARTS) { > + cfgpart[count] = tmp; > + } else { > + free(tmp.devpath); > } > count++; > } > @@ -77,6 +76,13 @@ bool probe_config_partitions(CONFIG_PART *cfgpart) > "Error, less than %d config partitions exist.\n", > ENV_NUM_CONFIG_PARTS); > return false; > + } else if (count > ENV_NUM_CONFIG_PARTS) { > + VERBOSE(stderr, > + "Error, there are " > + "more than %d config " > + "partitions.\n", > + ENV_NUM_CONFIG_PARTS); > + return false; > } > return true; > } >
Good catch! But why not simply moving the limit check before using cfgpart[count]? Then the tmp variable would be unneeded. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux -- You received this message because you are subscribed to the Google Groups "EFI Boot Guard" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/efibootguard-dev/02dd46f6-c148-c033-0cb0-f4bcc4feaef3%40siemens.com.
