[PATCH v7 00/15] bootconfig: fixes, cleanups, and modernization

[PATCH v7 00/15] bootconfig: fixes, cleanups, and modernization

[Josh Law]

This series addresses a collection of issues found during a review of
lib/bootconfig.c, include/linux/bootconfig.h, and tools/bootconfig,
ranging from off-by-one errors and unchecked return values to coding
style, signedness/type cleanup, and API modernization.

Changes since v6:
  - Dropped "add missing __init annotations to static helpers"
    (v6 patch 1).
  - Dropped "fix sign-compare in xbc_node_compose_key_after()"
    (v6 patch 16).
  - Updated "fix fd leak in load_xbc_file() on fstat failure" to save
    errno before close(), since close() may overwrite it before the
    error is returned (patch 10).
  - Updated "fix signed comparison in xbc_node_get_data()" to use
    size_t for the local offset variable, matching the warning and
    xbc_data_size (patch 11).
  - Updated "use signed type for offset in xbc_init_node()" to use a
    signed long and explicitly check offset < 0 in WARN_ON(), making
    the pre-base pointer case explicit instead of relying on unsigned
    wraparound (patch 13).

Changes since v5:
  - Folded typo fixes, kerneldoc blank line, and inconsistent bracing
    patches (v5 02-05, 07) into a single patch (patch 1).
  - Dropped "use __packed macro for struct xbc_node" (v5 11) and
    "add __packed definition to tools/bootconfig shim header" (v5 14)
    per review feedback.
  - Added Fixes: tag to "check xbc_init_node() return in override
    path" (patch 9).
  - Added Fixes: tag to "fix fd leak in load_xbc_file() on fstat
    failure" (patch 10).

Changes since v4:
  - Added six follow-up patches found via static analysis with strict
    GCC warnings (patches 11-15, plus the now-dropped v6 patch 16).
  - Added "fix signed comparison in xbc_node_get_data()" to match the
    local offset type to xbc_data_size and eliminate the sign-compare
    warning (patch 11).
  - Added "use size_t for strlen result in xbc_node_match_prefix()"
    and "use size_t for key length tracking in xbc_verify_tree()" to
    match strlen() return types (patches 12, 14).
  - Added "use signed type for offset in xbc_init_node()" to make the
    offset bounds check explicit and avoid sign-conversion warnings
    from pointer subtraction (patch 13).
  - Added "change xbc_node_index() return type to uint16_t" to match
    the 16-bit storage fields and XBC_NODE_MAX bounds (patch 15).

Changes since v3:
  - Added commit descriptions to all patches that were missing them.
  - Added real-world impact statements to all bug-fix patches.

Changes since v2:
  - Added "validate child node index in xbc_verify_tree()" (patch 8).
  - Added "check xbc_init_node() return in override path" (patch 9).
  - Added "fix fd leak in load_xbc_file() on fstat failure" (patch 10).

Changes since v1:
  - Dropped "return empty string instead of NULL from
    xbc_node_get_data()" -- returning "" causes false matches in
    xbc_node_match_prefix() because strncmp(..., "", 0) always
    returns 0.

Bug fixes:
  - Fix off-by-one in xbc_verify_tree() where a next-node index equal
    to xbc_node_num passes the bounds check despite being out of range;
    a malformed bootconfig could cause an out-of-bounds read of kernel
    memory during tree traversal at boot time (patch 3).
  - Move xbc_node_num increment to after xbc_init_node() validation so
    a failed init does not leave a partially initialized node counted
    in the array; on a maximum-size bootconfig, the uninitialized node
    could be traversed leading to unpredictable boot behavior (patch 4).
  - Validate child node indices in xbc_verify_tree() alongside the
    existing next-node check; without this, a corrupt bootconfig could
    trigger an out-of-bounds memory access via an invalid child index
    during tree traversal (patch 8).
  - Check xbc_init_node() return value in the ':=' override path; a
    bootconfig using ':=' near the 32KB data limit could silently
    retain the old value, meaning a security-relevant boot parameter
    override would not take effect (patch 9).
  - Fix file descriptor leak in tools/bootconfig load_xbc_file() when
    fstat() fails, and preserve errno across close() on that error path
    (patch 10).

