Re: [PATCH v4 1/2] lib/vsprintf: Fix to check field_width and precision

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

On Wed, 25 Mar 2026 11:29:22 +0000
David Laight <david.laight.linux@gmail.com> wrote:

> On Wed, 25 Mar 2026 11:22:47 +0100
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > On Wed 2026-03-25 11:25:16, Masami Hiramatsu (Google) wrote:  
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > Check the field_width and presition correctly. Previously it depends
> > > on the bitfield conversion from int to check out-of-range error.
> > > However, commit 938df695e98d ("vsprintf: associate the format state
> > > with the format pointer") changed those fields to int.
> > > We need to check the out-of-range correctly without bitfield
> > > conversion.
> > > 
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -2679,9 +2679,6 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
> > >  
> > >  	/* we finished early by reading the precision */
> > >  	if (unlikely(fmt.state == FORMAT_STATE_PRECISION)) {
> > > -		if (spec->precision < 0)
> > > -			spec->precision = 0;    
> > 
> > This changes the existing kernel behavior and breaks the existing
> > KUnit test in lib/tests/printf_kunit.c:
> > 
> > static void
> > test_string(struct kunit *kunittest)
> > {
> > [...]
> > 	/*
> > 	 * POSIX and C99 say that a negative precision (which is only
> > 	 * possible to pass via a * argument) should be treated as if
> > 	 * the precision wasn't present, and that if the precision is
> > 	 * omitted (as in %.s), the precision should be taken to be
> > 	 * 0. However, the kernel's printf behave exactly opposite,
> > 	 * treating a negative precision as 0 and treating an omitted
> > 	 * precision specifier as if no precision was given.  
> 
> Ugg...

The only format string matches for '".*%[-+ #0]*[0-9]*\.[a-z].*"' are in
printf_kuint.c
There are some "%*.s" lurking, most are outputting "" or " " for alignment,
the '.' can/should be removed, but truncating " " to "" makes no difference.
(Well, it might change one pad space to none...)
That leaves three "%*.s" in diagnostic printk() in dx_show_leaf() in
fs/ext4/namei.c - all should be "%.*s" anyway.
So "%.s" can safely be changed to be the same as "%.0s".

Changing "%.d" from being "%d" to "%.0d" only affects the conversion of zero.
But I didn't find any.

It is harder to check the ("%.*s" len, str) cases for a possible negative len.
Only really because of the shear number, most are 'namelen, name'.
I guess a script/program to convert ("%.*s", prec, ptr) to ("%.*s", FMT_PREC(prec), ptr)
then get the compiler to error !statically_true(prec >= 0) and look at
what it finds.
That should reduce the 700+ cases to a manageable number.

	David


> 
> 	David
> 
> > 	 *
> > 	 * These test cases document the current behaviour; should
> > 	 * anyone ever feel the need to follow the standards more
> > 	 * closely, this can be revisited.
> > 	 */
> > 	test("    ", "%4.*s", -5, "123456");
> > [...]
> > }
> > 
> > The output is:
> > 
> > [   86.234405]     # test_string: EXPECTATION FAILED at lib/tests/printf_kunit.c:56
> >                lib/tests/printf_kunit.c:208: vsnprintf(buf, 256, "%4.*s", ...) returned 6, expected 4
> > [   86.237524]     # test_string: EXPECTATION FAILED at lib/tests/printf_kunit.c:56
> >                lib/tests/printf_kunit.c:208: vsnprintf(buf, 2, "%4.*s", ...) returned 6, expected 4
> > [   86.237542]     # test_string: EXPECTATION FAILED at lib/tests/printf_kunit.c:56
> >                lib/tests/printf_kunit.c:208: vsnprintf(buf, 0, "%4.*s", ...) returned 6, expected 4
> > [   86.237559]     # test_string: EXPECTATION FAILED at lib/tests/printf_kunit.c:141
> >                lib/tests/printf_kunit.c:208: kvasprintf(..., "%4.*s", ...) returned '123456', expected '    '
> > 
> > Do we really want to change the existing behavior?
> > Would it break any existing kernel caller?
> > 
> > I would personally keep the existing behavior unless anyone checks
> > the existing callers.
> >   
> > > -
> > >  		fmt.state = FORMAT_STATE_NONE;
> > >  		goto qualifier;
> > >  	}
> > > @@ -2802,19 +2799,17 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
> > >  static void
> > >  set_field_width(struct printf_spec *spec, int width)
> > >  {
> > > -	spec->field_width = width;
> > > -	if (WARN_ONCE(spec->field_width != width, "field width %d too large", width)) {
> > > -		spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX);
> > > -	}
> > > +	spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX);
> > > +	WARN_ONCE(spec->field_width != width, "field width %d out of range",
> > > +		  width);
> > >  }
> > >  
> > >  static void
> > >  set_precision(struct printf_spec *spec, int prec)
> > >  {
> > > -	spec->precision = prec;
> > > -	if (WARN_ONCE(spec->precision != prec, "precision %d too large", prec)) {
> > > -		spec->precision = clamp(prec, 0, PRECISION_MAX);
> > > -	}
> > > +	/* We allow negative precision, but treat it as if there was no precision. */
> > > +	spec->precision = clamp(prec, -1, PRECISION_MAX);    
> > 
> > And I would keep clamp(prec, 0, PRECISION_MAX) unless anyone checks
> > that changing the existing behavior does not break existing
> > callers.
> >   
> > > +	WARN_ONCE(spec->precision < prec, "precision %d too large", prec);
> > >  }    
> > 
> > Best Regards,
> > Petr  
>