Syslinux - Oct 2015 - [PATCH 0/2] Stack overflows when running commands

From: Sylvain Gault <sylvain.gault at gmail.com>

Hello there,

I propose 2 patches that fix two possible stack overflows either when running a
COM32 module or when loading a new config file.

I didn't find a better way to do this than to use the infamous
setjmp/longjmp
functions to restore the stack to a previous state. This makes the logic a bit
more complex, but the behavior is not changed.

Although these bugs are not very visible right now because of some useless
section in the binaries taking some space allowing the stack to overflow
without consequences, they might need to be fixed very soon.

Still remain a limitation when running a lot of commands (or loading lots of
time some config file) is that some files might not be closed and some memory
may not be freed.

Sylvain Gault (2):
  ldlinux: fix stack overflow when running COM32 modules
  core: Fix stack overflow when reloading config

 com32/elflink/ldlinux/execute.c |  4 ++-
 com32/elflink/ldlinux/ldlinux.c | 28 ++++++++++++++------
 core/elflink/load_env32.c       | 58 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 10 deletions(-)

-- 
2.6.1

From: Sylvain Gault <sylvain.gault at gmail.com>

When a COM32 module exits, the functions never return and a new call to
ldlinux_enter_command is made. This could fill the stack and overflow on
some data present in memory.

This patch use setjmp/longjmp to return to the main function and restart
from there when a COM32 module exits.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 com32/elflink/ldlinux/execute.c |  4 +++-
 com32/elflink/ldlinux/ldlinux.c | 28 ++++++++++++++++++++--------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/com32/elflink/ldlinux/execute.c b/com32/elflink/ldlinux/execute.c
index 653c880..3955571 100644
--- a/com32/elflink/ldlinux/execute.c
+++ b/com32/elflink/ldlinux/execute.c
@@ -44,6 +44,7 @@ const struct image_types image_boot_types[] = {
     { NULL, 0 },
 };
 
+extern jmp_buf __return_to_command_prompt;
 extern int create_args_and_load(char *);
 
 __export void execute(const char *cmdline, uint32_t type, bool sysappend)
@@ -136,7 +137,8 @@ __export void execute(const char *cmdline, uint32_t type,
bool sysappend)
 		/* Restore the console */
 		ldlinux_console_init();
 
-		ldlinux_enter_command();
+		/* Jump back to the main to call ldlinux_enter_command */
+		longjmp(__return_to_command_prompt, 1);
 	} else if (type == IMAGE_TYPE_CONFIG) {
 		char *argv[] = { LDLINUX, NULL, NULL };
 		char *config;
diff --git a/com32/elflink/ldlinux/ldlinux.c b/com32/elflink/ldlinux/ldlinux.c
index 9b01dd3..0172117 100644
--- a/com32/elflink/ldlinux/ldlinux.c
+++ b/com32/elflink/ldlinux/ldlinux.c
@@ -31,6 +31,8 @@ static const struct file_ext file_extensions[] = {
 	{ NULL, 0 },
 };
 
+jmp_buf __return_to_command_prompt;
+
 /*
  * Return a pointer to one byte after the last character of the
  * command.
@@ -302,6 +304,7 @@ __export int main(int argc __unused, char **argv)
 	const void *adv;
 	const char *cmdline;
 	size_t count = 0;
+	int retval;
 
 	ldlinux_console_init();
 
@@ -333,16 +336,25 @@ __export int main(int argc __unused, char **argv)
 		if (!syslinux_setadv(ADV_BOOTONCE, 0, NULL))
 			syslinux_adv_write();
 
-		load_kernel(cmdline); /* Shouldn't return */
-		ldlinux_enter_command();
-	}
-
-	if (!forceprompt && !shift_is_held())
-		ldlinux_auto_boot();
+		/*
+		 * The corresponding longjmp is located in the execute function
+		 * after a COM32 module has returned.
+		 */
+		retval = setjmp(__return_to_command_prompt);
+		if (retval == 0)
+			load_kernel(cmdline); /* Shouldn't return */
+	} else {
+		retval = setjmp(__return_to_command_prompt);
+		if (retval == 0) {
+			if (!forceprompt && !shift_is_held())
+				ldlinux_auto_boot();
 
-	if (defaultlevel > 1)
-		ldlinux_auto_boot();
+			if (defaultlevel > 1)
+				ldlinux_auto_boot();
+		}
+	}
 
+	retval = setjmp(__return_to_command_prompt);
 	ldlinux_enter_command();
 	return 0;
 }
-- 
2.6.1

From: Sylvain Gault <sylvain.gault at gmail.com>

The behavior when running a "CONFIG" command line is to reload
ldlinux.c32 with the new file as argument. This call never return.

In order to avoid stacking up the calls to start_ldlinux, this patch
introduce a setjmp/longjmp to return to the first call to start_ldlinux,
thus freeing all the stack space.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 core/elflink/load_env32.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/core/elflink/load_env32.c b/core/elflink/load_env32.c
index 492cc09..db19c7a 100644
--- a/core/elflink/load_env32.c
+++ b/core/elflink/load_env32.c
@@ -55,7 +55,7 @@ void init_module_subsystem(struct elf_module *module)
     list_add(&module->list, &modules_head);
 }
 
-__export int start_ldlinux(int argc, char **argv)
+static int _start_ldlinux(int argc, char **argv)
 {
 	int rv;
 
@@ -96,6 +96,62 @@ again:
 	return rv;
 }
 