Correctness:
  - Narrow the flag parameter in node creation helpers from uint32_t to
    uint16_t to match the xbc_node.data field width (patch 2).
  - Constify the xbc_calc_checksum() data parameter since it only reads
    the buffer (patch 6).
  - Fix strict-GCC signedness and narrowing warnings by aligning local
    types with strlen() APIs and the node index/data storage in
    xbc_node_get_data(), xbc_node_match_prefix(), xbc_init_node(),
    xbc_verify_tree(), and xbc_node_index() (patches 11-15).

Cleanups:
  - Fix comment typos, missing blank line before kerneldoc, and
    inconsistent if/else bracing (patch 1).
  - Drop redundant memset after memblock_alloc which already returns
    zeroed memory; switch the userspace path from malloc to calloc to
    match (patch 5).

Modernization:
  - Replace the catch-all linux/kernel.h include with the specific
    headers needed: linux/cache.h, linux/compiler.h, and
    linux/sprintf.h (patch 7).

Build-tested with both the in-kernel build (lib/bootconfig.o,
init/main.o) and the userspace tools/bootconfig build. All 70
tools/bootconfig test cases pass.

Josh Law (15):
  lib/bootconfig: clean up comment typos and bracing
  lib/bootconfig: narrow flag parameter type from uint32_t to uint16_t
  lib/bootconfig: fix off-by-one in xbc_verify_tree() next node check
  lib/bootconfig: increment xbc_node_num after node init succeeds
  lib/bootconfig: drop redundant memset of xbc_nodes
  bootconfig: constify xbc_calc_checksum() data parameter
  lib/bootconfig: replace linux/kernel.h with specific includes
  lib/bootconfig: validate child node index in xbc_verify_tree()
  lib/bootconfig: check xbc_init_node() return in override path
  tools/bootconfig: fix fd leak in load_xbc_file() on fstat failure
  lib/bootconfig: fix signed comparison in xbc_node_get_data()
  lib/bootconfig: use size_t for strlen result in
    xbc_node_match_prefix()
  lib/bootconfig: use signed type for offset in xbc_init_node()
  lib/bootconfig: use size_t for key length tracking in
    xbc_verify_tree()
  lib/bootconfig: change xbc_node_index() return type to uint16_t

 include/linux/bootconfig.h |  6 ++--
 lib/bootconfig.c           | 65 ++++++++++++++++++++++----------------
 tools/bootconfig/main.c    |  7 ++--
 3 files changed, 46 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH v7 01/15] lib/bootconfig: clean up comment typos and bracing

[Josh Law]

Fixes kerneldoc typos ("initiized" and "uder") and adds a missing blank line.
Also fixes inconsistent if/else bracing in __xbc_add_key() and elsewhere.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index b0ef1e74e98a..d77b6533ac00 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -79,6 +79,7 @@ static inline void xbc_free_mem(void *addr, size_t size, bool early)
 	free(addr);
 }
 #endif
+
 /**
  * xbc_get_info() - Get the information of loaded boot config
  * @node_size: A pointer to store the number of nodes.
@@ -112,7 +113,7 @@ static int __init xbc_parse_error(const char *msg, const char *p)
  * xbc_root_node() - Get the root node of extended boot config
  *
  * Return the address of root node of extended boot config. If the
- * extended boot config is not initiized, return NULL.
+ * extended boot config is not initialized, return NULL.
  */
 struct xbc_node * __init xbc_root_node(void)
 {
@@ -364,7 +365,7 @@ struct xbc_node * __init xbc_node_find_next_leaf(struct xbc_node *root,
 			node = xbc_node_get_parent(node);
 			if (node == root)
 				return NULL;
-			/* User passed a node which is not uder parent */
+			/* User passed a node which is not under parent */
 			if (WARN_ON(!node))
 				return NULL;
 		}
@@ -472,8 +473,9 @@ static struct xbc_node * __init __xbc_add_sibling(char *data, uint32_t flag, boo
 				sib->next = xbc_node_index(node);
 			}
 		}
-	} else
+	} else {
 		xbc_parse_error("Too many nodes", data);
+	}
 
 	return node;
 }
