lib/bootconfig.c | 8 ++++++++ tools/bootconfig/include/linux/bootconfig.h | 5 +++++ 2 files changed, 13 insertions(+)
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.1On 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>
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
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>
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
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
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>
© 2016 - 2026 Red Hat, Inc.