[PATCH 0/3] bootconfig: Reject unexpected data after null character and cleanups

[PATCH 0/3] bootconfig: Reject unexpected data after null character and cleanups

[Masami Hiramatsu (Google)]

Hi,

Here are patches for bootconfig to rejects unexpected config data after null
character and other cleanups including tools/bootconfig to consolidate
bootconfig initialization with errors, and skipping internal tree sanity
check in kernel.

The last one is initially reported by Sang-Heon Jeon [1].

[1] https://lore.kernel.org/all/20260905141637.1547429-1-ekffu200098@gmail.com/

Thank you,

---
base-commit: 13e8b7707efcea3a401d242958ba0a392f227847

Masami Hiramatsu (Google) (3):
      bootconfig: Reject unexpected data after null character
      tools/bootconfig: Consolidate xbc_init() to error message wrapper
      bootconfig: Skip internal tree sanity checks in kernel


 lib/bootconfig.c                    |   45 ++++++++++----
 tools/bootconfig/main.c             |  112 +++++++++++++++++------------------
 tools/bootconfig/test-bootconfig.sh |   12 ++++
 3 files changed, 99 insertions(+), 70 deletions(-)

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

[PATCH 1/3] bootconfig: Reject unexpected data after null character

[Masami Hiramatsu (Google)]

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

If a bootconfig buffer contains an intermediate null character in the
middle of the configuration, xbc_parse_tree() stops at the null character
because string delimiter searches (e.g. strpbrk()) stop at '\0', and
cleanly breaks out of the loop without error. As a result, any
configuration data following the intermediate null character is silently
ignored, allowing unparsed or potentially malicious data to be hidden
after an early termination.

Fix this in xbc_parse_tree() by checking that no non-null data remains
between the parser termination point and the end of the input buffer.
Trailing null characters (such as alignment padding in initrd) continue
to be accepted as valid.

Also update apply_xbc() in tools/bootconfig/main.c to calculate the
buffer size based on the loaded file size rather than strlen(), so that
files with intermediate null characters are not truncated before
validation.

Assisted-by: Antigravity:gemini-3.8-flash
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 lib/bootconfig.c                    |    7 +++++++
 tools/bootconfig/main.c             |    4 +++-
 tools/bootconfig/test-bootconfig.sh |   12 ++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index aba11caf6903..0ec2874db9c7 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -1116,6 +1116,13 @@ static int __init xbc_parse_tree(void)
 		}
 	} while (!ret);
 
+	if (!ret) {
+		while (p < xbc_data + xbc_data_size - 1 && *p == '\0')
+			p++;
+		if (p < xbc_data + xbc_data_size - 1)
+			ret = xbc_parse_error("Unexpected data after null character", p);
+	}
+
 	return ret;
 }
 
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 7dc9fff9b637..6035404733c3 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -422,7 +422,9 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		pr_err("Failed to load %s : %d\n", xbc_path, ret);
 		return ret;
 	}
-	size = strlen(buf) + 1;
+	size = ret;
+	if (size == 0 || buf[size - 1] != '\0')
+		size++;
 	csum = xbc_calc_checksum(buf, size);
 
 	/* Backup the bootconfig data */
diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index fc69f815ce4a..530ce7e28d63 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -180,6 +180,18 @@ EOF
 $BOOTCONF -a $TEMPCONF $INITRD 2> $OUTFILE
 xpass grep -q "1:1" $OUTFILE
 
+echo "Intermediate null character test"
+printf "key = value\n\0extra = data\n" > $TEMPCONF
+xfail $BOOTCONF -a $TEMPCONF $INITRD
+$BOOTCONF -a $TEMPCONF $INITRD 2> $OUTFILE
+xpass grep -q "Unexpected" $OUTFILE
+
+echo "Trailing null character test"
+printf "key = value\n\0" > $TEMPCONF
+xpass $BOOTCONF -a $TEMPCONF $INITRD
+$BOOTCONF $INITRD > $OUTFILE
+xpass grep -q "value" $OUTFILE
+
 echo "=== expected failure cases ==="
 for i in samples/bad-* ; do
   xfail $BOOTCONF -a $i $INITRD

[PATCH 2/3] tools/bootconfig: Consolidate xbc_init() to error message wrapper

[Masami Hiramatsu (Google)]

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

Use init_xbc_with_error() for all bootconfig initialization in the
bootconfig tool instead of showing errors in different way.

This simplifies the code logic and make it easy to maintain.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 tools/bootconfig/main.c |  108 ++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 57 deletions(-)

diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 6035404733c3..7117aa9b2a83 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -21,6 +21,47 @@
 #define BOOTCONFIG_FOOTER_SIZE	\
 	(sizeof(uint32_t) * 2 + BOOTCONFIG_MAGIC_LEN)
 
+static void show_xbc_error(const char *data, const char *msg, int pos)
+{
+	int lin = 1, col, i;
+
+	if (pos < 0) {
+		pr_err("Error: %s.\n", msg);
+		return;
+	}
+
+	/* Note that pos starts from 0 but lin and col should start from 1. */
+	col = pos + 1;
+	for (i = 0; i < pos; i++) {
+		if (data[i] == '\n') {
+			lin++;
+			col = pos - i;
+		}
+	}
+	pr_err("Parse Error: %s at %d:%d\n", msg, lin, col);
+
+}
+
+static int init_xbc_with_error(char *buf, int len)
+{
+	char *copy = malloc(len);
+	const char *msg;
+	int ret, pos;
+
+	if (!copy)
+		return -ENOMEM;
+
+	memcpy(copy, buf, len);
+	/* We do not terminate the copy with \0 for sanity checking */
+
+	ret = xbc_init(buf, len, &msg, &pos);
+	if (ret < 0)
+		show_xbc_error(copy, msg, pos);
+	free(copy);
+
+	return ret;
+}
+
 static int xbc_show_value(struct xbc_node *node, bool semicolon)
 {
 	const char *val, *eol;
@@ -187,7 +228,6 @@ static int load_xbc_from_initrd(int fd, char **buf)
 	int ret;
 	uint32_t size = 0, csum = 0, rcsum;
 	char magic[BOOTCONFIG_MAGIC_LEN];
-	const char *msg;
 
 	ret = fstat(fd, &stat);
 	if (ret < 0)
@@ -238,52 +278,9 @@ static int load_xbc_from_initrd(int fd, char **buf)
 		return -EINVAL;
 	}
 
-	ret = xbc_init(*buf, size, &msg, NULL);
-	/* Wrong data */
-	if (ret < 0) {
-		pr_err("parse error: %s.\n", msg);
-		return ret;
-	}
+	ret = init_xbc_with_error(*buf, size);
 
-	return size;
-}
-
-static void show_xbc_error(const char *data, const char *msg, int pos)
-{
-	int lin = 1, col, i;
-
-	if (pos < 0) {
-		pr_err("Error: %s.\n", msg);
-		return;
-	}
-
-	/* Note that pos starts from 0 but lin and col should start from 1. */
-	col = pos + 1;
-	for (i = 0; i < pos; i++) {
-		if (data[i] == '\n') {
-			lin++;
-			col = pos - i;
-		}
-	}
-	pr_err("Parse Error: %s at %d:%d\n", msg, lin, col);
-
-}
-
-static int init_xbc_with_error(char *buf, int len)
-{
-	char *copy = strdup(buf);
-	const char *msg;
-	int ret, pos;
-
-	if (!copy)
-		return -ENOMEM;
-
-	ret = xbc_init(buf, len, &msg, &pos);
-	if (ret < 0)
-		show_xbc_error(copy, msg, pos);
-	free(copy);
-
-	return ret;
+	return ret < 0 ? ret : size;
 }
 
 static int show_xbc_kernel_cmdline(void)
@@ -412,9 +409,8 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	char *buf, *data;
 	size_t total_size;
 	struct stat stat;
-	const char *msg;
 	uint32_t size, csum;
-	int pos, pad;
+	int pad;
 	int ret, fd;
 
 	ret = load_xbc_file(xbc_path, &buf);
@@ -427,6 +423,13 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		size++;
 	csum = xbc_calc_checksum(buf, size);
 
+	/* Verify the data format */
+	ret = init_xbc_with_error(buf, size);
+	if (ret < 0) {
+		free(buf);
+		return ret;
+	}
+
 	/* Backup the bootconfig data */
 	data = calloc(size + BOOTCONFIG_ALIGN + BOOTCONFIG_FOOTER_SIZE, 1);
 	if (!data) {
@@ -435,15 +438,6 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	}
 	memcpy(data, buf, size);
 
-	/* Check the data format */
-	ret = xbc_init(buf, size, &msg, &pos);
-	if (ret < 0) {
-		show_xbc_error(data, msg, pos);
-		free(data);
-		free(buf);
-
-		return ret;
-	}
 	printf("Apply %s to %s\n", xbc_path, path);
 	xbc_get_info(&ret, NULL);
 	printf("\tNumber of nodes: %d\n", ret);

