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

[David Laight <david.laight.linux@gmail.com>]

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/
> >   
> 
>