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)};
Ok, understood now why this is needed.
> + 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);
But if we end up here, why don't you bail out immediately and rather
increase count even further? You do no even print that 10 partitions
were found while only 3 are configured.
> }
> 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;
I think this check and return should go into the loop above.
Jan
> }
> return true;
> }
>
--
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/ba01f2c3-e74f-d928-c3e4-a3a704279f28%40siemens.com.
