* [PATCH] lib: sbi: Fix sbi_snprintf
@ 2022-07-26 11:25 Andrew Jones
2022-07-26 11:42 ` Anup Patel
2022-07-27 8:47 ` Xiang W
0 siblings, 2 replies; 5+ messages in thread
From: Andrew Jones @ 2022-07-26 11:25 UTC (permalink / raw)
To: opensbi
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);
}
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] lib: sbi: Fix sbi_snprintf
2022-07-26 11:25 [PATCH] lib: sbi: Fix sbi_snprintf Andrew Jones
@ 2022-07-26 11:42 ` Anup Patel
2022-07-26 13:26 ` Andrew Jones
2022-07-27 8:47 ` Xiang W
1 sibling, 1 reply; 5+ messages in thread
From: Anup Patel @ 2022-07-26 11:42 UTC (permalink / raw)
To: opensbi
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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Regards,
Anup
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] lib: sbi: Fix sbi_snprintf
2022-07-26 11:42 ` Anup Patel
@ 2022-07-26 13:26 ` Andrew Jones
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Jones @ 2022-07-26 13:26 UTC (permalink / raw)
To: opensbi
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] lib: sbi: Fix sbi_snprintf
2022-07-26 11:25 [PATCH] lib: sbi: Fix sbi_snprintf Andrew Jones
2022-07-26 11:42 ` Anup Patel
@ 2022-07-27 8:47 ` Xiang W
2022-07-27 10:35 ` Andrew Jones
1 sibling, 1 reply; 5+ messages in thread
From: Xiang W @ 2022-07-27 8:47 UTC (permalink / raw)
To: opensbi
? 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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] lib: sbi: Fix sbi_snprintf
2022-07-27 8:47 ` Xiang W
@ 2022-07-27 10:35 ` Andrew Jones
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Jones @ 2022-07-27 10:35 UTC (permalink / raw)
To: opensbi
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-27 10:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-26 11:25 [PATCH] lib: sbi: Fix sbi_snprintf Andrew Jones
2022-07-26 11:42 ` Anup Patel
2022-07-26 13:26 ` Andrew Jones
2022-07-27 8:47 ` Xiang W
2022-07-27 10:35 ` Andrew Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox