[PATCH] lib: sbi: Fix sbi_snprintf

[PATCH] lib: sbi: Fix sbi_snprintf

[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);
 }
 
 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

[PATCH] lib: sbi: Fix sbi_snprintf

[Anup Patel]

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

[PATCH] lib: sbi: Fix sbi_snprintf

[Andrew Jones]

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

[PATCH] lib: sbi: Fix sbi_snprintf

[Xiang W]

? 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

[PATCH] lib: sbi: Fix sbi_snprintf

[Andrew Jones]

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