+__export int start_ldlinux(int argc, char **argv)
+{
+	/* These variables are static to survive the longjmp. */
+	static int has_jmpbuf = 0;
+	static jmp_buf restart;
+	static int savedargc;
+	static char *heapargs;
+	static size_t argsmem = 0;
+	char **stackargv;
+	char *stackargs;
+	char *p, *q;
+	int i;
+
+
+	/* 1. Save the arguments on the heap */
+	for (i = 0; i < argc; i++)
+		argsmem += strlen(argv[i]) + 1;
+
+	savedargc = argc;
+	heapargs = malloc(argsmem);
+
+	p = heapargs;
+	for (i = 0; i < savedargc; i++) {
+		q = argv[i];
+		while (*q)
+			*p++ = *q++;
+
+		*p++ = '\0';
+	}
+
+	/* 2. Undo the stack if we're restarting ldlinux */
+	if (has_jmpbuf)
+		longjmp(restart, 1);
+
+	setjmp(restart);
+	has_jmpbuf = 1;
+
+	/* 3. Convert the heap memory to stack memory to avoid memory leaks */
+	stackargs = alloca(argsmem);
+	stackargv = alloca(savedargc * (sizeof(char *) + 1));
+
+	memcpy(stackargs, heapargs, argsmem);
+
+	p = stackargs;
+	for (i = 0; i < savedargc; i++) {
+		stackargv[i] = p;
+		p += strlen(p) + 1;
+	}
+
+	stackargv[savedargc] = NULL;
+
+	free(heapargs);
+
+	return _start_ldlinux(savedargc, stackargv);
+}
+
 /* note to self: do _*NOT*_ use static key word on this function */
 void load_env32(com32sys_t * regs __unused)
 {
-- 
2.6.1

On Tue, Oct 13, 2015 at 12:04 AM, celelibi--- via Syslinux
<syslinux at zytor.com> wrote:
> From: Sylvain Gault <sylvain.gault at gmail.com>
>
> Hello there,
>
> I propose 2 patches that fix two possible stack overflows either when
running a
> COM32 module or when loading a new config file.
>
> I didn't find a better way to do this than to use the infamous
setjmp/longjmp
> functions to restore the stack to a previous state. This makes the logic a
bit
> more complex, but the behavior is not changed.
The other way to do this is set variable(s) about the next target
state and then return from the functions.  I'm not sure which is the
better way, however.
> Although these bugs are not very visible right now because of some useless
> section in the binaries taking some space allowing the stack to overflow
> without consequences, they might need to be fixed very soon.
>
> Still remain a limitation when running a lot of commands (or loading lots
of
> time some config file) is that some files might not be closed and some
memory
> may not be freed.
>
> Sylvain Gault (2):
>   ldlinux: fix stack overflow when running COM32 modules
>   core: Fix stack overflow when reloading config
>
>  com32/elflink/ldlinux/execute.c |  4 ++-
>  com32/elflink/ldlinux/ldlinux.c | 28 ++++++++++++++------
>  core/elflink/load_env32.c       | 58
++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 80 insertions(+), 10 deletions(-)

-- 
-Gene

2016-01-09 17:05 UTC+01:00, Gene Cumm <gene.cumm at
gmail.com>:
> On Tue, Oct 13, 2015 at 12:04 AM, celelibi--- via Syslinux
> <syslinux at zytor.com> wrote:
>> From: Sylvain Gault <sylvain.gault at gmail.com>
>>
>> Hello there,
>>
>> I propose 2 patches that fix two possible stack overflows either when
>> running a
>> COM32 module or when loading a new config file.
>>
>> I didn't find a better way to do this than to use the infamous
>> setjmp/longjmp
>> functions to restore the stack to a previous state. This makes the
logic a
>> bit
>> more complex, but the behavior is not changed.
>
> The other way to do this is set variable(s) about the next target
> state and then return from the functions.  I'm not sure which is the
> better way, however.
That would change the semantics of the execute() function which is
supposed to never return. So all the modules using that function
directly or indirectly would need to be patched.

Are there some people who wrote their own modules? If so, Ady would
yell at us as a backward compatibility guardian. ^^

But I agree that would allow the modules to clean up after themselves:
Freeing memory and closing files. But maybe a generic resource
tracking is better anyway.
>
>> Although these bugs are not very visible right now because of some
>> useless
>> section in the binaries taking some space allowing the stack to
overflow
>> without consequences, they might need to be fixed very soon.
>>
>> Still remain a limitation when running a lot of commands (or loading
lots
>> of
>> time some config file) is that some files might not be closed and some
>> memory
>> may not be freed.
>>
>> Sylvain Gault (2):
>>   ldlinux: fix stack overflow when running COM32 modules
>>   core: Fix stack overflow when reloading config
>>
>>  com32/elflink/ldlinux/execute.c |  4 ++-
>>  com32/elflink/ldlinux/ldlinux.c | 28 ++++++++++++++------
>>  core/elflink/load_env32.c       | 58
>> ++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 80 insertions(+), 10 deletions(-)
>
>
> --
> -Gene
>

Celelibi

On 10/12/15 21:04, celelibi--- via Syslinux wrote:
> From: Sylvain Gault <sylvain.gault at gmail.com>
> 
> The behavior when running a "CONFIG" command line is to reload
> ldlinux.c32 with the new file as argument. This call never return.
> 
> In order to avoid stacking up the calls to start_ldlinux, this patch
> introduce a setjmp/longjmp to return to the first call to start_ldlinux,
> thus freeing all the stack space.
> 
> Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
We don't re*load* ldlinux.c32, I hope?  I assume we just re-execute it?

	-hpa