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

Petr Mladek
2026-03-31T16:12:22Z

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