[PATCH v4 0/2] lib/vsprintf: Fixes size check

[PATCH v4 0/2] lib/vsprintf: Fixes size check

[Masami Hiramatsu (Google)]

Hi,

Here is the 4th version of patches to fix vsnprintf().

 - Fix to limit the size of width and precision.
 - Warn if the return size is over INT_MAX.

Previous version is here;

https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/

In this version, do clamp() the width and precision before checking it and
accept negative precision[1/3] and add Petr's Reviewed-by[2/2].

Thank you,

---

Masami Hiramatsu (Google) (2):
      lib/vsprintf: Fix to check field_width and precision
      lib/vsprintf: Limit the returning size to INT_MAX


 lib/vsprintf.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Masami Hiramatsu (Google)]

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.

Fixes: 938df695e98d ("vsprintf: associate the format state with the format pointer")
Reported-by: David Laight <david.laight.linux@gmail.com>
Closes: https://lore.kernel.org/all/20260318151250.40fef0ab@pumpkin/
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v4:
  - Do clamp() first.
  - Accept negative precision (this means no precision) .
  - Change the warning message for width.
 Changes in v3:
  - Check and update width and precision before assigning to spec.
 Changes in v2:
  - Fix to use logical split.
---
 lib/vsprintf.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 800b8ac49f53..5fa8f69030be 100644
--- 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;
-
 		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);
+	WARN_ONCE(spec->precision < prec, "precision %d too large", prec);
 }
 
 /*

[PATCH v4 2/2] lib/vsprintf: Limit the returning size to INT_MAX

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

The return value of vsnprintf() can overflow INT_MAX and return
a minus value. In the @size is checked input overflow, but it does
not check the output, which is expected required size.

This should never happen but it should be checked and limited.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 Changes in v4:
  - Add Petr's reviewed-by. (Thanks!)
 Changes in v3:
  - Use local variable for better readability.
---
 lib/vsprintf.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 5fa8f69030be..351b6f8e4796 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2859,6 +2859,7 @@ static unsigned long long convert_num_spec(unsigned int val, int size, struct pr
 int vsnprintf(char *buf, size_t size, const char *fmt_str, va_list args)
 {
 	char *str, *end;
+	size_t ret_size;
 	struct printf_spec spec = {0};
 	struct fmt fmt = {
 		.str = fmt_str,
@@ -2978,8 +2979,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt_str, va_list args)
 	}
 
 	/* the trailing null byte doesn't count towards the total */
-	return str-buf;
+	ret_size = str - buf;
 
+	/* Make sure the return value is within the positive integer range */
+	if (WARN_ON_ONCE(ret_size > INT_MAX))
+		ret_size = INT_MAX;
+	return ret_size;
 }
 EXPORT_SYMBOL(vsnprintf);
 

Re: [PATCH v4 0/2] lib/vsprintf: Fixes size check

[Andrew Morton]

On Wed, 25 Mar 2026 11:25:06 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> Here is the 4th version of patches to fix vsnprintf().
> 
>  - Fix to limit the size of width and precision.
>  - Warn if the return size is over INT_MAX.
> 
> Previous version is here;
> 
> https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/
> 
> In this version, do clamp() the width and precision before checking it and
> accept negative precision[1/3] and add Petr's Reviewed-by[2/2].

AI review has flagged a couple of possible issues:
	https://sashiko.dev/#/patchset/177440550682.147866.1854734911195480940.stgit@devnote2

Re: [PATCH v4 0/2] lib/vsprintf: Fixes size check

[Masami Hiramatsu]

On Tue, 24 Mar 2026 22:04:58 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 25 Mar 2026 11:25:06 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > Here is the 4th version of patches to fix vsnprintf().
> > 
> >  - Fix to limit the size of width and precision.
> >  - Warn if the return size is over INT_MAX.
> > 
> > Previous version is here;
> > 
> > https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/
> > 
> > In this version, do clamp() the width and precision before checking it and
> > accept negative precision[1/3] and add Petr's Reviewed-by[2/2].
> 
> AI review has flagged a couple of possible issues:
> 	https://sashiko.dev/#/patchset/177440550682.147866.1854734911195480940.stgit@devnote2

OK, I think I should decouple accept negative precision part from [1/2] since
it is changing the kernel specific behavior.

Thanks, 

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[David Laight]

On Wed, 25 Mar 2026 11:25:16 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> 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.
> 
> Fixes: 938df695e98d ("vsprintf: associate the format state with the format pointer")
> Reported-by: David Laight <david.laight.linux@gmail.com>
> Closes: https://lore.kernel.org/all/20260318151250.40fef0ab@pumpkin/
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

I've just read the code - my god is it complicated and confusing.
But I think it looks ok, so:
Reviewed-by: David Laight <david.laight.linux@gmail.com>

