[PATCH] probe_config_partitions: fix out-of-bounds access

[Michael Adler]

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 <michae...@siemens.com>
---
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;
}
--
2.33.1

[Jan Kiszka]

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

[Michael Adler]

Hi Jan,

> But why not simply moving the limit check before using cfgpart[count]? Then the tmp variable would be unneeded.

that was my first attempt as well, but during testing I realized it breaks the counting mechanism. The key observation is:

- probe_config_file needs write-access to a valid CONFIG_PART struct
- counter is only increased if probe_config_file was successful

Thus, in order to detect N config partitions, probe_config_file needs to succeed N times and therefore requires
N CONFIG_PART structs. Since the code wants to detect the situation where we have ENV_NUM_CONFIG_PARTS + 1 partitions,
and cfgpart is an array with ENV_NUM_CONFIG_PARTS elements, this won't work. Hence the introduction of the tmp variable.

I agree that it's not that nice. I guess we could also rewrite the probe_config_file method signature so that it no
longer requires the struct, but only some individual fields (although this could end up in a similar situation).

--
Michael Adler

Siemens AG
T RDA IOT SES-DE
Otto-Hahn-Ring 6
81739 München, Deutschland

Siemens Aktiengesellschaft: Vorsitzender des Aufsichtsrats: Jim Hagemann Snabe; Vorstand: Roland Busch, Vorsitzender; Klaus Helmrich, Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese; Sitz der Gesellschaft: Berlin und München, Deutschland; Registergericht: Berlin-Charlottenburg, HRB 12300, München, HRB 6684; WEEE-Reg.-Nr. DE 23691322

[Jan Kiszka]

On 02.12.21 09:59, Michael Adler wrote:

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;

[Michael Adler]

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 <michae...@siemens.com>
---

env/env_config_partitions.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/env/env_config_partitions.c b/env/env_config_partitions.c
index db3e08f..8ddd0ac 100644
--- a/env/env_config_partitions.c
+++ b/env/env_config_partitions.c
@@ -53,17 +53,22 @@ 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",

+ if (count < ENV_NUM_CONFIG_PARTS) {
+ cfgpart[count] = tmp;
+ } else {
+ free(tmp.devpath);

+ VERBOSE(stderr,
+ "Error, there are "
+ "more than %d config "
+ "partitions.\n",

ENV_NUM_CONFIG_PARTS);
return false;
}
--
2.33.1

[Michael Adler]

> 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.
>

> I think this check and return should go into the loop above.

Agreed, this optimization makes sense, see patch v2.

[Jan Kiszka]

Thanks, applied.

Jan