| Message ID | 20220726112522.5070-1-ajones@ventanamicro.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | lib: sbi: Fix sbi_snprintf | expand |
On Tue, Jul 26, 2022 at 4:55 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. Finally, ensure all > writes to 'out' go through printc and rename a goto label which > clashed with it. > > Fixes: 9e8ff05cb61f ("Initial commit.") > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- > 1 file changed, 20 insertions(+), 15 deletions(-) > > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c > index 34c843d3f9e3..47f361705fc7 100644 > --- a/lib/sbi/sbi_console.c > +++ b/lib/sbi/sbi_console.c > @@ -76,20 +76,24 @@ 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; > } > + > + if (!*out) > + return; What if the output buffer is filled with zeros ? > + > + if (!out_len || *out_len > 1) > + **out = ch; > + else if (*out_len == 1) > + **out = '\0'; > + > + if (out_len && *out_len > 0) > + --(*out_len); > + > + if (!out_len || *out_len > 0) > + ++(*out); > } > > static int prints(char **out, u32 *out_len, const char *string, int width, > @@ -193,7 +197,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 +336,14 @@ 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'; > + printc(out, out_len, '\0'); > > return pc; > } > -- > 2.36.1 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi Regards, Anup
On Tue, Jul 26, 2022 at 05:12:47PM +0530, Anup Patel wrote: > On Tue, Jul 26, 2022 at 4:55 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. Finally, ensure all > > writes to 'out' go through printc and rename a goto label which > > clashed with it. > > > > Fixes: 9e8ff05cb61f ("Initial commit.") > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- > > 1 file changed, 20 insertions(+), 15 deletions(-) > > > > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c > > index 34c843d3f9e3..47f361705fc7 100644 > > --- a/lib/sbi/sbi_console.c > > +++ b/lib/sbi/sbi_console.c > > @@ -76,20 +76,24 @@ 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; > > } > > + > > + if (!*out) > > + return; > > What if the output buffer is filled with zeros ? This single dereferencing of 'out' won't reach the buffer. This check ensures we don't dereference NULL if sprintf/snprintf are called with str=NULL and is just a rewording of the original code. Actually, we probably shouldn't allow str=NULL for sprintf nor for snprintf with size != 0 at all. We could hang in sprintf and snprintf if we detect that. Thanks, drew
在 2022-07-26星期二的 13:25 +0200，Andrew Jones写道： > 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. Finally, ensure all > writes to 'out' go through printc and rename a goto label which > clashed with it. > > Fixes: 9e8ff05cb61f ("Initial commit.") > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- > 1 file changed, 20 insertions(+), 15 deletions(-) > > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c > index 34c843d3f9e3..47f361705fc7 100644 > --- a/lib/sbi/sbi_console.c > +++ b/lib/sbi/sbi_console.c > @@ -76,20 +76,24 @@ 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; > } > + > + if (!*out) > + return; > + > + if (!out_len || *out_len > 1) > + **out = ch; > + else if (*out_len == 1) > + **out = '\0'; > + > + if (out_len && *out_len > 0) > + --(*out_len); > + > + if (!out_len || *out_len > 0) > + ++(*out); Too many branches static void printc(char **out, u32 *out_len, char ch) { if (out && *out && out_len) { if (*out_len > 0) { if (*out_len > 1) { (*out)[0] = ch; (*out)[1] = '\0'; } (*out)++; (*out_len)--; } else // add error message print } else { sbi_putc(ch); } } > } > > static int prints(char **out, u32 *out_len, const char *string, int width, > @@ -193,7 +197,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 +336,14 @@ 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'; > + printc(out, out_len, '\0'); this is not necessary > > return pc; > } > -- > 2.36.1 > > Regards, Xiang W
On Wed, Jul 27, 2022 at 04:47:03PM +0800, Xiang W wrote: > 在 2022-07-26星期二的 13:25 +0200，Andrew Jones写道： > > 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. Finally, ensure all > > writes to 'out' go through printc and rename a goto label which > > clashed with it. > > > > Fixes: 9e8ff05cb61f ("Initial commit.") > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- > > 1 file changed, 20 insertions(+), 15 deletions(-) > > > > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c > > index 34c843d3f9e3..47f361705fc7 100644 > > --- a/lib/sbi/sbi_console.c > > +++ b/lib/sbi/sbi_console.c > > @@ -76,20 +76,24 @@ 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; > > } > > + > > + if (!*out) > > + return; > > + > > + if (!out_len || *out_len > 1) > > + **out = ch; > > + else if (*out_len == 1) > > + **out = '\0'; > > + > > + if (out_len && *out_len > 0) > > + --(*out_len); > > + > > + if (!out_len || *out_len > 0) > > + ++(*out); > Too many branches > > static void printc(char **out, u32 *out_len, char ch) > { > if (out && *out && out_len) { It's possible to have non-NULL out/*out and NULL out_len (when sprintf is used), so we need at least one other branch. > if (*out_len > 0) { > if (*out_len > 1) { > (*out)[0] = ch; > (*out)[1] = '\0'; Sure. It saves a branch and allows us to drop the extra if (out) branch in print() below which you pointed out. > } > (*out)++; > (*out_len)--; > } else > // add error message print > } else { > sbi_putc(ch); > } > } > > > } > > > > static int prints(char **out, u32 *out_len, const char *string, int width, > > @@ -193,7 +197,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 +336,14 @@ 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'; > > + printc(out, out_len, '\0'); > this is not necessary I'll send a v2 with the "always write '\0'" approach integrated. Thanks, drew
diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c index 34c843d3f9e3..47f361705fc7 100644 --- a/lib/sbi/sbi_console.c +++ b/lib/sbi/sbi_console.c @@ -76,20 +76,24 @@ 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; } + + if (!*out) + return; + + if (!out_len || *out_len > 1) + **out = ch; + else if (*out_len == 1) + **out = '\0'; + + if (out_len && *out_len > 0) + --(*out_len); + + if (!out_len || *out_len > 0) + ++(*out); } static int prints(char **out, u32 *out_len, const char *string, int width, @@ -193,7 +197,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 +336,14 @@ 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'; + printc(out, out_len, '\0'); return pc; }
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. Finally, ensure all writes to 'out' go through printc and rename a goto label which clashed with it. Fixes: 9e8ff05cb61f ("Initial commit.") Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)