Maybe I'll find to time (haha) to rewrite it based on the nolibc version.
It'll be a lot smaller and a lot faster.
I also suspect that corner cases like ("%#.0o", 0) (where the '#' needs to
add a leading zero to the empty string) aren't right.
But that is different from this fix.

	David


> ---
>  Changes in v4:
>   - Do clamp() first.
>   - Accept negative precision (this means no precision) .
>   - Change the warning message for width.
>  Changes in v3:
>   - Check and update width and precision before assigning to spec.
>  Changes in v2:
>   - Fix to use logical split.
> ---
>  lib/vsprintf.c |   17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 800b8ac49f53..5fa8f69030be 100644
> --- 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;
> -
>  		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);
> +	WARN_ONCE(spec->precision < prec, "precision %d too large", prec);
>  }
>  
>  /*
> 

Re: [PATCH v4 0/2] lib/vsprintf: Fixes size check

[David Laight]

On Tue, 24 Mar 2026 22:04:58 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 25 Mar 2026 11:25:06 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > Here is the 4th version of patches to fix vsnprintf().
> > 
> >  - Fix to limit the size of width and precision.
> >  - Warn if the return size is over INT_MAX.
> > 
> > Previous version is here;
> > 
> > https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/
> > 
> > In this version, do clamp() the width and precision before checking it and
> > accept negative precision[1/3] and add Petr's Reviewed-by[2/2].  
> 
> AI review has flagged a couple of possible issues:
> 	https://sashiko.dev/#/patchset/177440550682.147866.1854734911195480940.stgit@devnote2

I'd guess there are exactly 0 places where a negative precision is passed
to "%.*s" - if there were any someone would have complained about the
output being missing.
Checking all 759 cases grep -r '".*%.*\.%*s.*"' found will be tedious.
But pretty much all are 'namelen'.

In any case worst thing should be a panic if the code hits an invalid
address before finding a '\0' byte - probably unlikely anyway.

I'd fix it, but try to stop it being backported.

	David

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

[Petr Mladek]

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.
	 *
	 * 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

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

[David Laight]

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

	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

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

[Masami Hiramatsu]

On Wed, 25 Mar 2026 11:25:16 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> 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.
> 

Hmm, I also found that width/precision passed as string literals
also missed the range check.

Thanks,

> Fixes: 938df695e98d ("vsprintf: associate the format state with the format pointer")
> Reported-by: David Laight <david.laight.linux@gmail.com>
> Closes: https://lore.kernel.org/all/20260318151250.40fef0ab@pumpkin/
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v4:
>   - Do clamp() first.
>   - Accept negative precision (this means no precision) .
>   - Change the warning message for width.
>  Changes in v3:
>   - Check and update width and precision before assigning to spec.
>  Changes in v2:
>   - Fix to use logical split.
> ---
>  lib/vsprintf.c |   17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 800b8ac49f53..5fa8f69030be 100644
> --- 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;
> -
>  		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);
> +	WARN_ONCE(spec->precision < prec, "precision %d too large", prec);
>  }
>  
>  /*
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Masami Hiramatsu]

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.
> 	 *
> 	 * These test cases document the current behaviour; should
> 	 * anyone ever feel the need to follow the standards more
> 	 * closely, this can be revisited.
> 	 */

Yeah, I also found this comment. So v5 drops the negative precision
support.


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

Of course no.

> Would it break any existing kernel caller?

it is possible. Anyway to update the behavior, we also need to update
the test case.

> 
> I would personally keep the existing behavior unless anyone checks
> the existing callers.

OK.

Thanks,

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


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[David Laight]

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  
> 

Re: [PATCH v4 0/2] lib/vsprintf: Fixes size check

[Masami Hiramatsu]

On Wed, 25 Mar 2026 10:20:39 +0000
David Laight <david.laight.linux@gmail.com> wrote:

> On Tue, 24 Mar 2026 22:04:58 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Wed, 25 Mar 2026 11:25:06 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > 
> > > Here is the 4th version of patches to fix vsnprintf().
> > > 
> > >  - Fix to limit the size of width and precision.
> > >  - Warn if the return size is over INT_MAX.
> > > 
> > > Previous version is here;
> > > 
> > > https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/
> > > 
> > > In this version, do clamp() the width and precision before checking it and
> > > accept negative precision[1/3] and add Petr's Reviewed-by[2/2].  
> > 
> > AI review has flagged a couple of possible issues:
> > 	https://sashiko.dev/#/patchset/177440550682.147866.1854734911195480940.stgit@devnote2
> 
> I'd guess there are exactly 0 places where a negative precision is passed
> to "%.*s" - if there were any someone would have complained about the
> output being missing.
> Checking all 759 cases grep -r '".*%.*\.%*s.*"' found will be tedious.
> But pretty much all are 'namelen'.

