[PATCH 0/4] lib/vsprintf: assorted bug fixes

[PATCH 0/4] lib/vsprintf: assorted bug fixes

[Josh Law]

Four small fixes found during an audit of lib/vsprintf.c:

1. bstr_printf() fails to advance the args pointer past a
   pre-rendered pointer string when the output buffer is full,
   corrupting all subsequent output.

2. vbin_printf() writes end[-1] unconditionally when NUL-terminating
   a pointer string, which is an OOB write when size is zero.

3. vsscanf() uses s16 for field_width but assigns from skip_atoi()
   which returns int, silently truncating large widths to negative
   and aborting parsing.

4. format_decode() is missing a (u8) cast on the second lookup into
   the format_state table, allowing a negative array index on
   signed-char platforms.

Josh Law (4):
  lib/vsprintf: always advance args in bstr_printf() pointer path
  lib/vsprintf: fix OOB write in vbin_printf() when size is zero
  lib/vsprintf: use int for field_width in vsscanf()
  lib/vsprintf: add missing (u8) cast in format_decode() lookup

 lib/vsprintf.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/4] lib/vsprintf: always advance args in bstr_printf() pointer path

[Josh Law]

When the output buffer is full (str >= end), bstr_printf() skips
advancing the args pointer past the pre-rendered pointer string in
bin_buf. This causes all subsequent format specifiers to read from
the wrong position, corrupting the rest of the output.

Always compute the string length and advance args regardless of
whether there is space to copy into the output buffer.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/vsprintf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 800b8ac49f53..7898fb998b21 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3389,14 +3389,15 @@ int bstr_printf(char *buf, size_t size, const char *fmt_str, const u32 *bin_buf)
 					break;
 				}
 				/* Pointer dereference was already processed */
+				len = strlen(args);
 				if (str < end) {
-					len = copy = strlen(args);
+					copy = len;
 					if (copy > end - str)
 						copy = end - str;
 					memcpy(str, args, copy);
-					str += len;
-					args += len + 1;
 				}
+				str += len;
+				args += len + 1;
 			}
 			if (process)
 				str = pointer(fmt.str, str, end, get_arg(void *), spec);
-- 
2.34.1

Re: [PATCH 1/4] lib/vsprintf: always advance args in bstr_printf() pointer path

[Petr Mladek]

On Tue 2026-03-24 22:49:37, Josh Law wrote:
> When the output buffer is full (str >= end), bstr_printf() skips
> advancing the args pointer past the pre-rendered pointer string in
> bin_buf. This causes all subsequent format specifiers to read from
> the wrong position, corrupting the rest of the output.
> 
> Always compute the string length and advance args regardless of
> whether there is space to copy into the output buffer.
> 
> Signed-off-by: Josh Law <objecting@objecting.org>

It looks correct to me. It is interesting that nobody found it yet.
But I guess that all users of bstr_printf() are using big enough
buffers so that it is hard to hit in practice.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH 2/4] lib/vsprintf: fix OOB write in vbin_printf() when size is zero

[Josh Law]

When vbin_printf() is called with size==0, end equals bin_buf and
the else branch writes end[-1], which is one byte before the buffer.

Guard the write so it only happens when the buffer is non-empty.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7898fb998b21..72fbfe181076 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3234,7 +3234,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt_str, va_list args)
 					      spec);
 				if (str + 1 < end)
 					*str++ = '\0';
-				else
+				else if (end > (char *)bin_buf)
 					end[-1] = '\0'; /* Must be nul terminated */
 			}
 			/* skip all alphanumeric pointer suffixes */
-- 
2.34.1

Re: [PATCH 2/4] lib/vsprintf: fix OOB write in vbin_printf() when size is zero

[Petr Mladek]

On Tue 2026-03-24 22:49:38, Josh Law wrote:
> When vbin_printf() is called with size==0, end equals bin_buf and
> the else branch writes end[-1], which is one byte before the buffer.
> 
> Guard the write so it only happens when the buffer is non-empty.
> 
> Signed-off-by: Josh Law <objecting@objecting.org>

Great catch!

There is only one in-tree user and it never passes size=0 so there was
no overflow in the reality. But better be on the safe side.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 2/4] lib/vsprintf: fix OOB write in vbin_printf() when size is zero

[Steven Rostedt]

On Tue, 24 Mar 2026 22:49:38 +0000
Josh Law <objecting@objecting.org> wrote:

> When vbin_printf() is called with size==0, end equals bin_buf and
> the else branch writes end[-1], which is one byte before the buffer.
> 
> Guard the write so it only happens when the buffer is non-empty.
> 
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 7898fb998b21..72fbfe181076 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -3234,7 +3234,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt_str, va_list args)
>  					      spec);
>  				if (str + 1 < end)
>  					*str++ = '\0';
> -				else
> +				else if (end > (char *)bin_buf)
>  					end[-1] = '\0'; /* Must be nul terminated */

instead of doing a comparison, couldn't we just check size?

				else if (size) /* do nothing if size is zero */

