| Message ID | 20201013123632.12061-1-sami.hartikainen@teleste.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libuboot_env_store: fix env double-null termination | expand |
Hi Sami, sorry to take so long: On 13.10.20 14:36, Sami Hartikainen wrote: > Environment lines are terminated with a single null byte, followed > by another null byte to terminate the env itself. Fix termination > in case where flags need saving. I assume you have hit the bug, but I cannot discover it myself and I need your help. .flags are stored as a variable inside the environment (just with ".flags" as name), and I do not get why this additional null is required. Do you have a short test case that I can try myself and some more information about the issue ? Thanks. > > Signed-off-by: Sami Hartikainen <sami.hartikainen@teleste.com> > --- > src/uboot_env.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/uboot_env.c b/src/uboot_env.c > index c9a900f..756a5fb 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -898,6 +898,7 @@ int libuboot_env_store(struct uboot_ctx *ctx) > first = false; > } > } > + buf++; > } > *buf++ = '\0'; > > Best regards, Stefano Babic
> I assume you have hit the bug, but I cannot discover it myself and I need your > help. .flags are stored as a variable inside the environment (just with ".flags" > as name), and I do not get why this additional null is required. Do you have a > short test case that I can try myself and some more information about the > issue ? Thanks. Environment block must be terminated by two consecutive nulls, i.e. first to terminate the (last) variable line, and second to stop reading for additional lines by libuboot_load(). Please see function libuboot_env_store: within the first LIST_FOREACH (when storing "normal" variables), buf++ is called to move the pointer past the terminating null set by the snprintf(). The same pointer move must be done after storing the .flags, but this time *after* the LIST_FOREACH (because this for_each just builds .flags value as a comma -separated concatenation of values). Once done, the following *buf++ = '\0' sets the block -terminating (i.e. second) null properly. Eventually the bug will manifest by just doing repeated call to `fw_setenv <variable> <value>`, but it may take some time. To speed things up, test code below may be used to initialize the malloc()'ed env buffer: diff --git a/src/uboot_env.c b/src/uboot_env.c index c9a900f..eee124a 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -859,6 +859,16 @@ int libuboot_env_store(struct uboot_ctx *ctx) if (!image) return -ENOMEM; + /* + * Reveal env block termination bug + */ + const char *testpatt = "garbagevar=g\n ar\n bageval"; + const size_t testlen = strlen(testpatt); + const size_t testmax = sizeof(struct uboot_env_redund) + ctx->size - testlen - 1; + for (size = 0; size < testmax; size += (testlen + 1)) { + sprintf(image + size, testpatt); + } + if (ctx->redundant) offsetdata = offsetof(struct uboot_env_redund, data); else Now using commands: fw_setenv .flags <some_integer_var>:d fw_setenv testvar testval fw_printenv should show unwanted behavior: either some portion the <testpatt> appears in output, or env is deemed totally unreadable. Br, Sami PS. No proper check is done to see if there is space in the env block for the second null; it would be a topic for a separate patch.
Hi Sami, On 25.11.20 16:00, Hartikainen, Sami wrote: >> I assume you have hit the bug, but I cannot discover it myself and I need your >> help. .flags are stored as a variable inside the environment (just with ".flags" >> as name), and I do not get why this additional null is required. Do you have a >> short test case that I can try myself and some more information about the >> issue ? Thanks. > > Environment block must be terminated by two consecutive nulls, i.e. first to > terminate the (last) variable line, and second to stop reading for additional lines > by libuboot_load(). > > Please see function libuboot_env_store: within the first LIST_FOREACH (when storing > "normal" variables), buf++ is called to move the pointer past the terminating null set > by the snprintf(). The same pointer move must be done after storing the .flags, but this > time *after* the LIST_FOREACH (because this for_each just builds .flags value as a comma > -separated concatenation of values). Once done, the following *buf++ = '\0' sets the block > -terminating (i.e. second) null properly. Ouch, I understand now - I have not applied the patch, just reading it, and I was errouneously thinking it applies to the first FOREACH loop. Yes, without the increment, "*buf++ = '\0'" still apply to the string with the flag. Got it. IMHO your patch is ok, thanks for fixing it. I will apply it. Best regards, Stefano Babic > > Eventually the bug will manifest by just doing repeated call to `fw_setenv <variable> <value>`, > but it may take some time. To speed things up, test code below may be used to initialize the > malloc()'ed env buffer: > > > diff --git a/src/uboot_env.c b/src/uboot_env.c > index c9a900f..eee124a 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -859,6 +859,16 @@ int libuboot_env_store(struct uboot_ctx *ctx) > if (!image) > return -ENOMEM; > > + /* > + * Reveal env block termination bug > + */ > + const char *testpatt = "garbagevar=g\n ar\n bageval"; > + const size_t testlen = strlen(testpatt); > + const size_t testmax = sizeof(struct uboot_env_redund) + ctx->size - testlen - 1; > + for (size = 0; size < testmax; size += (testlen + 1)) { > + sprintf(image + size, testpatt); > + } > + > if (ctx->redundant) > offsetdata = offsetof(struct uboot_env_redund, data); > else > > > Now using commands: > fw_setenv .flags <some_integer_var>:d > fw_setenv testvar testval > fw_printenv > should show unwanted behavior: either some portion the <testpatt> appears > in output, or env is deemed totally unreadable. > > Br, Sami > > PS. No proper check is done to see if there is space in the env block for the second > null; it would be a topic for a separate patch. >
diff --git a/src/uboot_env.c b/src/uboot_env.c index c9a900f..756a5fb 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -898,6 +898,7 @@ int libuboot_env_store(struct uboot_ctx *ctx) first = false; } } + buf++; } *buf++ = '\0';
Environment lines are terminated with a single null byte, followed by another null byte to terminate the env itself. Fix termination in case where flags need saving. Signed-off-by: Sami Hartikainen <sami.hartikainen@teleste.com> --- src/uboot_env.c | 1 + 1 file changed, 1 insertion(+)