@@ -655,9 +657,9 @@ static int __init __xbc_add_key(char *k)
 	if (unlikely(xbc_node_num == 0))
 		goto add_node;
 
-	if (!last_parent)	/* the first level */
+	if (!last_parent) {	/* the first level */
 		node = find_match_node(xbc_nodes, k);
-	else {
+	} else {
 		child = xbc_node_get_child(last_parent);
 		/* Since the value node is the first child, skip it. */
 		if (child && xbc_node_is_value(child))
@@ -665,9 +667,9 @@ static int __init __xbc_add_key(char *k)
 		node = find_match_node(child, k);
 	}
 
-	if (node)
+	if (node) {
 		last_parent = node;
-	else {
+	} else {
 add_node:
 		node = xbc_add_child(k, XBC_KEY);
 		if (!node)
@@ -991,8 +993,9 @@ int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
 		if (emsg)
 			*emsg = xbc_err_msg;
 		_xbc_exit(true);
-	} else
+	} else {
 		ret = xbc_node_num;
+	}
 
 	return ret;
 }
-- 
2.34.1

[PATCH v7 02/15] lib/bootconfig: narrow flag parameter type from uint32_t to uint16_t

[Josh Law]

The flag parameter in the node creation helpers only ever carries
XBC_KEY (0) or XBC_VALUE (0x8000), both of which fit in uint16_t.
Using uint16_t matches the width of xbc_node.data where the flag is
ultimately stored.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index d77b6533ac00..d69ec95d6062 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -408,7 +408,7 @@ const char * __init xbc_node_find_next_key_value(struct xbc_node *root,
 
 /* XBC parse and tree build */
 
-static int __init xbc_init_node(struct xbc_node *node, char *data, uint32_t flag)
+static int __init xbc_init_node(struct xbc_node *node, char *data, uint16_t flag)
 {
 	unsigned long offset = data - xbc_data;
 
@@ -422,7 +422,7 @@ static int __init xbc_init_node(struct xbc_node *node, char *data, uint32_t flag
 	return 0;
 }
 
-static struct xbc_node * __init xbc_add_node(char *data, uint32_t flag)
+static struct xbc_node * __init xbc_add_node(char *data, uint16_t flag)
 {
 	struct xbc_node *node;
 
@@ -452,7 +452,7 @@ static inline __init struct xbc_node *xbc_last_child(struct xbc_node *node)
 	return node;
 }
 
-static struct xbc_node * __init __xbc_add_sibling(char *data, uint32_t flag, bool head)
+static struct xbc_node * __init __xbc_add_sibling(char *data, uint16_t flag, bool head)
 {
 	struct xbc_node *sib, *node = xbc_add_node(data, flag);
 
@@ -480,17 +480,17 @@ static struct xbc_node * __init __xbc_add_sibling(char *data, uint32_t flag, boo
 	return node;
 }
 
-static inline struct xbc_node * __init xbc_add_sibling(char *data, uint32_t flag)
+static inline struct xbc_node * __init xbc_add_sibling(char *data, uint16_t flag)
 {
 	return __xbc_add_sibling(data, flag, false);
 }
 
-static inline struct xbc_node * __init xbc_add_head_sibling(char *data, uint32_t flag)
+static inline struct xbc_node * __init xbc_add_head_sibling(char *data, uint16_t flag)
 {
 	return __xbc_add_sibling(data, flag, true);
 }
 
-static inline __init struct xbc_node *xbc_add_child(char *data, uint32_t flag)
+static inline __init struct xbc_node *xbc_add_child(char *data, uint16_t flag)
 {
 	struct xbc_node *node = xbc_add_sibling(data, flag);
 
-- 
2.34.1

[PATCH v7 03/15] lib/bootconfig: fix off-by-one in xbc_verify_tree() next node check

[Josh Law]

Valid node indices are 0 to xbc_node_num-1, so a next value equal to
xbc_node_num is out of bounds.  Use >= instead of > to catch this.

A malformed or corrupt bootconfig could pass tree verification with
an out-of-bounds next index.  On subsequent tree traversal at boot
time, xbc_node_get_next() would return a pointer past the allocated
xbc_nodes array, causing an out-of-bounds read of kernel memory.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index d69ec95d6062..ca668ead1db6 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -816,7 +816,7 @@ static int __init xbc_verify_tree(void)
 	}
 
 	for (i = 0; i < xbc_node_num; i++) {
-		if (xbc_nodes[i].next > xbc_node_num) {
+		if (xbc_nodes[i].next >= xbc_node_num) {
 			return xbc_parse_error("No closing brace",
 				xbc_node_get_data(xbc_nodes + i));
 		}
-- 
2.34.1

[PATCH v7 04/15] lib/bootconfig: increment xbc_node_num after node init succeeds

[Josh Law]

Move the xbc_node_num increment to after xbc_init_node() so a failed
init does not leave a partially initialized node counted in the array.

If xbc_init_node() fails on a data offset at the boundary of a
maximum-size bootconfig, the pre-incremented count causes subsequent
tree verification and traversal to consider the uninitialized node as
valid, potentially leading to an out-of-bounds read or unpredictable
boot behavior.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index ca668ead1db6..6b899e24189c 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -429,9 +429,10 @@ static struct xbc_node * __init xbc_add_node(char *data, uint16_t flag)
 	if (xbc_node_num == XBC_NODE_MAX)
 		return NULL;
 
-	node = &xbc_nodes[xbc_node_num++];
+	node = &xbc_nodes[xbc_node_num];
 	if (xbc_init_node(node, data, flag) < 0)
 		return NULL;
+	xbc_node_num++;
 
 	return node;
 }
-- 
2.34.1

[PATCH v7 05/15] lib/bootconfig: drop redundant memset of xbc_nodes

[Josh Law]

memblock_alloc() already returns zeroed memory, so the explicit memset
in xbc_init() is redundant. Switch the userspace xbc_alloc_mem() from
malloc() to calloc() so both paths return zeroed memory, and remove
the separate memset call.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 6b899e24189c..69486bd56fea 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -71,7 +71,7 @@ static inline void __init xbc_free_mem(void *addr, size_t size, bool early)
 
 static inline void *xbc_alloc_mem(size_t size)
 {
-	return malloc(size);
+	return calloc(1, size);
 }
 
 static inline void xbc_free_mem(void *addr, size_t size, bool early)
@@ -982,7 +982,6 @@ int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
 		_xbc_exit(true);
 		return -ENOMEM;
 	}
-	memset(xbc_nodes, 0, sizeof(struct xbc_node) * XBC_NODE_MAX);
 
 	ret = xbc_parse_tree();
 	if (!ret)
-- 
2.34.1

[PATCH v7 06/15] bootconfig: constify xbc_calc_checksum() data parameter

[Josh Law]

xbc_calc_checksum() only reads the data buffer, so mark the parameter
as const void * and the internal pointer as const unsigned char *.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 include/linux/bootconfig.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index 25df9260d206..23a96c5edcf3 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -36,9 +36,9 @@ bool __init cmdline_has_extra_options(void);
  * The checksum will be used with the BOOTCONFIG_MAGIC and the size for
  * embedding the bootconfig in the initrd image.
  */
-static inline __init uint32_t xbc_calc_checksum(void *data, uint32_t size)
+static inline __init uint32_t xbc_calc_checksum(const void *data, uint32_t size)
 {
-	unsigned char *p = data;
+	const unsigned char *p = data;
 	uint32_t ret = 0;
 
 	while (size--)
-- 
2.34.1

[PATCH v7 07/15] lib/bootconfig: replace linux/kernel.h with specific includes

[Josh Law]

linux/kernel.h is a legacy catch-all header. Replace it with the
specific headers actually needed: linux/cache.h for SMP_CACHE_BYTES,
linux/compiler.h for unlikely(), and linux/sprintf.h for snprintf().

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 69486bd56fea..d7e2395b7e83 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -17,7 +17,9 @@
 #include <linux/bug.h>
 #include <linux/ctype.h>
 #include <linux/errno.h>
-#include <linux/kernel.h>
+#include <linux/cache.h>
+#include <linux/compiler.h>
+#include <linux/sprintf.h>
 #include <linux/memblock.h>
 #include <linux/string.h>
 
-- 
2.34.1

[PATCH v7 08/15] lib/bootconfig: validate child node index in xbc_verify_tree()

[Josh Law]

xbc_verify_tree() validates that each node's next index is within
bounds, but does not check the child index.  Add the same bounds
check for the child field.

Without this check, a corrupt bootconfig that passes next-index
validation could still trigger an out-of-bounds memory access via an
invalid child index when xbc_node_get_child() is called during tree
traversal at boot time.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index d7e2395b7e83..1adf592cc038 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -823,6 +823,10 @@ static int __init xbc_verify_tree(void)
 			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 */
-- 
2.34.1

[PATCH v7 09/15] lib/bootconfig: check xbc_init_node() return in override path

[Josh Law]

The ':=' override path in xbc_parse_kv() calls xbc_init_node() to
re-initialize an existing value node but does not check the return
value. If xbc_init_node() fails (data offset out of range), parsing
silently continues with stale node data.

Add the missing error check to match the xbc_add_node() call path
which already checks for failure.

In practice, a bootconfig using ':=' to override a value near the
32KB data limit could silently retain the old value, meaning a
security-relevant boot parameter override (e.g., a trace filter or
debug setting) would not take effect as intended.

Fixes: e5efaeb8a8f5 ("bootconfig: Support mixing a value and subkeys under a key")
Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 1adf592cc038..ecc4e8d93547 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -728,7 +728,8 @@ static int __init xbc_parse_kv(char **k, char *v, int op)
 		if (op == ':') {
 			unsigned short nidx = child->next;
 
-			xbc_init_node(child, v, XBC_VALUE);
+			if (xbc_init_node(child, v, XBC_VALUE) < 0)
+				return xbc_parse_error("Failed to override value", v);
 			child->next = nidx;	/* keep subkeys */
 			goto array;
 		}
-- 
2.34.1

[PATCH v7 10/15] tools/bootconfig: fix fd leak in load_xbc_file() on fstat failure

[Josh Law]

If fstat() fails after open() succeeds, the function returns without
closing the file descriptor. Also preserve errno across close(), since
close() may overwrite it before the error is returned.

Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
Signed-off-by: Josh Law <objecting@objecting.org>
---
 tools/bootconfig/main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 55d59ed507d5..643f707b8f1d 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -162,8 +162,11 @@ static int load_xbc_file(const char *path, char **buf)
 	if (fd < 0)
 		return -errno;
 	ret = fstat(fd, &stat);
-	if (ret < 0)
-		return -errno;
+	if (ret < 0) {
+		ret = -errno;
+		close(fd);
+		return ret;
+	}
 
 	ret = load_xbc_fd(fd, buf, stat.st_size);
 
-- 
2.34.1

[PATCH v7 11/15] lib/bootconfig: fix signed comparison in xbc_node_get_data()

[Josh Law]

  lib/bootconfig.c:188:28: warning: comparison of integer expressions
  of different signedness: 'int' and 'size_t' [-Wsign-compare]

The local variable 'offset' is declared as int, but xbc_data_size is
size_t.  Change the type to size_t to match and eliminate the warning.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index ecc4e8d93547..dd639046912c 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -183,7 +183,7 @@ struct xbc_node * __init xbc_node_get_next(struct xbc_node *node)
  */
 const char * __init xbc_node_get_data(struct xbc_node *node)
 {
-	int offset = node->data & ~XBC_VALUE;
+	size_t offset = node->data & ~XBC_VALUE;
 
 	if (WARN_ON(offset >= xbc_data_size))
 		return NULL;
-- 
2.34.1

[PATCH v7 12/15] lib/bootconfig: use size_t for strlen result in xbc_node_match_prefix()

[Josh Law]

  lib/bootconfig.c:198:19: warning: conversion from 'size_t' to 'int'
  may change value [-Wconversion]
  lib/bootconfig.c:200:33: warning: conversion to '__kernel_size_t'
  from 'int' may change the sign of the result [-Wsign-conversion]

strlen() returns size_t but the result was stored in an int.  The value
is then passed back to strncmp() which expects size_t, causing a second
sign-conversion warning on the round-trip.  Use size_t throughout to
match the API types.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index dd639046912c..3c5b6ad32a54 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -195,7 +195,7 @@ static bool __init
 xbc_node_match_prefix(struct xbc_node *node, const char **prefix)
 {
 	const char *p = xbc_node_get_data(node);
-	int len = strlen(p);
+	size_t len = strlen(p);
 
 	if (strncmp(*prefix, p, len))
 		return false;
-- 
2.34.1

[PATCH v7 13/15] lib/bootconfig: use signed type for offset in xbc_init_node()

[Josh Law]

  lib/bootconfig.c:415:32: warning: conversion to 'long unsigned int'
  from 'long int' may change the sign of the result [-Wsign-conversion]

Pointer subtraction yields ptrdiff_t (signed), which was stored in
unsigned long.  The original unsigned type implicitly caught a negative
offset (data < xbc_data) because the wrapped value would exceed
XBC_DATA_MAX.  Make this intent explicit by using a signed long and
adding an offset < 0 check to the WARN_ON condition.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 3c5b6ad32a54..119c3d429c1f 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -412,9 +412,9 @@ const char * __init xbc_node_find_next_key_value(struct xbc_node *root,
 
 static int __init xbc_init_node(struct xbc_node *node, char *data, uint16_t flag)
 {
-	unsigned long offset = data - xbc_data;
+	long offset = data - xbc_data;
 
-	if (WARN_ON(offset >= XBC_DATA_MAX))
+	if (WARN_ON(offset < 0 || offset >= XBC_DATA_MAX))
 		return -EINVAL;
 
 	node->data = (uint16_t)offset | flag;
-- 
2.34.1

[PATCH v7 14/15] lib/bootconfig: use size_t for key length tracking in xbc_verify_tree()

[Josh Law]

  lib/bootconfig.c:839:24: warning: conversion from 'size_t' to 'int'
  may change value [-Wconversion]
  lib/bootconfig.c:860:32: warning: conversion from 'size_t' to 'int'
  may change value [-Wconversion]
  lib/bootconfig.c:860:29: warning: conversion to 'size_t' from 'int'
  may change the sign of the result [-Wsign-conversion]

The key length variables len and wlen accumulate strlen() results but
were declared as int, causing truncation and sign-conversion warnings.
Change both to size_t to match the strlen() return type and avoid
mixed-sign arithmetic.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 119c3d429c1f..272d9427e879 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -803,7 +803,8 @@ static int __init xbc_close_brace(char **k, char *n)
 
 static int __init xbc_verify_tree(void)
 {
-	int i, depth, len, wlen;
+	int i, depth;
+	size_t len, wlen;
 	struct xbc_node *n, *m;
 
 	/* Brace closing */
-- 
2.34.1

[PATCH v7 15/15] lib/bootconfig: change xbc_node_index() return type to uint16_t

[Josh Law]

  lib/bootconfig.c:136:21: warning: conversion from 'long int' to
  'int' may change value [-Wconversion]
  lib/bootconfig.c:308:33: warning: conversion from 'int' to 'uint16_t'
  may change value [-Wconversion]
  lib/bootconfig.c:467:37: warning: conversion from 'int' to 'uint16_t'
  may change value [-Wconversion]
  lib/bootconfig.c:469:40: warning: conversion from 'int' to 'uint16_t'
  may change value [-Wconversion]
  lib/bootconfig.c:472:54: warning: conversion from 'int' to 'uint16_t'
  may change value [-Wconversion]
  lib/bootconfig.c:476:45: warning: conversion from 'int' to 'uint16_t'
  may change value [-Wconversion]

xbc_node_index() returns the position of a node in the xbc_nodes array,
which has at most XBC_NODE_MAX (8192) entries, well within uint16_t
range.  Every caller stores the result in a uint16_t field (node->parent,
node->child, node->next, or the keys[] array in compose_key_after), so
the int return type causes narrowing warnings at all six call sites.

Change the return type to uint16_t and add an explicit cast on the
pointer subtraction to match the storage width and eliminate the
warnings.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 include/linux/bootconfig.h | 2 +-
 lib/bootconfig.c           | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index 23a96c5edcf3..692a5acc2ffc 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -66,7 +66,7 @@ struct xbc_node {
 
 /* Node tree access raw APIs */
 struct xbc_node * __init xbc_root_node(void);
-int __init xbc_node_index(struct xbc_node *node);
+uint16_t __init xbc_node_index(struct xbc_node *node);
 struct xbc_node * __init xbc_node_get_parent(struct xbc_node *node);
 struct xbc_node * __init xbc_node_get_child(struct xbc_node *node);
 struct xbc_node * __init xbc_node_get_next(struct xbc_node *node);
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 272d9427e879..57f07e33868e 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -131,9 +131,9 @@ struct xbc_node * __init xbc_root_node(void)
  *
  * Return the index number of @node in XBC node list.
  */
-int __init xbc_node_index(struct xbc_node *node)
+uint16_t __init xbc_node_index(struct xbc_node *node)
 {
-	return node - &xbc_nodes[0];
+	return (uint16_t)(node - &xbc_nodes[0]);
 }
 
 /**
-- 
2.34.1

Re: [PATCH v7 00/15] bootconfig: fixes, cleanups, and modernization

[Masami Hiramatsu]

Hi Josh,

It is mostly OK, but can you reorder this series as the fixes (which
has "Fixes" tag, [9/15] and [10/15]) first and others second, so that
I can cleanly merge it to bootconfig/fixes and bootconfig/for-next?

The patches which has Fixes tag should go into stable tree, but other
improvements/cleanups should go to next merge window.

Thank you,


On Tue, 17 Mar 2026 16:09:01 +0000
Josh Law <objecting@objecting.org> wrote:

> This series addresses a collection of issues found during a review of
> lib/bootconfig.c, include/linux/bootconfig.h, and tools/bootconfig,
> ranging from off-by-one errors and unchecked return values to coding
> style, signedness/type cleanup, and API modernization.
> 
> Changes since v6:
>   - Dropped "add missing __init annotations to static helpers"
>     (v6 patch 1).
>   - Dropped "fix sign-compare in xbc_node_compose_key_after()"
>     (v6 patch 16).
>   - Updated "fix fd leak in load_xbc_file() on fstat failure" to save
>     errno before close(), since close() may overwrite it before the
>     error is returned (patch 10).
>   - Updated "fix signed comparison in xbc_node_get_data()" to use
>     size_t for the local offset variable, matching the warning and
>     xbc_data_size (patch 11).
>   - Updated "use signed type for offset in xbc_init_node()" to use a
>     signed long and explicitly check offset < 0 in WARN_ON(), making
>     the pre-base pointer case explicit instead of relying on unsigned
>     wraparound (patch 13).
> 
> Changes since v5:
>   - Folded typo fixes, kerneldoc blank line, and inconsistent bracing
>     patches (v5 02-05, 07) into a single patch (patch 1).
>   - Dropped "use __packed macro for struct xbc_node" (v5 11) and
>     "add __packed definition to tools/bootconfig shim header" (v5 14)
>     per review feedback.
>   - Added Fixes: tag to "check xbc_init_node() return in override
>     path" (patch 9).
>   - Added Fixes: tag to "fix fd leak in load_xbc_file() on fstat
>     failure" (patch 10).
> 
> Changes since v4:
>   - Added six follow-up patches found via static analysis with strict
>     GCC warnings (patches 11-15, plus the now-dropped v6 patch 16).
>   - Added "fix signed comparison in xbc_node_get_data()" to match the
>     local offset type to xbc_data_size and eliminate the sign-compare
>     warning (patch 11).
>   - Added "use size_t for strlen result in xbc_node_match_prefix()"
>     and "use size_t for key length tracking in xbc_verify_tree()" to
>     match strlen() return types (patches 12, 14).
>   - Added "use signed type for offset in xbc_init_node()" to make the
>     offset bounds check explicit and avoid sign-conversion warnings
>     from pointer subtraction (patch 13).
>   - Added "change xbc_node_index() return type to uint16_t" to match
>     the 16-bit storage fields and XBC_NODE_MAX bounds (patch 15).
> 
> Changes since v3:
>   - Added commit descriptions to all patches that were missing them.
>   - Added real-world impact statements to all bug-fix patches.
> 
> Changes since v2:
>   - Added "validate child node index in xbc_verify_tree()" (patch 8).
>   - Added "check xbc_init_node() return in override path" (patch 9).
>   - Added "fix fd leak in load_xbc_file() on fstat failure" (patch 10).
> 
> Changes since v1:
>   - Dropped "return empty string instead of NULL from
>     xbc_node_get_data()" -- returning "" causes false matches in
>     xbc_node_match_prefix() because strncmp(..., "", 0) always
>     returns 0.
> 
> Bug fixes:
>   - Fix off-by-one in xbc_verify_tree() where a next-node index equal
>     to xbc_node_num passes the bounds check despite being out of range;
>     a malformed bootconfig could cause an out-of-bounds read of kernel
>     memory during tree traversal at boot time (patch 3).
>   - Move xbc_node_num increment to after xbc_init_node() validation so
>     a failed init does not leave a partially initialized node counted
>     in the array; on a maximum-size bootconfig, the uninitialized node
>     could be traversed leading to unpredictable boot behavior (patch 4).
>   - Validate child node indices in xbc_verify_tree() alongside the
>     existing next-node check; without this, a corrupt bootconfig could
>     trigger an out-of-bounds memory access via an invalid child index
>     during tree traversal (patch 8).
>   - Check xbc_init_node() return value in the ':=' override path; a
>     bootconfig using ':=' near the 32KB data limit could silently
>     retain the old value, meaning a security-relevant boot parameter
>     override would not take effect (patch 9).
>   - Fix file descriptor leak in tools/bootconfig load_xbc_file() when
>     fstat() fails, and preserve errno across close() on that error path
>     (patch 10).
> 
> Correctness:
>   - Narrow the flag parameter in node creation helpers from uint32_t to
>     uint16_t to match the xbc_node.data field width (patch 2).
>   - Constify the xbc_calc_checksum() data parameter since it only reads
>     the buffer (patch 6).
>   - Fix strict-GCC signedness and narrowing warnings by aligning local
>     types with strlen() APIs and the node index/data storage in
>     xbc_node_get_data(), xbc_node_match_prefix(), xbc_init_node(),
>     xbc_verify_tree(), and xbc_node_index() (patches 11-15).
> 
> Cleanups:
>   - Fix comment typos, missing blank line before kerneldoc, and
>     inconsistent if/else bracing (patch 1).
>   - Drop redundant memset after memblock_alloc which already returns
>     zeroed memory; switch the userspace path from malloc to calloc to
>     match (patch 5).
> 
> Modernization:
>   - Replace the catch-all linux/kernel.h include with the specific
>     headers needed: linux/cache.h, linux/compiler.h, and
>     linux/sprintf.h (patch 7).
> 
> Build-tested with both the in-kernel build (lib/bootconfig.o,
> init/main.o) and the userspace tools/bootconfig build. All 70
> tools/bootconfig test cases pass.
> 
> Josh Law (15):
>   lib/bootconfig: clean up comment typos and bracing
>   lib/bootconfig: narrow flag parameter type from uint32_t to uint16_t
>   lib/bootconfig: fix off-by-one in xbc_verify_tree() next node check
>   lib/bootconfig: increment xbc_node_num after node init succeeds
>   lib/bootconfig: drop redundant memset of xbc_nodes
>   bootconfig: constify xbc_calc_checksum() data parameter
>   lib/bootconfig: replace linux/kernel.h with specific includes
>   lib/bootconfig: validate child node index in xbc_verify_tree()
>   lib/bootconfig: check xbc_init_node() return in override path
>   tools/bootconfig: fix fd leak in load_xbc_file() on fstat failure
>   lib/bootconfig: fix signed comparison in xbc_node_get_data()
>   lib/bootconfig: use size_t for strlen result in
>     xbc_node_match_prefix()
>   lib/bootconfig: use signed type for offset in xbc_init_node()
>   lib/bootconfig: use size_t for key length tracking in
>     xbc_verify_tree()
>   lib/bootconfig: change xbc_node_index() return type to uint16_t
> 
>  include/linux/bootconfig.h |  6 ++--
>  lib/bootconfig.c           | 65 ++++++++++++++++++++++----------------
>  tools/bootconfig/main.c    |  7 ++--
>  3 files changed, 46 insertions(+), 32 deletions(-)
> 
> -- 
> 2.34.1


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