I'm guessing this would be slightly more efficient in the assembly.

-- Steve

>  			}
>  			/* skip all alphanumeric pointer suffixes */

Re: [PATCH 2/4] lib/vsprintf: fix OOB write in vbin_printf() when size is zero

[Josh Law]

---- On Mon, 30 Mar 2026 17:30:19 +0100 rostedt@goodmis.org wrote ----


> On Tue, 24 Mar 2026 22:49:38 +0000
> Josh Law wrote:
>
> > When vbin_printf() is called with size==0, end equals bin_buf and
> > the else branch writes end[-1], which is one byte before the buffer.
> >
> > Guard the write so it only happens when the buffer is non-empty.
> >
> > Signed-off-by: Josh Law
>
> > ---
> > lib/vsprintf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7898fb998b21..72fbfe181076 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -3234,7 +3234,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char
> *fmt_str, va_list args)
> >                      spec);
> >                 if (str + 1 < end)
> >                     *str++ = '\0';
> > -                else
> > +                else if (end > (char *)bin_buf)
> >                     end[-1] = '\0'; /* Must be nul terminated */
>
> instead of doing a comparison, couldn't we just check size?
>
>                 else if (size) /* do nothing if size is zero */
>
> I'm guessing this would be slightly more efficient in the assembly.
>
> -- Steve
>
> >             }
> >             /* skip all alphanumeric pointer suffixes */

Re: [PATCH 2/4] lib/vsprintf: fix OOB write in vbin_printf() when size is zero

[Josh Law]

---- On Mon, 30 Mar 2026 17:30:19 +0100 rostedt@goodmis.org wrote ----


> On Tue, 24 Mar 2026 22:49:38 +0000
> Josh Law wrote:
>
> > When vbin_printf() is called with size==0, end equals bin_buf and
> > the else branch writes end[-1], which is one byte before the buffer.
> >
> > Guard the write so it only happens when the buffer is non-empty.
> >
> > Signed-off-by: Josh Law
>
> > ---
> > lib/vsprintf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7898fb998b21..72fbfe181076 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -3234,7 +3234,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char
> *fmt_str, va_list args)
> >                      spec);
> >                 if (str + 1 < end)
> >                     *str++ = '\0';
> > -                else
> > +                else if (end > (char *)bin_buf)
> >                     end[-1] = '\0'; /* Must be nul terminated */
>
> instead of doing a comparison, couldn't we just check size?
>
>                 else if (size) /* do nothing if size is zero */
>
> I'm guessing this would be slightly more efficient in the assembly.
>
> -- Steve
>
> >             }
> >             /* skip all alphanumeric pointer suffixes */

Re: [PATCH 2/4] lib/vsprintf: fix OOB write in vbin_printf() when size is zero

[Josh Law]

---- On Mon, 30 Mar 2026 17:30:19 +0100 rostedt@goodmis.org wrote ----


> On Tue, 24 Mar 2026 22:49:38 +0000
> Josh Law wrote:
>
> > When vbin_printf() is called with size==0, end equals bin_buf and
> > the else branch writes end[-1], which is one byte before the buffer.
> >
> > Guard the write so it only happens when the buffer is non-empty.
> >
> > Signed-off-by: Josh Law
>
> > ---
> > lib/vsprintf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7898fb998b21..72fbfe181076 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -3234,7 +3234,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char
> *fmt_str, va_list args)
> >                      spec);
> >                 if (str + 1 < end)
> >                     *str++ = '\0';
> > -                else
> > +                else if (end > (char *)bin_buf)
> >                     end[-1] = '\0'; /* Must be nul terminated */
>
> instead of doing a comparison, couldn't we just check size?
>
>                 else if (size) /* do nothing if size is zero */
>
> I'm guessing this would be slightly more efficient in the assembly.
>
> -- Steve
>
> >             }
> >             /* skip all alphanumeric pointer suffixes */

Re: [PATCH 2/4] lib/vsprintf: fix OOB write in vbin_printf() when size is zero

[Josh Law]

---- On Mon, 30 Mar 2026 17:30:19 +0100 rostedt@goodmis.org wrote ----


> On Tue, 24 Mar 2026 22:49:38 +0000
> Josh Law wrote:
>
> > When vbin_printf() is called with size==0, end equals bin_buf and
> > the else branch writes end[-1], which is one byte before the buffer.
> >
> > Guard the write so it only happens when the buffer is non-empty.
> >
> > Signed-off-by: Josh Law
>
> > ---
> > lib/vsprintf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7898fb998b21..72fbfe181076 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -3234,7 +3234,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char
> *fmt_str, va_list args)
> >                      spec);
> >                 if (str + 1 < end)
> >                     *str++ = '\0';
> > -                else
> > +                else if (end > (char *)bin_buf)
> >                     end[-1] = '\0'; /* Must be nul terminated */
>
> instead of doing a comparison, couldn't we just check size?
>
>                 else if (size) /* do nothing if size is zero */


Good call. I'll make a V2 soon on that patch.