[PATCH 3/3] bootconfig: Skip internal tree sanity checks in kernel

[Masami Hiramatsu (Google)]

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

In xbc_verify_tree(), the loop iterating through all nodes to check that
xbc_nodes[i].next < xbc_node_num and xbc_nodes[i].child < xbc_node_num
is a defensive sanity check against implementation regressions (such
an out-of-bounds index cannot be produced by malformed input).

Running this check in the kernel adds unnecessary boot-time overhead.
Split this check out into xbc_sanity_check_tree() for userspace, so
that it continues to run during userspace bootconfig validation (e.g.
when applying or testing bootconfig with tools/bootconfig), but is
omitted in the kernel to speed up initialization.

Reported-by: Sang-Heon Jeon <ekffu200098@gmail.com>
Closes: https://lore.kernel.org/all/20260905141637.1547429-1-ekffu200098@gmail.com/
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 lib/bootconfig.c |   38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 0ec2874db9c7..884f186b1989 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -1000,9 +1000,30 @@ static int __init xbc_close_brace(char **k, char *n)
 	return __xbc_close_brace(n - 1);
 }
 
+#ifndef __KERNEL__
+/* Sanity check for regression: node indices must be within bounds */
+static int __init xbc_sanity_check_tree(void)
+{
+	int i;
+
+	for (i = 0; i < xbc_node_num; i++) {
+		if (xbc_nodes[i].next >= xbc_node_num) {
+			return xbc_parse_error("No closing brace",
+				xbc_node_get_data(xbc_nodes + i));
+		}
+		if (xbc_nodes[i].child >= xbc_node_num) {
+			return xbc_parse_error("Broken child node",
+				xbc_node_get_data(xbc_nodes + i));
+		}
+	}
+
+	return 0;
+}
+#endif
+
 static int __init xbc_verify_tree(void)
 {
-	int i, depth;
+	int depth;
 	size_t len, wlen;
 	struct xbc_node *n, *m;
 
@@ -1019,17 +1040,6 @@ static int __init xbc_verify_tree(void)
 		return -ENOENT;
 	}
 
-	for (i = 0; i < xbc_node_num; i++) {
-		if (xbc_nodes[i].next >= xbc_node_num) {
-			return xbc_parse_error("No closing brace",
-				xbc_node_get_data(xbc_nodes + i));
-		}
-		if (xbc_nodes[i].child >= xbc_node_num) {
-			return xbc_parse_error("Broken child node",
-				xbc_node_get_data(xbc_nodes + i));
-		}
-	}
-
 	/* Key tree limitation check */
 	n = &xbc_nodes[0];
 	depth = 1;
@@ -1199,6 +1209,10 @@ int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
 	ret = xbc_parse_tree();
 	if (!ret)
 		ret = xbc_verify_tree();
+#ifndef __KERNEL__
+	if (!ret)
+		ret = xbc_sanity_check_tree();
+#endif
 
 	if (ret < 0) {
 		if (epos)

Re: [PATCH 2/3] tools/bootconfig: Consolidate xbc_init() to error message wrapper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer overflow in initrd bootconfig size calculation leading to a heap buffer overflow.
--

commit 51beb61f7aa059b732b7d7560c90f8155150ccd7
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

tools/bootconfig: Consolidate xbc_init() to error message wrapper

Use init_xbc_with_error() for all bootconfig initialization in the
bootconfig tool instead of showing errors in different way.

This simplifies the code logic and make it easy to maintain.

> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> index 6035404733c38..7117aa9b2a839 100644
> --- a/tools/bootconfig/main.c
> +++ b/tools/bootconfig/main.c
[ ... ]
> @@ -238,52 +278,9 @@ static int load_xbc_from_initrd(int fd, char **buf)
>  		return -EINVAL;
>  	}
>  
> -	ret = xbc_init(*buf, size, &msg, NULL);
> -	/* Wrong data */
> -	if (ret < 0) {
> -		pr_err("parse error: %s.\n", msg);
> -		return ret;
> -	}
> +	ret = init_xbc_with_error(*buf, size);

[Severity: High]
This is a pre-existing issue, but is it possible for a 32-bit integer overflow
to bypass the size validation check in load_xbc_from_initrd()?

In load_xbc_from_initrd(), size is read from the file as a uint32_t:

