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

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

[Masami Hiramatsu (Google)]

Hi,

Here is the 5th 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/177440550682.147866.1854734911195480940.stgit@devnote2/

In this version, negative precision is treated as zero to match the
previous behavior and check the field/precision passed as string
literals too[1/2]. Also, update bstr_printf() not to return negative
value[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 |   54 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

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

[PATCH v5 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 v5:
  - Drop negative precision support.
  - The field and precision passed as string literals are also checked.
 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 |   39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 800b8ac49f53..2f6b7179f581 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2640,6 +2640,21 @@ static unsigned char spec_flag(unsigned char c)
 	return (c < sizeof(spec_flag_array)) ? spec_flag_array[c] : 0;
 }
 
+static void
+set_field_width(struct printf_spec *spec, int width)
+{
+	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 = clamp(prec, 0, PRECISION_MAX);
+	WARN_ONCE(spec->precision < prec, "precision %d too large", prec);
+}
+
 /*
  * Helper function to decode printf style format.
  * Each call decode a token from the format and return the
@@ -2710,7 +2725,7 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
 	spec->field_width = -1;
 
 	if (isdigit(*fmt.str))
-		spec->field_width = skip_atoi(&fmt.str);
+		set_field_width(spec, skip_atoi(&fmt.str));
 	else if (unlikely(*fmt.str == '*')) {
 		/* it's the next argument */
 		fmt.state = FORMAT_STATE_WIDTH;
@@ -2724,9 +2739,7 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
 	if (unlikely(*fmt.str == '.')) {
 		fmt.str++;
 		if (isdigit(*fmt.str)) {
-			spec->precision = skip_atoi(&fmt.str);
-			if (spec->precision < 0)
-				spec->precision = 0;
+			set_precision(spec, skip_atoi(&fmt.str));
 		} else if (*fmt.str == '*') {
 			/* it's the next argument */
 			fmt.state = FORMAT_STATE_PRECISION;
@@ -2799,24 +2812,6 @@ struct fmt format_decode(struct fmt fmt, struct printf_spec *spec)
 	return fmt;
 }
 
-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);
-	}
-}
-
-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);
-	}
-}
-
 /*
  * Turn a 1/2/4-byte value into a 64-bit one for printing: truncate
  * as necessary and deal with signedness.

[PATCH v5 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() and bstr_printf() 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>
---
 Changes in v5:
  - Fix bstr_printf() too.
 Changes in v4:
  - Add Petr's reviewed-by. (Thanks!)
 Changes in v3:
  - Use local variable for better readability.
---
 lib/vsprintf.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 2f6b7179f581..69dec9b18428 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);
 
@@ -3283,6 +3288,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt_str, const u32 *bin_buf)
 	struct printf_spec spec = {0};
 	char *str, *end;
 	const char *args = (const char *)bin_buf;
+	size_t ret_size;
 
 	if (WARN_ON_ONCE(size > INT_MAX))
 		return 0;
@@ -3431,7 +3437,12 @@ int bstr_printf(char *buf, size_t size, const char *fmt_str, const u32 *bin_buf)
 #undef get_arg
 
 	/* 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_GPL(bstr_printf);
 

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

[Masami Hiramatsu]

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

> Hi,
> 
> Here is the 5th 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/177440550682.147866.1854734911195480940.stgit@devnote2/
> 
> In this version, negative precision is treated as zero to match the
> previous behavior and check the field/precision passed as string
> literals too[1/2]. Also, update bstr_printf() not to return negative
> value[2/2].
> 

BTW, skip_atoi() is used for converting precision and width,
but this does not check the overflow. This is expected to be
checked by compiler (-Wformat-overflow) but it checks the
width <= INT_MAX, but precision <= LONG_MAX (why?) and clang
does not check precision.

To avoid this issue, below fix is needed, but I'm not sure
this is meaningful check, because with [1/2] change, the
return value is limited anyway, and it's easy to check
during the review process if an obviously abnormal
precision value is passed in the format string.

Thanks,

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 69dec9b18428..8846d3a960dc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -187,10 +187,20 @@ static inline int skip_atoi(const char **s)
 	int i = 0;
 
 	do {
-		i = i*10 + *((*s)++) - '0';
+		int next = *((*s)++) - '0';
+		if (unlikely(i > INT_MAX / 10U ||
+			     (i == INT_MAX / 10U && next > INT_MAX % 10U))) {
+			goto overflow;
+		}
+		i = i*10 + next;
 	} while (isdigit(**s));
 
 	return i;
+
+overflow:
+	while (isdigit(**s))
+		(*s)++;
+	return INT_MAX;
 }
 
 /*



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

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

[Andy Shevchenko]

On Wed, Mar 25, 2026 at 10:41:58PM +0900, Masami Hiramatsu wrote:
> On Wed, 25 Mar 2026 22:27:31 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > Hi,
> > 
> > Here is the 5th 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/177440550682.147866.1854734911195480940.stgit@devnote2/
> > 
> > In this version, negative precision is treated as zero to match the
> > previous behavior and check the field/precision passed as string
> > literals too[1/2]. Also, update bstr_printf() not to return negative
> > value[2/2].

> BTW, skip_atoi() is used for converting precision and width,
> but this does not check the overflow. This is expected to be
> checked by compiler (-Wformat-overflow) but it checks the
> width <= INT_MAX, but precision <= LONG_MAX (why?) and clang
> does not check precision.
> 
> To avoid this issue, below fix is needed, but I'm not sure
> this is meaningful check, because with [1/2] change, the
> return value is limited anyway, and it's easy to check
> during the review process if an obviously abnormal
> precision value is passed in the format string.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c

I you event want to do that, it should use macros from overflow.h,
also see how kstrto*() and memparse() perform such checks. Also
this may slow down the conversion.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Mar 25, 2026 at 10:27:41PM +0900, Masami Hiramatsu (Google) wrote:

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

...

> +static void
> +set_field_width(struct printf_spec *spec, int width)

One line.

static void set_field_width(struct printf_spec *spec, int width)

> +{
> +	spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX);
> +	WARN_ONCE(spec->field_width != width, "field width %d out of range",
> +		  width);

I would also make it one line for readability.

	WARN_ONCE(spec->field_width != width, "field width %d out of range", width);

> +}
> +
> +static void
> +set_precision(struct printf_spec *spec, int prec)

One line

static void set_precision(struct printf_spec *spec, int prec)

> +{
> +	spec->precision = clamp(prec, 0, PRECISION_MAX);
> +	WARN_ONCE(spec->precision < prec, "precision %d too large", prec);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Masami Hiramatsu]

On Thu, 26 Mar 2026 11:54:27 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Mar 25, 2026 at 10:41:58PM +0900, Masami Hiramatsu wrote:
> > On Wed, 25 Mar 2026 22:27:31 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > 
> > > Hi,
> > > 
> > > Here is the 5th 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/177440550682.147866.1854734911195480940.stgit@devnote2/
> > > 
> > > In this version, negative precision is treated as zero to match the
> > > previous behavior and check the field/precision passed as string
> > > literals too[1/2]. Also, update bstr_printf() not to return negative
> > > value[2/2].
> 
> > BTW, skip_atoi() is used for converting precision and width,
> > but this does not check the overflow. This is expected to be
> > checked by compiler (-Wformat-overflow) but it checks the
> > width <= INT_MAX, but precision <= LONG_MAX (why?) and clang
> > does not check precision.
> > 
> > To avoid this issue, below fix is needed, but I'm not sure
> > this is meaningful check, because with [1/2] change, the
> > return value is limited anyway, and it's easy to check
> > during the review process if an obviously abnormal
> > precision value is passed in the format string.
> 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> 
> I you event want to do that, it should use macros from overflow.h,
> also see how kstrto*() and memparse() perform such checks. Also
> this may slow down the conversion.

Agreed, I don't want to push it. Since this overflow currently
only happens on precision and only by string literals, I think
it is better to be checked by review process.

Thank you,

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 


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

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

[Masami Hiramatsu]

On Thu, 26 Mar 2026 11:57:46 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Mar 25, 2026 at 10:27:41PM +0900, Masami Hiramatsu (Google) wrote:
> 
> > 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.
> 
> ...
> 
> > +static void
> > +set_field_width(struct printf_spec *spec, int width)
> 
> One line.
> 

Ah, OK.

> static void set_field_width(struct printf_spec *spec, int width)
> 
> > +{
> > +	spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX);
> > +	WARN_ONCE(spec->field_width != width, "field width %d out of range",
> > +		  width);
> 
> I would also make it one line for readability.
> 
> 	WARN_ONCE(spec->field_width != width, "field width %d out of range", width);

OK.

> 
> > +}
> > +
> > +static void
> > +set_precision(struct printf_spec *spec, int prec)
> 
> One line
> 
> static void set_precision(struct printf_spec *spec, int prec)

OK, let me update it.

Thanks for review!

> 
> > +{
> > +	spec->precision = clamp(prec, 0, PRECISION_MAX);
> > +	WARN_ONCE(spec->precision < prec, "precision %d too large", prec);
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 


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