[PATCH] bgenv_get: Catch overflow on misuse of ebg_env_get_ex API

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

Our API can be confusing, and it contains nasty traps. One is snapping
if ebg_env_get_ex is used for retrieving values of pre-defined
variables, something it was not foreseen for, while providing a maxlen
value which is be too small for that, and then expecting no harm is
caused. No one should do that, and no sane user we are aware of ever
did, but we are better of detecting this corner case and rejecting it.

As we are now changing the internal API, we also need to adjust the test
case for it which was so far incorrectly assuming we were respecting
maxlen.

Reported-by: Liyander Rishwanth <missionu...@gmail.com>
Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
env/env_api.c | 2 +-
env/env_api_fat.c | 8 ++++++++
tools/tests/test_ebgenv_api_internal.c | 6 +++---
3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index ce6abbd..d9b9816 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -143,7 +143,7 @@ int ebg_env_open_current(ebgenv_t *e)
int ebg_env_get(ebgenv_t *e, const char *key, char *buffer)
{
return bgenv_get((BGENV *)e->bgenv, key, NULL, buffer,
- ENV_STRING_LENGTH);
+ ENV_STRING_LENGTH+1);
}

int ebg_env_get_ex(ebgenv_t *e, const char *key, uint64_t *usertype,
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 67898c8..053ebd2 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -342,6 +342,14 @@ int bgenv_get(BGENV *env, const char *key, uint64_t *type, void *data,
return bgenv_get_uservar(env->data->userdata, key, type, data,
maxlen);
}
+ /*
+ * Callers are not supposed to use bgenv_get via ebg_env_get_ex
+ * on pre-defined variables. If they do nevertheless, demand
+ * at least enough space as we are not checking it internally.
+ */
+ if (maxlen < ENV_STRING_LENGTH+1) {
+ return -EINVAL;
+ }
switch (e) {
case EBGENV_KERNELFILE:
return bgenv_get_string(type, data, env->data->kernelfile);
diff --git a/tools/tests/test_ebgenv_api_internal.c b/tools/tests/test_ebgenv_api_internal.c
index 9c174f6..8df16db 100644
--- a/tools/tests/test_ebgenv_api_internal.c
+++ b/tools/tests/test_ebgenv_api_internal.c
@@ -285,7 +285,7 @@ START_TEST(ebgenv_api_internal_bgenv_get)
handle->data->ustate = USTATE_INSTALLED;

char *data = NULL;
- char buffera[22];
+ char buffera[ENV_STRING_LENGTH+1];
int res;

/* Test if bgenv_get fails if maxlen is set to 0
@@ -311,14 +311,14 @@ START_TEST(ebgenv_api_internal_bgenv_get)

/* Test if bgenv_get returns the correct value
*/
- res = bgenv_get(handle, "kernelfile", NULL, buffera, res);
+ res = bgenv_get(handle, "kernelfile", NULL, buffera, sizeof(buffera));
ck_assert_int_eq(res, 0);
ck_assert_int_eq(strcmp(buffera, test_strings[0]), 0);

res = bgenv_get(handle, "kernelparams", NULL, NULL, 1000);
ck_assert_int_eq(res, strlen(test_strings[1]) + 1);

- res = bgenv_get(handle, "kernelparams", NULL, buffera, res);
+ res = bgenv_get(handle, "kernelparams", NULL, buffera, sizeof(buffera));
ck_assert_int_eq(res, 0);
ck_assert_int_eq(strcmp(buffera, test_strings[1]), 0);

--
2.47.3