tools/bootconfig/main.c:load_xbc_from_initrd() {
    ...
    if (read(fd, &size, sizeof(uint32_t)) < 0)
        return pr_errno("Failed to read size", -errno);
    size = le32toh(size);
    ...
    if (stat.st_size < size + BOOTCONFIG_FOOTER_SIZE) {
        pr_err("bootconfig size is too big\n");
        return -E2BIG;
    }
    ...
}

On 32-bit systems, if an attacker crafts size such that adding
BOOTCONFIG_FOOTER_SIZE (20) wraps around (for instance, if size is 0xFFFFFFFF),
the stat.st_size < size + BOOTCONFIG_FOOTER_SIZE check can be bypassed.

This would lead to load_xbc_fd() being called with size evaluating to -1:

tools/bootconfig/main.c:load_xbc_fd() {
    ...
    *buf = malloc(size + 1);
    if (!*buf)
        return -ENOMEM;

    ret = read(fd, *buf, size);
    if (ret < 0)
        return -errno;
    (*buf)[size] = '\0';
    ...
}

The malloc(size + 1) would result in a 0-byte allocation, while the
read(fd, *buf, size) would cast the negative size to size_t (0xFFFFFFFF) and
overflow the chunk. Finally, (*buf)[size] = '\0' would write to a negative
index (-1), which corrupts heap metadata.

Could we make this size check more robust against integer overflow?

>  
> -	return size;
> -}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178896921555.177508.434402948295885560.stgit@devnote2?part=2

Re: [PATCH 2/3] tools/bootconfig: Consolidate xbc_init() to error message wrapper

[Masami Hiramatsu]

On Wed, 09 Sep 2026 16:11:12 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Integer overflow in initrd bootconfig size calculation leading to a heap buffer overflow.
> --
> 
> commit 51beb61f7aa059b732b7d7560c90f8155150ccd7
> Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> tools/bootconfig: Consolidate xbc_init() to error message wrapper
> 
> Use init_xbc_with_error() for all bootconfig initialization in the
> bootconfig tool instead of showing errors in different way.
> 
> This simplifies the code logic and make it easy to maintain.
> 
> > diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> > index 6035404733c38..7117aa9b2a839 100644
> > --- a/tools/bootconfig/main.c
> > +++ b/tools/bootconfig/main.c
> [ ... ]
> > @@ -238,52 +278,9 @@ static int load_xbc_from_initrd(int fd, char **buf)
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = xbc_init(*buf, size, &msg, NULL);
> > -	/* Wrong data */
> > -	if (ret < 0) {
> > -		pr_err("parse error: %s.\n", msg);
> > -		return ret;
> > -	}
> > +	ret = init_xbc_with_error(*buf, size);
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for a 32-bit integer overflow
> to bypass the size validation check in load_xbc_from_initrd()?

Oh, I never thought I could shoot myself in the foot like that.

Anyway, since bootconfig file size should be smaller than 32KB,
so we can check the file size before opening it.

> 
> In load_xbc_from_initrd(), size is read from the file as a uint32_t:
> 
> tools/bootconfig/main.c:load_xbc_from_initrd() {
>     ...
>     if (read(fd, &size, sizeof(uint32_t)) < 0)
>         return pr_errno("Failed to read size", -errno);
>     size = le32toh(size);
>     ...
>     if (stat.st_size < size + BOOTCONFIG_FOOTER_SIZE) {
>         pr_err("bootconfig size is too big\n");
>         return -E2BIG;
>     }
>     ...
> }
> 
> On 32-bit systems, if an attacker crafts size such that adding
> BOOTCONFIG_FOOTER_SIZE (20) wraps around (for instance, if size is 0xFFFFFFFF),
> the stat.st_size < size + BOOTCONFIG_FOOTER_SIZE check can be bypassed.
> 
> This would lead to load_xbc_fd() being called with size evaluating to -1:
> 
> tools/bootconfig/main.c:load_xbc_fd() {
>     ...
>     *buf = malloc(size + 1);
>     if (!*buf)
>         return -ENOMEM;
> 
>     ret = read(fd, *buf, size);
>     if (ret < 0)
>         return -errno;
>     (*buf)[size] = '\0';
>     ...
> }
> 
> The malloc(size + 1) would result in a 0-byte allocation, while the
> read(fd, *buf, size) would cast the negative size to size_t (0xFFFFFFFF) and
> overflow the chunk. Finally, (*buf)[size] = '\0' would write to a negative
> index (-1), which corrupts heap metadata.

Good catch!

> 
> Could we make this size check more robust against integer overflow?

 OK, let me fix it too.

Thanks!


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