I also verified and found only one suspicious usage which can pass
a negative precision.

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index d2c7b1090df0..1f90775ea8a8 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2224,7 +2224,7 @@ void i915_request_show(struct drm_printer *m,
 	rcu_read_lock();
 	timeline = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
 	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
-		   prefix, indent, "                ",
+		   prefix, max(0, indent), "                ",
 		   queue_status(rq),
 		   rq->fence.context, rq->fence.seqno,
 		   run_status(rq),

Thanks,
> 
> In any case worst thing should be a panic if the code hits an invalid
> address before finding a '\0' byte - probably unlikely anyway.
> 
> I'd fix it, but try to stop it being backported.
> 
> 	David


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v4 0/2] lib/vsprintf: Fixes size check

[David Laight]

On Thu, 26 Mar 2026 16:39:44 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Wed, 25 Mar 2026 10:20:39 +0000
> David Laight <david.laight.linux@gmail.com> wrote:
> 
> > On Tue, 24 Mar 2026 22:04:58 -0700
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> >   
> > > On Wed, 25 Mar 2026 11:25:06 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > >   
> > > > Here is the 4th version of patches to fix vsnprintf().
> > > > 
> > > >  - Fix to limit the size of width and precision.
> > > >  - Warn if the return size is over INT_MAX.
> > > > 
> > > > Previous version is here;
> > > > 
> > > > https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/
> > > > 
> > > > In this version, do clamp() the width and precision before checking it and
> > > > accept negative precision[1/3] and add Petr's Reviewed-by[2/2].    
> > > 
> > > AI review has flagged a couple of possible issues:
> > > 	https://sashiko.dev/#/patchset/177440550682.147866.1854734911195480940.stgit@devnote2  
> > 
> > I'd guess there are exactly 0 places where a negative precision is passed
> > to "%.*s" - if there were any someone would have complained about the
> > output being missing.
> > Checking all 759 cases grep -r '".*%.*\.%*s.*"' found will be tedious.
> > But pretty much all are 'namelen'.  
> 
> I also verified and found only one suspicious usage which can pass
> a negative precision.

It is always called with a constant, in any case the string being output
is constant so nothing nasty can happen.
I didn't even see any recursive/loop calls that indent by significant amounts.
The code could use the more usual ("%*s", indent, ""), but it doesn't matter
much - mostly just a shorter line.

	David

> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index d2c7b1090df0..1f90775ea8a8 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -2224,7 +2224,7 @@ void i915_request_show(struct drm_printer *m,
>  	rcu_read_lock();
>  	timeline = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
>  	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
> -		   prefix, indent, "                ",
> +		   prefix, max(0, indent), "                ",
>  		   queue_status(rq),
>  		   rq->fence.context, rq->fence.seqno,
>  		   run_status(rq),
> 
> Thanks,
> > 
> > In any case worst thing should be a panic if the code hits an invalid
> > address before finding a '\0' byte - probably unlikely anyway.
> > 
> > I'd fix it, but try to stop it being backported.
> > 
> > 	David  
> 
> 

Re: [PATCH v4 0/2] lib/vsprintf: Fixes size check

[Masami Hiramatsu]

On Thu, 26 Mar 2026 09:12:12 +0000
David Laight <david.laight.linux@gmail.com> wrote:

> On Thu, 26 Mar 2026 16:39:44 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Wed, 25 Mar 2026 10:20:39 +0000
> > David Laight <david.laight.linux@gmail.com> wrote:
> > 
> > > On Tue, 24 Mar 2026 22:04:58 -0700
> > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > >   
> > > > On Wed, 25 Mar 2026 11:25:06 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > > >   
> > > > > Here is the 4th version of patches to fix vsnprintf().
> > > > > 
> > > > >  - Fix to limit the size of width and precision.
> > > > >  - Warn if the return size is over INT_MAX.
> > > > > 
> > > > > Previous version is here;
> > > > > 
> > > > > https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/
> > > > > 
> > > > > In this version, do clamp() the width and precision before checking it and
> > > > > accept negative precision[1/3] and add Petr's Reviewed-by[2/2].    
> > > > 
> > > > AI review has flagged a couple of possible issues:
> > > > 	https://sashiko.dev/#/patchset/177440550682.147866.1854734911195480940.stgit@devnote2  
> > > 
> > > I'd guess there are exactly 0 places where a negative precision is passed
> > > to "%.*s" - if there were any someone would have complained about the
> > > output being missing.
> > > Checking all 759 cases grep -r '".*%.*\.%*s.*"' found will be tedious.
> > > But pretty much all are 'namelen'.  
> > 
> > I also verified and found only one suspicious usage which can pass
> > a negative precision.
> 
> It is always called with a constant, in any case the string being output
> is constant so nothing nasty can happen.

Since this function is not a static function, it can be called anywhere in
the module. So it is safer to check the indent is positive.

> I didn't even see any recursive/loop calls that indent by significant amounts.
> The code could use the more usual ("%*s", indent, ""), but it doesn't matter
> much - mostly just a shorter line.

I think the current code looks a bit tricky and not clear what it does.
Maybe it is better to replace it with above usual usage.

Thanks,

> 
> 	David
> 
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index d2c7b1090df0..1f90775ea8a8 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -2224,7 +2224,7 @@ void i915_request_show(struct drm_printer *m,
> >  	rcu_read_lock();
> >  	timeline = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
> >  	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
> > -		   prefix, indent, "                ",
> > +		   prefix, max(0, indent), "                ",
> >  		   queue_status(rq),
> >  		   rq->fence.context, rq->fence.seqno,
> >  		   run_status(rq),
> > 
> > Thanks,
> > > 
> > > In any case worst thing should be a panic if the code hits an invalid
> > > address before finding a '\0' byte - probably unlikely anyway.
> > > 
> > > I'd fix it, but try to stop it being backported.
> > > 
> > > 	David  
> > 
> > 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v4 0/2] lib/vsprintf: Fixes size check

[David Laight]

On Fri, 27 Mar 2026 16:28:12 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Thu, 26 Mar 2026 09:12:12 +0000
> David Laight <david.laight.linux@gmail.com> wrote:
> 
> > On Thu, 26 Mar 2026 16:39:44 +0900
> > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> >   
> > > On Wed, 25 Mar 2026 10:20:39 +0000
> > > David Laight <david.laight.linux@gmail.com> wrote:
> > >   
> > > > On Tue, 24 Mar 2026 22:04:58 -0700
> > > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > >     
> > > > > On Wed, 25 Mar 2026 11:25:06 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > > > >     
> > > > > > Here is the 4th version of patches to fix vsnprintf().
> > > > > > 
> > > > > >  - Fix to limit the size of width and precision.
> > > > > >  - Warn if the return size is over INT_MAX.
> > > > > > 
> > > > > > Previous version is here;
> > > > > > 
> > > > > > https://lore.kernel.org/all/177410406326.38798.16853803119128725972.stgit@devnote2/
> > > > > > 
> > > > > > In this version, do clamp() the width and precision before checking it and
> > > > > > accept negative precision[1/3] and add Petr's Reviewed-by[2/2].      
> > > > > 
> > > > > AI review has flagged a couple of possible issues:
> > > > > 	https://sashiko.dev/#/patchset/177440550682.147866.1854734911195480940.stgit@devnote2    
> > > > 
> > > > I'd guess there are exactly 0 places where a negative precision is passed
> > > > to "%.*s" - if there were any someone would have complained about the
> > > > output being missing.
> > > > Checking all 759 cases grep -r '".*%.*\.%*s.*"' found will be tedious.
> > > > But pretty much all are 'namelen'.    
> > > 
> > > I also verified and found only one suspicious usage which can pass
> > > a negative precision.  
> > 
> > It is always called with a constant, in any case the string being output
> > is constant so nothing nasty can happen.  
> 
> Since this function is not a static function, it can be called anywhere in
> the module. So it is safer to check the indent is positive.

That would have to be an out or tree module using a function that looks
pretty specific to the i915 display code.
Even as someone who has supported out of tree drivers I'd say that they
get what they deserve.
The snprintf() code itself won't break anything.

	David
 
> 
> > I didn't even see any recursive/loop calls that indent by significant amounts.
> > The code could use the more usual ("%*s", indent, ""), but it doesn't matter
> > much - mostly just a shorter line.  
> 
> I think the current code looks a bit tricky and not clear what it does.
> Maybe it is better to replace it with above usual usage.
> 
> Thanks,
> 
> > 
> > 	David
> >   
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > > index d2c7b1090df0..1f90775ea8a8 100644
> > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > @@ -2224,7 +2224,7 @@ void i915_request_show(struct drm_printer *m,
> > >  	rcu_read_lock();
> > >  	timeline = dma_fence_timeline_name((struct dma_fence *)&rq->fence);
> > >  	drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n",
> > > -		   prefix, indent, "                ",
> > > +		   prefix, max(0, indent), "                ",
> > >  		   queue_status(rq),
> > >  		   rq->fence.context, rq->fence.seqno,
> > >  		   run_status(rq),
> > > 
> > > Thanks,  
> > > > 
> > > > In any case worst thing should be a panic if the code hits an invalid
> > > > address before finding a '\0' byte - probably unlikely anyway.
> > > > 
> > > > I'd fix it, but try to stop it being backported.
> > > > 
> > > > 	David    
> > > 
> > >   
> >   
> 
>