[PATCH v4] lib/bootconfig: guard xbc_node_compose_key_after() buffer size

[PATCH v4] lib/bootconfig: guard xbc_node_compose_key_after() buffer size

[Josh Law]

xbc_node_compose_key_after() passes a size_t buffer length to
snprintf(), but snprintf() returns int. Guard against size values above
INT_MAX before the loop so the existing truncation check can continue to
compare ret against (int)size safely.

Add a small WARN_ON_ONCE shim for the tools/bootconfig userspace build
so the same source continues to build there.

Signed-off-by: Josh Law <objecting@objecting.org>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v3:
 - Moved the revision history below the --- separator so it does not
   become part of the git commit.
 - Added Reviewed-by from Steven Rostedt.

Changes since v2:
 - Added a comment explaining the INT_MAX guard.

Changes since v1:
 - Removed casting ret to size_t; with the INT_MAX guard, the existing
   ret >= (int)size check is sufficient, per Steven Rostedt.
 - Link to v1:
   https://lore.kernel.org/all/20260317173703.46092-1-objecting@objecting.org/

 lib/bootconfig.c                            | 8 ++++++++
 tools/bootconfig/include/linux/bootconfig.h | 5 +++++
 2 files changed, 13 insertions(+)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 96cbe6738ffe..2a54b51dec5c 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -313,6 +313,14 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
 	if (!node && root)
 		return -EINVAL;
 