>
> I'm guessing this would be slightly more efficient in the assembly.
>
> -- Steve
>
> >             }
> >             /* skip all alphanumeric pointer suffixes */

[PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[Josh Law]

vsscanf() declares field_width as s16 but assigns it from skip_atoi()
which returns int. Values above 32767 silently truncate to negative,
causing vsscanf() to abort all remaining parsing. This is inconsistent
with struct printf_spec which uses int for field_width.

Change the type to int to match.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 72fbfe181076..2758096b6f53 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3461,7 +3461,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		long long s;
 		unsigned long long u;
 	} val;
-	s16 field_width;
+	int field_width;
 	bool is_sign;
 
 	while (*fmt) {
-- 
2.34.1

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[Andy Shevchenko]

On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:
> vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> which returns int. Values above 32767 silently truncate to negative,
> causing vsscanf() to abort all remaining parsing. This is inconsistent
> with struct printf_spec which uses int for field_width.

Is the field_width an acceptable integer range by the specifications?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[Petr Mladek]

On Wed 2026-03-25 14:00:17, Andy Shevchenko wrote:
> On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:
> > vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> > which returns int. Values above 32767 silently truncate to negative,
> > causing vsscanf() to abort all remaining parsing. This is inconsistent
> > with struct printf_spec which uses int for field_width.
> 
> Is the field_width an acceptable integer range by the specifications?

I am not sure what is allowed by specification. Anyway, the code is
not ready for a bigger values, for example:

		case 's':
		{
			char *s = (char *)va_arg(args, char *);
			if (field_width == -1)
				field_width = SHRT_MAX;

clearly expects signed short int range.

I wonder if it might even open some backdoor. The code matching
as sequence of characters expects a defined field width, see


		case '[':
		{
[...]
			/* field width is required */
			if (field_width == -1)
				return num;

The current code limits valid field width values to positive ones,
aka SHRT_MAX which is clearly much lover than INT_MAX. And it might
prevent some out of bound access.

Best Regards,
Petr

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[David Laight]

On Tue, 31 Mar 2026 16:31:50 +0200
Petr Mladek <pmladek@suse.com> wrote:

> On Wed 2026-03-25 14:00:17, Andy Shevchenko wrote:
> > On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:  
> > > vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> > > which returns int. Values above 32767 silently truncate to negative,
> > > causing vsscanf() to abort all remaining parsing. This is inconsistent
> > > with struct printf_spec which uses int for field_width. 
> > 
> > Is the field_width an acceptable integer range by the specifications?  
> 
> I am not sure what is allowed by specification. Anyway, the code is
> not ready for a bigger values, for example:
> 
> 		case 's':
> 		{
> 			char *s = (char *)va_arg(args, char *);
> 			if (field_width == -1)
> 				field_width = SHRT_MAX;
> 
> clearly expects signed short int range.
> 
> I wonder if it might even open some backdoor. The code matching
> as sequence of characters expects a defined field width, see
> 
> 
> 		case '[':
> 		{
> [...]
> 			/* field width is required */
> 			if (field_width == -1)
> 				return num;
> 
> The current code limits valid field width values to positive ones,
> aka SHRT_MAX which is clearly much lover than INT_MAX. And it might
> prevent some out of bound access.
> 
> Best Regards,
> Petr
> 
> 

Notwithstanding what the code actually does there is no point defining a
local variable as a 'short' unless you really want arithmetic to wrap
at 16 bits.
All it does is force the compiler to keep adding code to fix the sign
extension to 32 bits.
Look at the object for anything other than x86 (or m68k).

	David

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[Petr Mladek]

On Tue 2026-03-31 16:35:22, David Laight wrote:
> On Tue, 31 Mar 2026 16:31:50 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > On Wed 2026-03-25 14:00:17, Andy Shevchenko wrote:
> > > On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:  
> > > > vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> > > > which returns int. Values above 32767 silently truncate to negative,
> > > > causing vsscanf() to abort all remaining parsing. This is inconsistent
> > > > with struct printf_spec which uses int for field_width. 
> > > 
> > > Is the field_width an acceptable integer range by the specifications?  
> > 
> > I am not sure what is allowed by specification. Anyway, the code is
> > not ready for a bigger values, for example:
> > 
> > 		case 's':
> > 		{
> > 			char *s = (char *)va_arg(args, char *);
> > 			if (field_width == -1)
> > 				field_width = SHRT_MAX;
> > 
> > clearly expects signed short int range.
> > 
> > I wonder if it might even open some backdoor. The code matching
> > as sequence of characters expects a defined field width, see
> > 
> > 
> > 		case '[':
> > 		{
> > [...]
> > 			/* field width is required */
> > 			if (field_width == -1)
> > 				return num;
> > 
> > The current code limits valid field width values to positive ones,

I meant this code:

		/* get field width */
		field_width = -1;
		if (isdigit(*fmt)) {
			field_width = skip_atoi(&fmt);
			if (field_width <= 0)
				break;
		}

If we change the type of the local variable then the above check will
suddenly accept fied_width <= INT_MAX instead of SHRT_MAX.

As a result, The above mentioned "case '[':" handling will suddely
allow to iternate over INT_MAX long string instead of SHRT_MAX.

I doubt that there is any kernel code which would be affected
by this. But I do not want to risk it.

> > aka SHRT_MAX which is clearly much lover than INT_MAX. And it might
> > prevent some out of bound access.
 
> > Best Regards,
> > Petr
> > 
> > 
> 
> Notwithstanding what the code actually does there is no point defining a
> local variable as a 'short' unless you really want arithmetic to wrap
> at 16 bits.
> All it does is force the compiler to keep adding code to fix the sign
> extension to 32 bits.
> Look at the object for anything other than x86 (or m68k).

If you think that it is important enough, feel free to send
a patch.

I not taking this patch from Josh Law, definitely!

Best Regards,
Petr

PS: Note that Josh Law seems to be an AI virtual person, see
    https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/

    I am even not sure what to do with the other 3 patches. They look
    correct. But I should not take patches with an unclear origin, see
    https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[Josh Law]

---- On Tue, 31 Mar 2026 17:12:18 +0100 pmladek@suse.com wrote ----


> On Tue 2026-03-31 16:35:22, David Laight wrote:
> > On Tue, 31 Mar 2026 16:31:50 +0200
> > Petr Mladek wrote:
> >
> > > On Wed 2026-03-25 14:00:17, Andy Shevchenko wrote:
> > > > On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:
> > > > > vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> > > > > which returns int. Values above 32767 silently truncate to negative,
> > > > > causing vsscanf() to abort all remaining parsing. This is inconsistent
> > > > > with struct printf_spec which uses int for field_width.
> > > >
> > > > Is the field_width an acceptable integer range by the specifications?
> > >
> > > I am not sure what is allowed by specification. Anyway, the code is
> > > not ready for a bigger values, for example:
> > >
> > >         case 's':
> > >         {
> > >             char *s = (char *)va_arg(args, char *);
> > >             if (field_width == -1)
> > >                 field_width = SHRT_MAX;
> > >
> > > clearly expects signed short int range.
> > >
> > > I wonder if it might even open some backdoor. The code matching
> > > as sequence of characters expects a defined field width, see
> > >
> > >
> > >         case '[':
> > >         {
> > > [...]
> > >             /* field width is required */
> > >             if (field_width == -1)
> > >                 return num;
> > >
> > > The current code limits valid field width values to positive ones,
>
> I meant this code:
>
>         /* get field width */
>         field_width = -1;
>         if (isdigit(*fmt)) {
>             field_width = skip_atoi(&fmt);
>             if (field_width <= 0)
>                 break;
>         }
>
> If we change the type of the local variable then the above check will
> suddenly accept fied_width <= INT_MAX instead of SHRT_MAX.
>
> As a result, The above mentioned "case '[':" handling will suddely
> allow to iternate over INT_MAX long string instead of SHRT_MAX.
>
> I doubt that there is any kernel code which would be affected
> by this. But I do not want to risk it.
>
> > > aka SHRT_MAX which is clearly much lover than INT_MAX. And it might
> > > prevent some out of bound access.
>
> > > Best Regards,
> > > Petr
> > >
> > >
> >
> > Notwithstanding what the code actually does there is no point defining a
> > local variable as a 'short' unless you really want arithmetic to wrap
> > at 16 bits.
> > All it does is force the compiler to keep adding code to fix the sign
> > extension to 32 bits.
> > Look at the object for anything other than x86 (or m68k).
>
> If you think that it is important enough, feel free to send
> a patch.
>
> I not taking this patch from Josh Law, definitely!
>
> Best Regards,
> Petr
>
> PS: Note that Josh Law seems to be an AI virtual person, see
> https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/[https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/]
>
> I am even not sure what to do with the other 3 patches. They look
> correct. But I should not take patches with an unclear origin, see
> https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/[https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/]




I'm gonna arrange a video call with someone to fix this

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[Josh Law]

---- On Tue, 31 Mar 2026 17:12:18 +0100 pmladek@suse.com wrote ----


> On Tue 2026-03-31 16:35:22, David Laight wrote:
> > On Tue, 31 Mar 2026 16:31:50 +0200
> > Petr Mladek wrote:
> >
> > > On Wed 2026-03-25 14:00:17, Andy Shevchenko wrote:
> > > > On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:
> > > > > vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> > > > > which returns int. Values above 32767 silently truncate to negative,
> > > > > causing vsscanf() to abort all remaining parsing. This is inconsistent
> > > > > with struct printf_spec which uses int for field_width.
> > > >
> > > > Is the field_width an acceptable integer range by the specifications?
> > >
> > > I am not sure what is allowed by specification. Anyway, the code is
> > > not ready for a bigger values, for example:
> > >
> > >         case 's':
> > >         {
> > >             char *s = (char *)va_arg(args, char *);
> > >             if (field_width == -1)
> > >                 field_width = SHRT_MAX;
> > >
> > > clearly expects signed short int range.
> > >
> > > I wonder if it might even open some backdoor. The code matching
> > > as sequence of characters expects a defined field width, see
> > >
> > >
> > >         case '[':
> > >         {
> > > [...]
> > >             /* field width is required */
> > >             if (field_width == -1)
> > >                 return num;
> > >
> > > The current code limits valid field width values to positive ones,
>
> I meant this code:
>
>         /* get field width */
>         field_width = -1;
>         if (isdigit(*fmt)) {
>             field_width = skip_atoi(&fmt);
>             if (field_width <= 0)
>                 break;
>         }
>
> If we change the type of the local variable then the above check will
> suddenly accept fied_width <= INT_MAX instead of SHRT_MAX.
>
> As a result, The above mentioned "case '[':" handling will suddely
> allow to iternate over INT_MAX long string instead of SHRT_MAX.
>
> I doubt that there is any kernel code which would be affected
> by this. But I do not want to risk it.
>
> > > aka SHRT_MAX which is clearly much lover than INT_MAX. And it might
> > > prevent some out of bound access.
>
> > > Best Regards,
> > > Petr
> > >
> > >
> >
> > Notwithstanding what the code actually does there is no point defining a
> > local variable as a 'short' unless you really want arithmetic to wrap
> > at 16 bits.
> > All it does is force the compiler to keep adding code to fix the sign
> > extension to 32 bits.
> > Look at the object for anything other than x86 (or m68k).
>
> If you think that it is important enough, feel free to send
> a patch.
>
> I not taking this patch from Josh Law, definitely!
>
> Best Regards,
> Petr
>
> PS: Note that Josh Law seems to be an AI virtual person, see
> https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/[https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/]
>
> I am even not sure what to do with the other 3 patches. They look
> correct. But I should not take patches with an unclear origin, see
> https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/[https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/]




I'm gonna arrange a video call with someone to fix this

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[Josh Law]

---- On Tue, 31 Mar 2026 17:12:18 +0100 pmladek@suse.com wrote ----


> On Tue 2026-03-31 16:35:22, David Laight wrote:
> > On Tue, 31 Mar 2026 16:31:50 +0200
> > Petr Mladek wrote:
> >
> > > On Wed 2026-03-25 14:00:17, Andy Shevchenko wrote:
> > > > On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:
> > > > > vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> > > > > which returns int. Values above 32767 silently truncate to negative,
> > > > > causing vsscanf() to abort all remaining parsing. This is inconsistent
> > > > > with struct printf_spec which uses int for field_width.
> > > >
> > > > Is the field_width an acceptable integer range by the specifications?
> > >
> > > I am not sure what is allowed by specification. Anyway, the code is
> > > not ready for a bigger values, for example:
> > >
> > >         case 's':
> > >         {
> > >             char *s = (char *)va_arg(args, char *);
> > >             if (field_width == -1)
> > >                 field_width = SHRT_MAX;
> > >
> > > clearly expects signed short int range.
> > >
> > > I wonder if it might even open some backdoor. The code matching
> > > as sequence of characters expects a defined field width, see
> > >
> > >
> > >         case '[':
> > >         {
> > > [...]
> > >             /* field width is required */
> > >             if (field_width == -1)
> > >                 return num;
> > >
> > > The current code limits valid field width values to positive ones,
>
> I meant this code:
>
>         /* get field width */
>         field_width = -1;
>         if (isdigit(*fmt)) {
>             field_width = skip_atoi(&fmt);
>             if (field_width <= 0)
>                 break;
>         }
>
> If we change the type of the local variable then the above check will
> suddenly accept fied_width <= INT_MAX instead of SHRT_MAX.
>
> As a result, The above mentioned "case '[':" handling will suddely
> allow to iternate over INT_MAX long string instead of SHRT_MAX.
>
> I doubt that there is any kernel code which would be affected
> by this. But I do not want to risk it.
>
> > > aka SHRT_MAX which is clearly much lover than INT_MAX. And it might
> > > prevent some out of bound access.
>
> > > Best Regards,
> > > Petr
> > >
> > >
> >
> > Notwithstanding what the code actually does there is no point defining a
> > local variable as a 'short' unless you really want arithmetic to wrap
> > at 16 bits.
> > All it does is force the compiler to keep adding code to fix the sign
> > extension to 32 bits.
> > Look at the object for anything other than x86 (or m68k).
>
> If you think that it is important enough, feel free to send
> a patch.
>
> I not taking this patch from Josh Law, definitely!
>
> Best Regards,
> Petr
>
> PS: Note that Josh Law seems to be an AI virtual person, see
> https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/[https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/]
>
> I am even not sure what to do with the other 3 patches. They look
> correct. But I should not take patches with an unclear origin, see
> https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/[https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/]




I'm gonna arrange a video call with someone to fix this

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[David Laight]

On Tue, 31 Mar 2026 18:12:18 +0200
Petr Mladek <pmladek@suse.com> wrote:

> On Tue 2026-03-31 16:35:22, David Laight wrote:
> > On Tue, 31 Mar 2026 16:31:50 +0200
> > Petr Mladek <pmladek@suse.com> wrote:
> >   
> > > On Wed 2026-03-25 14:00:17, Andy Shevchenko wrote:  
> > > > On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:    
> > > > > vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> > > > > which returns int. Values above 32767 silently truncate to negative,
> > > > > causing vsscanf() to abort all remaining parsing. This is inconsistent
> > > > > with struct printf_spec which uses int for field_width.   
> > > > 
> > > > Is the field_width an acceptable integer range by the specifications?    
> > > 
> > > I am not sure what is allowed by specification. Anyway, the code is
> > > not ready for a bigger values, for example:
> > > 
> > > 		case 's':
> > > 		{
> > > 			char *s = (char *)va_arg(args, char *);
> > > 			if (field_width == -1)
> > > 				field_width = SHRT_MAX;
> > > 
> > > clearly expects signed short int range.
> > > 
> > > I wonder if it might even open some backdoor. The code matching
> > > as sequence of characters expects a defined field width, see
> > > 
> > > 
> > > 		case '[':
> > > 		{
> > > [...]
> > > 			/* field width is required */
> > > 			if (field_width == -1)
> > > 				return num;
> > > 
> > > The current code limits valid field width values to positive ones,  
> 
> I meant this code:
> 
> 		/* get field width */
> 		field_width = -1;
> 		if (isdigit(*fmt)) {
> 			field_width = skip_atoi(&fmt);
> 			if (field_width <= 0)
> 				break;
> 		}
> 
> If we change the type of the local variable then the above check will
> suddenly accept field_width <= INT_MAX instead of SHRT_MAX.

But it is all broken anyway - consider "%65537[abc]".
The test needs to be:
			if (field_width <= 0 || field_width > MAX_XXX)
(and I doubt 32767 is a same limit anyway).

(oh and skip_atoi() need to either saturate or stop consuming digits)

	David


> 
> As a result, The above mentioned "case '[':" handling will suddely
> allow to iternate over INT_MAX long string instead of SHRT_MAX.
> 
> I doubt that there is any kernel code which would be affected
> by this. But I do not want to risk it.
> 
> > > aka SHRT_MAX which is clearly much lover than INT_MAX. And it might
> > > prevent some out of bound access.  
>  
> > > Best Regards,
> > > Petr
> > > 
> > >   
> > 
> > Notwithstanding what the code actually does there is no point defining a
> > local variable as a 'short' unless you really want arithmetic to wrap
> > at 16 bits.
> > All it does is force the compiler to keep adding code to fix the sign
> > extension to 32 bits.
> > Look at the object for anything other than x86 (or m68k).  
> 
> If you think that it is important enough, feel free to send
> a patch.
> 
> I not taking this patch from Josh Law, definitely!
> 
> Best Regards,
> Petr
> 
> PS: Note that Josh Law seems to be an AI virtual person, see
>     https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/
> 
>     I am even not sure what to do with the other 3 patches. They look
>     correct. But I should not take patches with an unclear origin, see
>     https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/
> 

Re: [PATCH 3/4] lib/vsprintf: use int for field_width in vsscanf()

[David Laight]

On Wed, 1 Apr 2026 15:22:56 +0100
David Laight <david.laight.linux@gmail.com> wrote:

> On Tue, 31 Mar 2026 18:12:18 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > On Tue 2026-03-31 16:35:22, David Laight wrote:  
> > > On Tue, 31 Mar 2026 16:31:50 +0200
> > > Petr Mladek <pmladek@suse.com> wrote:
> > >     
> > > > On Wed 2026-03-25 14:00:17, Andy Shevchenko wrote:    
> > > > > On Tue, Mar 24, 2026 at 10:49:39PM +0000, Josh Law wrote:      
> > > > > > vsscanf() declares field_width as s16 but assigns it from skip_atoi()
> > > > > > which returns int. Values above 32767 silently truncate to negative,
> > > > > > causing vsscanf() to abort all remaining parsing. This is inconsistent
> > > > > > with struct printf_spec which uses int for field_width.     
> > > > > 
> > > > > Is the field_width an acceptable integer range by the specifications?      
> > > > 
> > > > I am not sure what is allowed by specification. Anyway, the code is
> > > > not ready for a bigger values, for example:
> > > > 
> > > > 		case 's':
> > > > 		{
> > > > 			char *s = (char *)va_arg(args, char *);
> > > > 			if (field_width == -1)
> > > > 				field_width = SHRT_MAX;
> > > > 
> > > > clearly expects signed short int range.
> > > > 
> > > > I wonder if it might even open some backdoor. The code matching
> > > > as sequence of characters expects a defined field width, see
> > > > 
> > > > 
> > > > 		case '[':
> > > > 		{
> > > > [...]
> > > > 			/* field width is required */
> > > > 			if (field_width == -1)
> > > > 				return num;
> > > > 
> > > > The current code limits valid field width values to positive ones,    
> > 
> > I meant this code:
> > 
> > 		/* get field width */
> > 		field_width = -1;
> > 		if (isdigit(*fmt)) {
> > 			field_width = skip_atoi(&fmt);
> > 			if (field_width <= 0)
> > 				break;
> > 		}
> > 
> > If we change the type of the local variable then the above check will
> > suddenly accept field_width <= INT_MAX instead of SHRT_MAX.  
> 
> But it is all broken anyway - consider "%65537[abc]".
> The test needs to be:
> 			if (field_width <= 0 || field_width > MAX_XXX)
> (and I doubt 32767 is a same limit anyway).
> 
> (oh and skip_atoi() need to either saturate or stop consuming digits)


Actually I realised later this is all pointless.
All the formats are literal strings in the kernel - they are known to be ok.
I think even the (field_width <= 0) test can just be deleted.
The code won't loop forever, it might overrun the output buffer but
than can happen with a syntactically valid length as well.

	David

> 
> 	David
> 
> 
> > 
> > As a result, The above mentioned "case '[':" handling will suddely
> > allow to iternate over INT_MAX long string instead of SHRT_MAX.
> > 
> > I doubt that there is any kernel code which would be affected
> > by this. But I do not want to risk it.
> >   
> > > > aka SHRT_MAX which is clearly much lover than INT_MAX. And it might
> > > > prevent some out of bound access.    
> >    
> > > > Best Regards,
> > > > Petr
> > > > 
> > > >     
> > > 
> > > Notwithstanding what the code actually does there is no point defining a
> > > local variable as a 'short' unless you really want arithmetic to wrap
> > > at 16 bits.
> > > All it does is force the compiler to keep adding code to fix the sign
> > > extension to 32 bits.
> > > Look at the object for anything other than x86 (or m68k).    
> > 
> > If you think that it is important enough, feel free to send
> > a patch.
> > 
> > I not taking this patch from Josh Law, definitely!
> > 
> > Best Regards,
> > Petr
> > 
> > PS: Note that Josh Law seems to be an AI virtual person, see
> >     https://lore.kernel.org/all/cbd0aafa-bd45-4f4d-a2dd-440473657dba@lucifer.local/
> > 
> >     I am even not sure what to do with the other 3 patches. They look
> >     correct. But I should not take patches with an unclear origin, see
> >     https://lore.kernel.org/all/2f824be3-7b30-41c6-b517-de1086624171@kernel.org/
> >   
> 
> 

[PATCH 4/4] lib/vsprintf: add missing (u8) cast in format_decode() lookup

[Josh Law]

The first lookup into the format_state table correctly casts to (u8)
at line 2778, but the second lookup after consuming a length qualifier
does not. On signed-char platforms, a byte >= 0x80 sign-extends to a
negative index, reading before the array.

Add the same (u8) cast for consistency.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 2758096b6f53..3108823e8c22 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2783,7 +2783,7 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
 			fmt.str++;
 		}
 		fmt.str++;
-		p = lookup_state + *fmt.str;
+		p = lookup_state + (u8)*fmt.str;
 	}
 	if (p->state) {
 		if (p->base)
-- 
2.34.1

Re: [PATCH 4/4] lib/vsprintf: add missing (u8) cast in format_decode() lookup

[Andy Shevchenko]

On Tue, Mar 24, 2026 at 10:49:40PM +0000, Josh Law wrote:
> The first lookup into the format_state table correctly casts to (u8)
> at line 2778, but the second lookup after consuming a length qualifier
> does not. On signed-char platforms, a byte >= 0x80 sign-extends to a
> negative index, reading before the array.
> 
> Add the same (u8) cast for consistency.

Maybe yes, but get familiar on how the Linux kernel is built.
There is no such possibility IRL with this project since a commit
in the past. Feel free to find what I meant as your learning curve.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] lib/vsprintf: add missing (u8) cast in format_decode() lookup

[Petr Mladek]

On Tue 2026-03-24 22:49:40, Josh Law wrote:
> The first lookup into the format_state table correctly casts to (u8)
> at line 2778, but the second lookup after consuming a length qualifier
> does not. On signed-char platforms, a byte >= 0x80 sign-extends to a
> negative index, reading before the array.
> 
> Add the same (u8) cast for consistency.
> 
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 2758096b6f53..3108823e8c22 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2783,7 +2783,7 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
>  			fmt.str++;
>  		}
>  		fmt.str++;
> -		p = lookup_state + *fmt.str;
> +		p = lookup_state + (u8)*fmt.str;
>  	}
>  	if (p->state) {
>  		if (p->base)

This makes sense. Even though the current code is safe as pointed
out by Andy.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 4/4] lib/vsprintf: add missing (u8) cast in format_decode() lookup

[Josh Law]

---- On Tue, 31 Mar 2026 15:33:53 +0100 pmladek@suse.com wrote ----


> On Tue 2026-03-24 22:49:40, Josh Law wrote:
> > The first lookup into the format_state table correctly casts to (u8)
> > at line 2778, but the second lookup after consuming a length qualifier
> > does not. On signed-char platforms, a byte >= 0x80 sign-extends to a
> > negative index, reading before the array.
> >
> > Add the same (u8) cast for consistency.
> >
> > Signed-off-by: Josh Law
>
> > ---
> > lib/vsprintf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 2758096b6f53..3108823e8c22 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -2783,7 +2783,7 @@ struct fmt format_decode(struct fmt fmt, struct
> printf_spec *spec)
> >             fmt.str++;
> >         }
> >         fmt.str++;
> > -        p = lookup_state + *fmt.str;
> > +        p = lookup_state + (u8)*fmt.str;
> >     }
> >     if (p->state) {
> >         if (p->base)
>
> This makes sense. Even though the current code is safe as pointed
> out by Andy.
>
> Reviewed-by: Petr Mladek
>
> Best Regards,
> Petr


Yeah, better safe than sorry in my opinion.


Thanks for the review petr!

Re: [PATCH 4/4] lib/vsprintf: add missing (u8) cast in format_decode() lookup

[Josh Law]

---- On Tue, 31 Mar 2026 15:33:53 +0100 pmladek@suse.com wrote ----


> On Tue 2026-03-24 22:49:40, Josh Law wrote:
> > The first lookup into the format_state table correctly casts to (u8)
> > at line 2778, but the second lookup after consuming a length qualifier
> > does not. On signed-char platforms, a byte >= 0x80 sign-extends to a
> > negative index, reading before the array.
> >
> > Add the same (u8) cast for consistency.
> >
> > Signed-off-by: Josh Law
>
> > ---
> > lib/vsprintf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 2758096b6f53..3108823e8c22 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -2783,7 +2783,7 @@ struct fmt format_decode(struct fmt fmt, struct
> printf_spec *spec)
> >             fmt.str++;
> >         }
> >         fmt.str++;
> > -        p = lookup_state + *fmt.str;
> > +        p = lookup_state + (u8)*fmt.str;
> >     }
> >     if (p->state) {
> >         if (p->base)
>
> This makes sense. Even though the current code is safe as pointed
> out by Andy.
>
> Reviewed-by: Petr Mladek
>
> Best Regards,
> Petr


Yeah, better safe than sorry in my opinion.


Thanks for the review petr!

Re: [PATCH 0/4] lib/vsprintf: assorted bug fixes

[Andy Shevchenko]

On Tue, Mar 24, 2026 at 10:49:36PM +0000, Josh Law wrote:
> Four small fixes found during an audit of lib/vsprintf.c:
> 
> 1. bstr_printf() fails to advance the args pointer past a
>    pre-rendered pointer string when the output buffer is full,
>    corrupting all subsequent output.
> 
> 2. vbin_printf() writes end[-1] unconditionally when NUL-terminating
>    a pointer string, which is an OOB write when size is zero.
> 
> 3. vsscanf() uses s16 for field_width but assigns from skip_atoi()
>    which returns int, silently truncating large widths to negative
>    and aborting parsing.
> 
> 4. format_decode() is missing a (u8) cast on the second lookup into
>    the format_state table, allowing a negative array index on
>    signed-char platforms.

These all needs a good review. And I think binary printf() might have
a bit different rules on how to propagate the pointer in the buffer.
To me these might fix something or might break something or do nothing
(like in patch 4) due to lack of expertise in the area.

So, I am skeptical about accepting that series, sorry. But I leave it
to others to decide, not giving any tag here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] lib/vsprintf: assorted bug fixes

[Josh Law]

On 25 March 2026 12:05:24 GMT, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>On Tue, Mar 24, 2026 at 10:49:36PM +0000, Josh Law wrote:
>> Four small fixes found during an audit of lib/vsprintf.c:
>> 
>> 1. bstr_printf() fails to advance the args pointer past a
>>    pre-rendered pointer string when the output buffer is full,
>>    corrupting all subsequent output.
>> 
>> 2. vbin_printf() writes end[-1] unconditionally when NUL-terminating
>>    a pointer string, which is an OOB write when size is zero.
>> 
>> 3. vsscanf() uses s16 for field_width but assigns from skip_atoi()
>>    which returns int, silently truncating large widths to negative
>>    and aborting parsing.
>> 
>> 4. format_decode() is missing a (u8) cast on the second lookup into
>>    the format_state table, allowing a negative array index on
>>    signed-char platforms.
>
>These all needs a good review. And I think binary printf() might have
>a bit different rules on how to propagate the pointer in the buffer.
>To me these might fix something or might break something or do nothing
>(like in patch 4) due to lack of expertise in the area.
>
>So, I am skeptical about accepting that series, sorry. But I leave it
>to others to decide, not giving any tag here.
>



Yep! That's absolutely fine! If you would like any patches dropped, just tell me and I'll do it!

I hope I improved vsprintf with these patches, seriously haha


V/R


Josh Law