| Message ID | 20221022094914.117705-2-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] console: file should always be non-negative | expand |
Hi Heinrich, On Sat, 22 Oct 2022 at 03:49, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > We use the parameter file in console function to choose from an array after > checking against MAX_FILES but we never check if the value of file is > negative. > > Running ./u-boot -T -l and issuing the poweroff command has resulted in > crashes because > > os_exit() results in std::ostream::flush() calling U-Boot's fflush with > file being a pointer which when converted to int may be represented by a > negative number. > > This shows that checking against MAX_FILES is not enough we have to ensure > that the file argument is always positive. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > common/console.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) How about changing the 'file' parameter to a uint? It seems that this is what it is supposed to be, from your checks. Regards, Simon
On 10/30/22 02:43, Simon Glass wrote: > Hi Heinrich, > > On Sat, 22 Oct 2022 at 03:49, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> We use the parameter file in console function to choose from an array after >> checking against MAX_FILES but we never check if the value of file is >> negative. >> >> Running ./u-boot -T -l and issuing the poweroff command has resulted in >> crashes because >> >> os_exit() results in std::ostream::flush() calling U-Boot's fflush with >> file being a pointer which when converted to int may be represented by a >> negative number. >> >> This shows that checking against MAX_FILES is not enough we have to ensure >> that the file argument is always positive. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> common/console.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) > > How about changing the 'file' parameter to a uint? It seems that this > is what it is supposed to be, from your checks. As we support only 3 values the variable type should have been defined as an enum. Changing the parameter type would have meant to look at all callers and check if the value of file is stored in a variable and replace that variable type too. I just went for the least invasive change. Best regards Heinrich
Hi Heinrich, On Sat, 29 Oct 2022 at 20:47, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 10/30/22 02:43, Simon Glass wrote: > > Hi Heinrich, > > > > On Sat, 22 Oct 2022 at 03:49, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> We use the parameter file in console function to choose from an array after > >> checking against MAX_FILES but we never check if the value of file is > >> negative. > >> > >> Running ./u-boot -T -l and issuing the poweroff command has resulted in > >> crashes because > >> > >> os_exit() results in std::ostream::flush() calling U-Boot's fflush with > >> file being a pointer which when converted to int may be represented by a > >> negative number. > >> > >> This shows that checking against MAX_FILES is not enough we have to ensure > >> that the file argument is always positive. > >> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> --- > >> common/console.c | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > > > > How about changing the 'file' parameter to a uint? It seems that this > > is what it is supposed to be, from your checks. > > As we support only 3 values the variable type should have been defined > as an enum. > > Changing the parameter type would have meant to look at all callers and > check if the value of file is stored in a variable and replace that > variable type too. > > I just went for the least invasive change. Well I don't really mind. Reviewed-by: Simon Glass <sjg@chromium.org>
diff --git a/common/console.c b/common/console.c index 0c9bf66c3f..10ab361d00 100644 --- a/common/console.c +++ b/common/console.c @@ -497,7 +497,7 @@ int serial_printf(const char *fmt, ...) int fgetc(int file) { - if (file < MAX_FILES) { + if ((unsigned int)file < MAX_FILES) { /* * Effectively poll for input wherever it may be available. */ @@ -530,7 +530,7 @@ int fgetc(int file) int ftstc(int file) { - if (file < MAX_FILES) + if ((unsigned int)file < MAX_FILES) return console_tstc(file); return -1; @@ -538,20 +538,20 @@ int ftstc(int file) void fputc(int file, const char c) { - if (file < MAX_FILES) + if ((unsigned int)file < MAX_FILES) console_putc(file, c); } void fputs(int file, const char *s) { - if (file < MAX_FILES) + if ((unsigned int)file < MAX_FILES) console_puts(file, s); } #ifdef CONFIG_CONSOLE_FLUSH_SUPPORT void fflush(int file) { - if (file < MAX_FILES) + if ((unsigned int)file < MAX_FILES) console_flush(file); } #endif
We use the parameter file in console function to choose from an array after checking against MAX_FILES but we never check if the value of file is negative. Running ./u-boot -T -l and issuing the poweroff command has resulted in crashes because os_exit() results in std::ostream::flush() calling U-Boot's fflush with file being a pointer which when converted to int may be represented by a negative number. This shows that checking against MAX_FILES is not enough we have to ensure that the file argument is always positive. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- common/console.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)