+	/*
+	 * Bootconfig strings never need multi-GB buffers. Reject sizes
+	 * above INT_MAX so snprintf()'s int return value cannot overflow
+	 * the truncation check below.
+	 */
+	if (WARN_ON_ONCE(size > INT_MAX))
+		return -EINVAL;
+
 	while (--depth >= 0) {
 		node = xbc_nodes + keys[depth];
 		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
diff --git a/tools/bootconfig/include/linux/bootconfig.h b/tools/bootconfig/include/linux/bootconfig.h
index 6784296a0692..48383c10e036 100644
--- a/tools/bootconfig/include/linux/bootconfig.h
+++ b/tools/bootconfig/include/linux/bootconfig.h
@@ -8,6 +8,7 @@
 #include <stdbool.h>
 #include <ctype.h>
 #include <errno.h>
+#include <limits.h>
 #include <string.h>
 
 
@@ -19,6 +20,10 @@
 	((cond) ? printf("Internal warning(%s:%d, %s): %s\n",	\
 			__FILE__, __LINE__, __func__, #cond) : 0)
 
+#ifndef WARN_ON_ONCE
+#define WARN_ON_ONCE(cond)	WARN_ON(cond)
+#endif
+
 #define unlikely(cond)	(cond)
 
 /* Copied from lib/string.c */
-- 
2.34.1

Re: [PATCH v4] lib/bootconfig: guard xbc_node_compose_key_after() buffer size

[Masami Hiramatsu]

On Tue, 17 Mar 2026 20:44:03 +0000
Josh Law <objecting@objecting.org> wrote:

> xbc_node_compose_key_after() passes a size_t buffer length to
> snprintf(), but snprintf() returns int. Guard against size values above
> INT_MAX before the loop so the existing truncation check can continue to
> compare ret against (int)size safely.
> 
> Add a small WARN_ON_ONCE shim for the tools/bootconfig userspace build
> so the same source continues to build there.

NACK.

Don't do such over engineering effort.

Thanks,

> 
> Signed-off-by: Josh Law <objecting@objecting.org>
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v3:
>  - Moved the revision history below the --- separator so it does not
>    become part of the git commit.
>  - Added Reviewed-by from Steven Rostedt.
> 
> Changes since v2:
>  - Added a comment explaining the INT_MAX guard.
> 
> Changes since v1:
>  - Removed casting ret to size_t; with the INT_MAX guard, the existing
>    ret >= (int)size check is sufficient, per Steven Rostedt.
>  - Link to v1:
>    https://lore.kernel.org/all/20260317173703.46092-1-objecting@objecting.org/
> 
>  lib/bootconfig.c                            | 8 ++++++++
>  tools/bootconfig/include/linux/bootconfig.h | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index 96cbe6738ffe..2a54b51dec5c 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -313,6 +313,14 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>  	if (!node && root)
>  		return -EINVAL;
>  
> +	/*
> +	 * Bootconfig strings never need multi-GB buffers. Reject sizes
> +	 * above INT_MAX so snprintf()'s int return value cannot overflow
> +	 * the truncation check below.
> +	 */
> +	if (WARN_ON_ONCE(size > INT_MAX))
> +		return -EINVAL;
> +
>  	while (--depth >= 0) {
>  		node = xbc_nodes + keys[depth];
>  		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
> diff --git a/tools/bootconfig/include/linux/bootconfig.h b/tools/bootconfig/include/linux/bootconfig.h
> index 6784296a0692..48383c10e036 100644
> --- a/tools/bootconfig/include/linux/bootconfig.h
> +++ b/tools/bootconfig/include/linux/bootconfig.h
> @@ -8,6 +8,7 @@
>  #include <stdbool.h>
>  #include <ctype.h>
>  #include <errno.h>
> +#include <limits.h>
>  #include <string.h>
>  
>  
> @@ -19,6 +20,10 @@
>  	((cond) ? printf("Internal warning(%s:%d, %s): %s\n",	\
>  			__FILE__, __LINE__, __func__, #cond) : 0)
>  
> +#ifndef WARN_ON_ONCE
> +#define WARN_ON_ONCE(cond)	WARN_ON(cond)
> +#endif
> +
>  #define unlikely(cond)	(cond)
>  
>  /* Copied from lib/string.c */
> -- 
> 2.34.1
> 


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

Re: [PATCH v4] lib/bootconfig: guard xbc_node_compose_key_after() buffer size

[Steven Rostedt]

On Wed, 18 Mar 2026 08:03:51 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Tue, 17 Mar 2026 20:44:03 +0000
> Josh Law <objecting@objecting.org> wrote:
> 
> > xbc_node_compose_key_after() passes a size_t buffer length to
> > snprintf(), but snprintf() returns int. Guard against size values above
> > INT_MAX before the loop so the existing truncation check can continue to
> > compare ret against (int)size safely.
> > 
> > Add a small WARN_ON_ONCE shim for the tools/bootconfig userspace build
> > so the same source continues to build there.  
> 
> NACK.
> 
> Don't do such over engineering effort.

Hi Masami,

This was somewhat my idea. Why do you think it's over engineering?

This is your code, so you have final say. I'm not going to push it. I'm
just curious to your thoughts.

It is interesting that snprintf() takes a size_t size, and the iterator
inside is also size_t, but then it returns the value as an int.

That itself just looks wrong (and has nothing to do with your code).

-- Steve

Re: [PATCH v4] lib/bootconfig: guard xbc_node_compose_key_after() buffer size

[Masami Hiramatsu]

On Tue, 17 Mar 2026 19:16:26 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 18 Mar 2026 08:03:51 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Tue, 17 Mar 2026 20:44:03 +0000
> > Josh Law <objecting@objecting.org> wrote:
> > 
> > > xbc_node_compose_key_after() passes a size_t buffer length to
> > > snprintf(), but snprintf() returns int. Guard against size values above
> > > INT_MAX before the loop so the existing truncation check can continue to
> > > compare ret against (int)size safely.
> > > 
> > > Add a small WARN_ON_ONCE shim for the tools/bootconfig userspace build
> > > so the same source continues to build there.  
> > 
> > NACK.
> > 
> > Don't do such over engineering effort.
> 
> Hi Masami,
> 
> This was somewhat my idea. Why do you think it's over engineering?
> 
> This is your code, so you have final say. I'm not going to push it. I'm
> just curious to your thoughts.

I sent a mail why I thought this is over engineering. I think this
comes from vsnprintf() interface design. If all user of that needs
to do this, that is not fair. It should be checked in vsnprintf()
and caller should just check the returned error.

> 
> It is interesting that snprintf() takes a size_t size, and the iterator
> inside is also size_t, but then it returns the value as an int.

Yes, that is checked in vsnprintf(), not its caller.
I think linux kernel should ensure the the return value is smaller
than INT_MAX, and return -EOVERFLOW if not.

Thank you,

> 
> That itself just looks wrong (and has nothing to do with your code).
> 
> -- Steve


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

Re: [PATCH v4] lib/bootconfig: guard xbc_node_compose_key_after() buffer size

[Steven Rostedt]

On Wed, 18 Mar 2026 09:02:43 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Yes, that is checked in vsnprintf(), not its caller.
> I think linux kernel should ensure the the return value is smaller
> than INT_MAX, and return -EOVERFLOW if not.

Well, there's very few places that could have a buffer size of > 2G.

What's the max bootconfig limit? Could you create a bootconfig that is
greater than 2G?

If not, then yeah, we shouldn't really care about overflows (and that
includes not worrying about typecasting the size variable to int).

-- Steve

Re: [PATCH v4] lib/bootconfig: guard xbc_node_compose_key_after() buffer size

[Masami Hiramatsu]

On Tue, 17 Mar 2026 20:43:27 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 18 Mar 2026 09:02:43 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > Yes, that is checked in vsnprintf(), not its caller.
> > I think linux kernel should ensure the the return value is smaller
> > than INT_MAX, and return -EOVERFLOW if not.
> 
> Well, there's very few places that could have a buffer size of > 2G.
> 
> What's the max bootconfig limit? Could you create a bootconfig that is
> greater than 2G?

It's just 32KB. So we don't need it.
Anyway, I sent a patch about that. 

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

Thank you,

> 
> If not, then yeah, we shouldn't really care about overflows (and that
> includes not worrying about typecasting the size variable to int).
> 
> -- Steve
> 


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

Re: [PATCH v4] lib/bootconfig: guard xbc_node_compose_key_after() buffer size

[Steven Rostedt]

On Wed, 18 Mar 2026 09:02:43 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > This was somewhat my idea. Why do you think it's over engineering?
> > 
> > This is your code, so you have final say. I'm not going to push it. I'm
> > just curious to your thoughts.  
> 
> I sent a mail why I thought this is over engineering. I think this
> comes from vsnprintf() interface design. If all user of that needs
> to do this, that is not fair. It should be checked in vsnprintf()
> and caller should just check the returned error.

I wouldn't call this over-engineering. The reason you gave is more about
the checks being simply in the inappropriate location.

Over-engineering is if the patch had created 5 different macros to see if
the value passed to snprintf() was size_t and could be greater than MAX_INT,
and it used the trick of TRACE_EVENT() to create the code to do those
checks. Now THAT would be over-engineering! ;-)

-- Steve