| Message ID | 20220727141747.4793-1-ajones@ventanamicro.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] lib: sbi: Fix sbi_snprintf | expand |
On Wed, Jul 27, 2022 at 7:47 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > printc would happily write to 'out' even when 'out_len' was zero, > potentially overflowing buffers. Rework printc to not do that and > also ensure the null byte is written at the last position when > necessary, as stated in the snprintf man page. Also, panic if > sprintf or snprintf are called with NULL output strings (except > the special case of snprintf having a NULL output string and > a zero output size, allowing it to be used to get the number of > characters that would have been written). Finally, rename a > goto label which clashed with 'out'. > > Fixes: 9e8ff05cb61f ("Initial commit.") > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > --- > > v3: > - Add comment in printc stating our expectations of *out > > v2: > - Simply forbid *out == NULL by panicing when it's detected. > (The error message for snprintf has been split over two > lines to avoid going over 80 chars. I'd prefer error messages > not be split, but that seems like the general practice for > opensbi.) > - Drop some branches, particularly the extra 'if (out)' in print(), > by always writing a '\0' on each printc [Xiang W] > > > lib/sbi/sbi_console.c | 39 +++++++++++++++++++++++---------------- > 1 file changed, 23 insertions(+), 16 deletions(-) > > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c > index 34c843d3f9e3..7b9be4a4667f 100644 > --- a/lib/sbi/sbi_console.c > +++ b/lib/sbi/sbi_console.c > @@ -76,20 +76,22 @@ typedef __builtin_va_list va_list; > > static void printc(char **out, u32 *out_len, char ch) > { > - if (out) { > - if (*out) { > - if (out_len && (0 < *out_len)) { > - **out = ch; > - ++(*out); > - (*out_len)--; > - } else { > - **out = ch; > - ++(*out); > - } > - } > - } else { > + if (!out) { > sbi_putc(ch); > + return; > } > + > + /* > + * The *printf entry point functions have enforced that (*out) can > + * only be null when out_len is non-null and its value is zero. > + */ > + if (!out_len || *out_len > 1) { > + *(*out)++ = ch; > + **out = '\0'; > + } > + > + if (out_len && *out_len > 0) > + --(*out_len); > } > > static int prints(char **out, u32 *out_len, const char *string, int width, > @@ -193,7 +195,7 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) > if (*format == '\0') > break; > if (*format == '%') > - goto out; > + goto literal; > /* Get flags */ > if (*format == '-') { > ++format; > @@ -332,13 +334,11 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) > continue; > } > } else { > - out: > +literal: > printc(out, out_len, *format); > ++pc; > } > } > - if (out) > - **out = '\0'; > > return pc; > } > @@ -348,6 +348,9 @@ int sbi_sprintf(char *out, const char *format, ...) > va_list args; > int retval; > > + if (unlikely(!out)) > + sbi_panic("sbi_sprintf called with NULL output string\n"); > + > va_start(args, format); > retval = print(&out, NULL, format, args); > va_end(args); > @@ -360,6 +363,10 @@ int sbi_snprintf(char *out, u32 out_sz, const char *format, ...) > va_list args; > int retval; > > + if (unlikely(!out && out_sz != 0)) > + sbi_panic("sbi_snprintf called with NULL output string and " > + "output size is not zero\n"); > + > va_start(args, format); > retval = print(&out, &out_sz, format, args); > va_end(args); > -- > 2.36.1 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c index 34c843d3f9e3..7b9be4a4667f 100644 --- a/lib/sbi/sbi_console.c +++ b/lib/sbi/sbi_console.c @@ -76,20 +76,22 @@ typedef __builtin_va_list va_list; static void printc(char **out, u32 *out_len, char ch) { - if (out) { - if (*out) { - if (out_len && (0 < *out_len)) { - **out = ch; - ++(*out); - (*out_len)--; - } else { - **out = ch; - ++(*out); - } - } - } else { + if (!out) { sbi_putc(ch); + return; } + + /* + * The *printf entry point functions have enforced that (*out) can + * only be null when out_len is non-null and its value is zero. + */ + if (!out_len || *out_len > 1) { + *(*out)++ = ch; + **out = '\0'; + } + + if (out_len && *out_len > 0) + --(*out_len); } static int prints(char **out, u32 *out_len, const char *string, int width, @@ -193,7 +195,7 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) if (*format == '\0') break; if (*format == '%') - goto out; + goto literal; /* Get flags */ if (*format == '-') { ++format; @@ -332,13 +334,11 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) continue; } } else { - out: +literal: printc(out, out_len, *format); ++pc; } } - if (out) - **out = '\0'; return pc; } @@ -348,6 +348,9 @@ int sbi_sprintf(char *out, const char *format, ...) va_list args; int retval; + if (unlikely(!out)) + sbi_panic("sbi_sprintf called with NULL output string\n"); + va_start(args, format); retval = print(&out, NULL, format, args); va_end(args); @@ -360,6 +363,10 @@ int sbi_snprintf(char *out, u32 out_sz, const char *format, ...) va_list args; int retval; + if (unlikely(!out && out_sz != 0)) + sbi_panic("sbi_snprintf called with NULL output string and " + "output size is not zero\n"); + va_start(args, format); retval = print(&out, &out_sz, format, args); va_end(args);
printc would happily write to 'out' even when 'out_len' was zero, potentially overflowing buffers. Rework printc to not do that and also ensure the null byte is written at the last position when necessary, as stated in the snprintf man page. Also, panic if sprintf or snprintf are called with NULL output strings (except the special case of snprintf having a NULL output string and a zero output size, allowing it to be used to get the number of characters that would have been written). Finally, rename a goto label which clashed with 'out'. Fixes: 9e8ff05cb61f ("Initial commit.") Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- v3: - Add comment in printc stating our expectations of *out v2: - Simply forbid *out == NULL by panicing when it's detected. (The error message for snprintf has been split over two lines to avoid going over 80 chars. I'd prefer error messages not be split, but that seems like the general practice for opensbi.) - Drop some branches, particularly the extra 'if (out)' in print(), by always writing a '\0' on each printc [Xiang W] lib/sbi/sbi_console.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)