[PATCH v6 00/17] bootconfig: fixes, cleanups, and modernization

[PATCH v6 00/17] 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 v5:
  - Folded typo fixes, kerneldoc blank line, and inconsistent bracing
    patches (v5 02-05, 07) into a single patch (patch 2).
  - 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 10).
  - Added Fixes: tag to "fix fd leak in load_xbc_file() on fstat
    failure" (patch 11).

Changes since v4:
  - Added six follow-up patches found via static analysis with strict
    GCC warnings (patches 12-17).
  - Added "fix signed comparison in xbc_node_get_data()" -- switch the
    masked offset variable to unsigned int and compare against
    XBC_DATA_MAX to avoid a signed comparison and make the mask
    self-documenting (patch 12).
  - 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 13, 15).
  - Added "narrow offset type in xbc_init_node()" -- use a validated
    unsigned int temporary for the stored 15-bit data offset
    (patch 14).
  - Added "fix sign-compare in xbc_node_compose_key_after()" -- cast
    the checked snprintf() return when comparing and subtracting
    against a size_t buffer length (patch 16).
  - Added "change xbc_node_index() return type to uint16_t" -- match
    the 16-bit storage fields and XBC_NODE_MAX bounds (patch 17).

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 9).
  - Added "check xbc_init_node() return in override path" (patch 10).
  - Added "fix fd leak in load_xbc_file() on fstat failure" (patch 11).

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 4).
  - 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 5).
  - 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 9).
  - 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 10).
  - Fix file descriptor leak in tools/bootconfig load_xbc_file()
    when fstat() fails (patch 11).

Correctness:
  - Add missing __init annotations to skip_comment() and
    skip_spaces_until_newline() so their memory can be reclaimed
    after init (patch 1).
  - Narrow the flag parameter in node creation helpers from uint32_t
    to uint16_t to match the xbc_node.data field width (patch 3).
  - Constify the xbc_calc_checksum() data parameter since it only
    reads the buffer (patch 7).
  - Fix strict-GCC signedness and narrowing warnings by aligning local
    types with strlen()/snprintf() APIs and the 16-bit node index/data
    storage in xbc_node_get_data(), xbc_node_match_prefix(),
    xbc_init_node(), xbc_verify_tree(), xbc_node_compose_key_after(),
    and xbc_node_index() (patches 12-17).

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

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

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 (17):
  lib/bootconfig: add missing __init annotations to static helpers
  lib/bootconfig: fix typos, kerneldoc, and inconsistent if/else 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: narrow offset type in xbc_init_node()
  lib/bootconfig: use size_t for key length tracking in
    xbc_verify_tree()
  lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()
  lib/bootconfig: change xbc_node_index() return type to uint16_t

 include/linux/bootconfig.h |  6 ++--
 lib/bootconfig.c           | 71 ++++++++++++++++++++++----------------
 tools/bootconfig/main.c    |  4 ++-
 3 files changed, 47 insertions(+), 34 deletions(-)

-- 
2.34.1

[PATCH v6 01/17] lib/bootconfig: add missing __init annotations to static helpers

[Josh Law]

skip_comment() and skip_spaces_until_newline() are static functions
called exclusively from __init code paths but lack the __init
annotation themselves. Add it so their memory can be reclaimed after
init.

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 b0ef1e74e98a..51fd2299ec0f 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -509,7 +509,7 @@ static inline __init bool xbc_valid_keyword(char *key)
 	return *key == '\0';
 }
 
-static char *skip_comment(char *p)
+static char __init *skip_comment(char *p)
 {
 	char *ret;
 
@@ -522,7 +522,7 @@ static char *skip_comment(char *p)
 	return ret;
 }
 
-static char *skip_spaces_until_newline(char *p)
+static char __init *skip_spaces_until_newline(char *p)
 {
 	while (isspace(*p) && *p != '\n')
 		p++;
-- 
2.34.1

[PATCH v6 02/17] lib/bootconfig: fix typos, kerneldoc, and inconsistent if/else bracing

[Josh Law]

Fix comment typos ("initiized" -> "initialized" in xbc_root_node(),
"uder" -> "under" in xbc_node_find_next_leaf()), add a missing blank
line before the xbc_get_info() kerneldoc block, and add braces to
if/else blocks where one branch uses braces but the other does not,
per coding-style section 3.1.

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 51fd2299ec0f..80de9540245d 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 v6 03/17] 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 80de9540245d..58d6ae297280 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 v6 04/17] 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 58d6ae297280..56fbedc9e725 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 v6 05/17] 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 56fbedc9e725..06e8a79ab472 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 v6 06/17] 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 06e8a79ab472..fe1053043752 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 v6 07/17] 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 v6 08/17] 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 fe1053043752..0823491221f4 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 v6 09/17] 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 0823491221f4..038f56689a48 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 v6 10/17] 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 038f56689a48..182d9d9bc5a6 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 v6 11/17] tools/bootconfig: fix fd leak in load_xbc_file() on fstat failure

[Josh Law]

If fstat() fails after open() succeeds, load_xbc_file() returns
-errno without closing the file descriptor.  Add the missing close()
call on the error path.

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

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

[PATCH v6 12/17] 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.  Using ~XBC_VALUE as the mask also involves integer promotion
rules that obscure intent.

Change the type to unsigned int and mask with XBC_DATA_MAX (which is
the 15-bit data mask) instead of ~XBC_VALUE, making the expression
self-documenting and eliminating the signed/unsigned comparison.

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 182d9d9bc5a6..806a8f038d24 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;
+	unsigned int offset = node->data & XBC_DATA_MAX;
 
 	if (WARN_ON(offset >= xbc_data_size))
 		return NULL;
-- 
2.34.1

[PATCH v6 13/17] 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 806a8f038d24..995c2ec94cbe 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 v6 14/17] lib/bootconfig: narrow offset type 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 long), which was stored in
unsigned long.  The offset is immediately checked against XBC_DATA_MAX
(32767) and then truncated to uint16_t, so unsigned int is sufficient.
Add an explicit cast on the subtraction to suppress the sign-conversion
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 995c2ec94cbe..7296df003459 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -412,7 +412,7 @@ 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;
+	unsigned int offset = (unsigned int)(data - xbc_data);
 
 	if (WARN_ON(offset >= XBC_DATA_MAX))
 		return -EINVAL;
-- 
2.34.1

[PATCH v6 15/17] 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 7296df003459..e318b236e728 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 v6 16/17] lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()

[Josh Law]

  lib/bootconfig.c:322:25: warning: comparison of integer expressions
  of different signedness: 'int' and 'size_t' [-Wsign-compare]
  lib/bootconfig.c:325:30: warning: conversion to 'size_t' from 'int'
  may change the sign of the result [-Wsign-conversion]

snprintf() returns int but size is size_t, so comparing ret >= size
and subtracting size -= ret involve mixed-sign operations.  Cast ret
at the comparison and subtraction sites; ret is known non-negative at
this point because the ret < 0 early return has already been taken.

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 e318b236e728..68a72dbc38fa 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
 			       depth ? "." : "");
 		if (ret < 0)
 			return ret;
-		if (ret >= size) {
+		if (ret >= (int)size) {
 			size = 0;
 		} else {
-			size -= ret;
+			size -= (size_t)ret;
 			buf += ret;
 		}
 		total += ret;
-- 
2.34.1

[PATCH v6 17/17] 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 68a72dbc38fa..148084abae12 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 v6 12/17] lib/bootconfig: fix signed comparison in xbc_node_get_data()

[Masami Hiramatsu]

On Sun, 15 Mar 2026 12:20:10 +0000
Josh Law <objecting@objecting.org> wrote:

>   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.  Using ~XBC_VALUE as the mask also involves integer promotion
> rules that obscure intent.
> 
> Change the type to unsigned int and mask with XBC_DATA_MAX (which is
> the 15-bit data mask) instead of ~XBC_VALUE, making the expression
> self-documenting and eliminating the signed/unsigned comparison.

Please follow the warning message and use size_t instead.

Thanks,

> 
> 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 182d9d9bc5a6..806a8f038d24 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;
> +	unsigned int offset = node->data & XBC_DATA_MAX;
>  
>  	if (WARN_ON(offset >= xbc_data_size))
>  		return NULL;
> -- 
> 2.34.1
> 
> 


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

Re: [PATCH v6 14/17] lib/bootconfig: narrow offset type in xbc_init_node()

[Masami Hiramatsu]

On Sun, 15 Mar 2026 12:20:12 +0000
Josh Law <objecting@objecting.org> wrote:

>   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 long), which was stored in
> unsigned long.  The offset is immediately checked against XBC_DATA_MAX
> (32767) and then truncated to uint16_t, so unsigned int is sufficient.
> Add an explicit cast on the subtraction to suppress the sign-conversion
> 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 995c2ec94cbe..7296df003459 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -412,7 +412,7 @@ 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;
> +	unsigned int offset = (unsigned int)(data - xbc_data);
>  
>  	if (WARN_ON(offset >= XBC_DATA_MAX))

OK, then this can be changed to

	long offset = data - xbc_data;

	if (WARN_ON(offset < 0 || offset >= XBC_DATA_MAX))

The original code is to handle data < xbc_data case (in that
case, the offset is over LONG_MAX, so offset >= XBC_DATA_MAX
is also true.) Note that this is for catching broken pointer
to find program bug (WARN_ON is used for such case).

Thank you,

>  		return -EINVAL;
> -- 
> 2.34.1
> 


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

Re: [PATCH v6 11/17] tools/bootconfig: fix fd leak in load_xbc_file() on fstat failure

[Masami Hiramatsu]

On Sun, 15 Mar 2026 12:20:09 +0000
Josh Law <objecting@objecting.org> wrote:

> If fstat() fails after open() succeeds, load_xbc_file() returns
> -errno without closing the file descriptor.  Add the missing close()
> call on the error path.
> 
> Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---
>  tools/bootconfig/main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> index 55d59ed507d5..8078fee0b75b 100644
> --- a/tools/bootconfig/main.c
> +++ b/tools/bootconfig/main.c
> @@ -162,8 +162,10 @@ static int load_xbc_file(const char *path, char **buf)
>  	if (fd < 0)
>  		return -errno;
>  	ret = fstat(fd, &stat);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		close(fd);
>  		return -errno;

Sashiko.dev[1] found that close() will overwrite errno. So please make it

	ret = -errno;
	close(fd);
	return ret;

[1] https://sashiko.dev/#/patchset/20260315122015.55965-1-objecting%40objecting.org

Thanks,

> +	}
>  
>  	ret = load_xbc_fd(fd, buf, stat.st_size);
>  
> -- 
> 2.34.1
> 


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

Re: [PATCH v6 01/17] lib/bootconfig: add missing __init annotations to static helpers

[Masami Hiramatsu]

On Sun, 15 Mar 2026 12:19:59 +0000
Josh Law <objecting@objecting.org> wrote:

> skip_comment() and skip_spaces_until_newline() are static functions
> called exclusively from __init code paths but lack the __init
> annotation themselves. Add it so their memory can be reclaimed after
> init.
> 
> 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 b0ef1e74e98a..51fd2299ec0f 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -509,7 +509,7 @@ static inline __init bool xbc_valid_keyword(char *key)
>  	return *key == '\0';
>  }
>  
> -static char *skip_comment(char *p)
> +static char __init *skip_comment(char *p)

static __init char *skip_comment()

__init attribute is not for char but the function itself.


>  {
>  	char *ret;
>  
> @@ -522,7 +522,7 @@ static char *skip_comment(char *p)
>  	return ret;
>  }
>  
> -static char *skip_spaces_until_newline(char *p)
> +static char __init *skip_spaces_until_newline(char *p)

Ditto.

>  {
>  	while (isspace(*p) && *p != '\n')
>  		p++;
> -- 
> 2.34.1
> 
> 


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

Re: [PATCH v6 11/17] tools/bootconfig: fix fd leak in load_xbc_file() on fstat failure

[Josh Law]

On 17 March 2026 07:31:51 GMT, Masami Hiramatsu <mhiramat@kernel.org> wrote:
>On Sun, 15 Mar 2026 12:20:09 +0000
>Josh Law <objecting@objecting.org> wrote:
>
>> If fstat() fails after open() succeeds, load_xbc_file() returns
>> -errno without closing the file descriptor.  Add the missing close()
>> call on the error path.
>> 
>> Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
>> Signed-off-by: Josh Law <objecting@objecting.org>
>> ---
>>  tools/bootconfig/main.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
>> index 55d59ed507d5..8078fee0b75b 100644
>> --- a/tools/bootconfig/main.c
>> +++ b/tools/bootconfig/main.c
>> @@ -162,8 +162,10 @@ static int load_xbc_file(const char *path, char **buf)
>>  	if (fd < 0)
>>  		return -errno;
>>  	ret = fstat(fd, &stat);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>> +		close(fd);
>>  		return -errno;
>
>Sashiko.dev[1] found that close() will overwrite errno. So please make it
>
>	ret = -errno;
>	close(fd);
>	return ret;
>
>[1] https://sashiko.dev/#/patchset/20260315122015.55965-1-objecting%40objecting.org
>
>Thanks,
>
>> +	}
>>  
>>  	ret = load_xbc_fd(fd, buf, stat.st_size);
>>  
>> -- 
>> 2.34.1
>> 
>
>


I'm on the computer now, I'm cleaning everything up as requested, expect a patch coming.

V/R

Josh Law

Re: [PATCH v6 16/17] lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()

[Masami Hiramatsu]

On Sun, 15 Mar 2026 12:20:14 +0000
Josh Law <objecting@objecting.org> wrote:

>   lib/bootconfig.c:322:25: warning: comparison of integer expressions
>   of different signedness: 'int' and 'size_t' [-Wsign-compare]
>   lib/bootconfig.c:325:30: warning: conversion to 'size_t' from 'int'
>   may change the sign of the result [-Wsign-conversion]
> 
> snprintf() returns int but size is size_t, so comparing ret >= size
> and subtracting size -= ret involve mixed-sign operations.  Cast ret
> at the comparison and subtraction sites; ret is known non-negative at
> this point because the ret < 0 early return has already been taken.
> 
> 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 e318b236e728..68a72dbc38fa 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>  			       depth ? "." : "");
>  		if (ret < 0)
>  			return ret;
> -		if (ret >= size) {
> +		if (ret >= (int)size) {

nit:

	if ((size_t)ret >= size) {

because sizeof(size_t) > sizeof(int).

Thanks,

>  			size = 0;
>  		} else {
> -			size -= ret;
> +			size -= (size_t)ret;
>  			buf += ret;
>  		}
>  		total += ret;
> -- 
> 2.34.1
> 
> 


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

Re: [PATCH v6 09/17] lib/bootconfig: validate child node index in xbc_verify_tree()

[Markus Elfring]

…
> +++ 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));
> +		}
>  	}
…

How do you think about to omit curly brackets for this if branch?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.0-rc4#n197

Regards,
Markus

Re: [PATCH v6 06/17] lib/bootconfig: drop redundant memset of xbc_nodes

[Markus Elfring]

> memblock_alloc() already returns zeroed memory,

Interesting …


>                                                 so the explicit memset
> in xbc_init() is redundant. …

How is this conclusion appropriate for the mentioned function implementation?
https://elixir.bootlin.com/linux/v7.0-rc4/source/lib/bootconfig.c#L932-L998

Regards,
Markus

Re: [PATCH v6 09/17] lib/bootconfig: validate child node index in xbc_verify_tree()

[Steven Rostedt]

On Tue, 17 Mar 2026 12:03:40 +0100
Markus Elfring <Markus.Elfring@web.de> wrote:

> …
> > +++ 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));
> > +		}
> >  	}  
> …
> 
> How do you think about to omit curly brackets for this if branch?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.0-rc4#n197
> 

Markus, please stop. If you don't understand the rules, do not suggest them.

The brackets *are* appropriate. The rule of omitting the brackets is for
*single line* statements. The above return statement is long and there's a
line break, which means, curly brackets *are* required for visibility reasons.

-- Steve

Re: [PATCH v6 16/17] lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()

[Steven Rostedt]

On Tue, 17 Mar 2026 16:55:49 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > --- a/lib/bootconfig.c
> > +++ b/lib/bootconfig.c
> > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
> >  			       depth ? "." : "");
> >  		if (ret < 0)
> >  			return ret;
> > -		if (ret >= size) {
> > +		if (ret >= (int)size) {  
> 
> nit:
> 
> 	if ((size_t)ret >= size) {
> 
> because sizeof(size_t) > sizeof(int).

I don't think we need to worry about this. But this does bring up an issue.
ret comes from:

		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
			       depth ? "." : "");

Where size is of type size_t

snprintf() takes size_t but returns int.

snprintf() calls vsnprintf() which has:

	size_t len, pos;

Where pos is incremented based on fmt, and vsnprintf() returns:

	return pos;

Which can overflow.

Now, honestly, we should never have a 2Gig string as that would likely
cause other horrible things. Does size really need to be size_t?

Perhaps we should have:

	if (WARN_ON_ONCE(size > MAX_INT))
		return -EINVAL;

?

-- Steve

Re: [PATCH v6 16/17] lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()

[Josh Law]

On 17 March 2026 16:15:07 GMT, Steven Rostedt <rostedt@goodmis.org> wrote:
>On Tue, 17 Mar 2026 16:55:49 +0900
>Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>
>> > --- a/lib/bootconfig.c
>> > +++ b/lib/bootconfig.c
>> > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>> >  			       depth ? "." : "");
>> >  		if (ret < 0)
>> >  			return ret;
>> > -		if (ret >= size) {
>> > +		if (ret >= (int)size) {  
>> 
>> nit:
>> 
>> 	if ((size_t)ret >= size) {
>> 
>> because sizeof(size_t) > sizeof(int).
>
>I don't think we need to worry about this. But this does bring up an issue.
>ret comes from:
>
>		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
>			       depth ? "." : "");
>
>Where size is of type size_t
>
>snprintf() takes size_t but returns int.
>
>snprintf() calls vsnprintf() which has:
>
>	size_t len, pos;
>
>Where pos is incremented based on fmt, and vsnprintf() returns:
>
>	return pos;
>
>Which can overflow.
>
>Now, honestly, we should never have a 2Gig string as that would likely
>cause other horrible things. Does size really need to be size_t?
>
>Perhaps we should have:
>
>	if (WARN_ON_ONCE(size > MAX_INT))
>		return -EINVAL;
>
>?
>
>-- Steve


Hello Steven! I made a V7 dropping that since masami nitted it anyway, and I was too busy to fix it.

V/R


If you would like to review V7, go right ahead!

Re: [PATCH v6 16/17] lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()

[Josh Law]

On 17 March 2026 16:15:07 GMT, Steven Rostedt <rostedt@goodmis.org> wrote:
>On Tue, 17 Mar 2026 16:55:49 +0900
>Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>
>> > --- a/lib/bootconfig.c
>> > +++ b/lib/bootconfig.c
>> > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>> >  			       depth ? "." : "");
>> >  		if (ret < 0)
>> >  			return ret;
>> > -		if (ret >= size) {
>> > +		if (ret >= (int)size) {  
>> 
>> nit:
>> 
>> 	if ((size_t)ret >= size) {
>> 
>> because sizeof(size_t) > sizeof(int).
>
>I don't think we need to worry about this. But this does bring up an issue.
>ret comes from:
>
>		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
>			       depth ? "." : "");
>
>Where size is of type size_t
>
>snprintf() takes size_t but returns int.
>
>snprintf() calls vsnprintf() which has:
>
>	size_t len, pos;
>
>Where pos is incremented based on fmt, and vsnprintf() returns:
>
>	return pos;
>
>Which can overflow.
>
>Now, honestly, we should never have a 2Gig string as that would likely
>cause other horrible things. Does size really need to be size_t?
>
>Perhaps we should have:
>
>	if (WARN_ON_ONCE(size > MAX_INT))
>		return -EINVAL;
>
>?
>
>-- Steve



I'm making a separate patch based on this.

V/R

Josh Law

Re: [PATCH v6 16/17] lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()

[Masami Hiramatsu]

On Tue, 17 Mar 2026 12:15:07 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 17 Mar 2026 16:55:49 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > > --- a/lib/bootconfig.c
> > > +++ b/lib/bootconfig.c
> > > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
> > >  			       depth ? "." : "");
> > >  		if (ret < 0)
> > >  			return ret;
> > > -		if (ret >= size) {
> > > +		if (ret >= (int)size) {  
> > 
> > nit:
> > 
> > 	if ((size_t)ret >= size) {
> > 
> > because sizeof(size_t) > sizeof(int).
> 
> I don't think we need to worry about this. But this does bring up an issue.
> ret comes from:
> 
> 		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
> 			       depth ? "." : "");
> 
> Where size is of type size_t
> 
> snprintf() takes size_t but returns int.
> 
> snprintf() calls vsnprintf() which has:
> 
> 	size_t len, pos;
> 
> Where pos is incremented based on fmt, and vsnprintf() returns:
> 
> 	return pos;
> 
> Which can overflow.

I think that is vsnprintf() (maybe POSIX) design issue.
I believe we're simply using the size_t to represent size of memory
out of convention.

> 
> Now, honestly, we should never have a 2Gig string as that would likely
> cause other horrible things. Does size really need to be size_t?

Even if so, it should be done in vsnprintf() instead of this.
This function just believes that the caller gives collect size
and enough amount of memory. Or, we need to check "INT_MAX > size"
in everywhere.

> 
> Perhaps we should have:
> 
> 	if (WARN_ON_ONCE(size > MAX_INT))
> 		return -EINVAL;

I think this is an over engineering effort especially in
caller side. This overflow should be checked in vsnprintf() and
should return -EINVAL. (and the caller checks the return value.)

Thank you,

> 
> ?
> 
> -- Steve
> 


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

Re: [PATCH v6 16/17] lib/bootconfig: fix sign-compare in xbc_node_compose_key_after()

[Josh Law]

On 17 March 2026 23:15:40 GMT, Masami Hiramatsu <mhiramat@kernel.org> wrote:
>On Tue, 17 Mar 2026 12:15:07 -0400
>Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> On Tue, 17 Mar 2026 16:55:49 +0900
>> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>> 
>> > > --- a/lib/bootconfig.c
>> > > +++ b/lib/bootconfig.c
>> > > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>> > >  			       depth ? "." : "");
>> > >  		if (ret < 0)
>> > >  			return ret;
>> > > -		if (ret >= size) {
>> > > +		if (ret >= (int)size) {  
>> > 
>> > nit:
>> > 
>> > 	if ((size_t)ret >= size) {
>> > 
>> > because sizeof(size_t) > sizeof(int).
>> 
>> I don't think we need to worry about this. But this does bring up an issue.
>> ret comes from:
>> 
>> 		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
>> 			       depth ? "." : "");
>> 
>> Where size is of type size_t
>> 
>> snprintf() takes size_t but returns int.
>> 
>> snprintf() calls vsnprintf() which has:
>> 
>> 	size_t len, pos;
>> 
>> Where pos is incremented based on fmt, and vsnprintf() returns:
>> 
>> 	return pos;
>> 
>> Which can overflow.
>
>I think that is vsnprintf() (maybe POSIX) design issue.
>I believe we're simply using the size_t to represent size of memory
>out of convention.
>
>> 
>> Now, honestly, we should never have a 2Gig string as that would likely
>> cause other horrible things. Does size really need to be size_t?
>
>Even if so, it should be done in vsnprintf() instead of this.
>This function just believes that the caller gives collect size
>and enough amount of memory. Or, we need to check "INT_MAX > size"
>in everywhere.
>
>> 
>> Perhaps we should have:
>> 
>> 	if (WARN_ON_ONCE(size > MAX_INT))
>> 		return -EINVAL;
>
>I think this is an over engineering effort especially in
>caller side. This overflow should be checked in vsnprintf() and
>should return -EINVAL. (and the caller checks the return value.)
>
>Thank you,
>
>> 
>> ?
>> 
>> -- Steve
>> 
>
>


I submitted V7 dropping all them patches anyway, V7 should be perfect now.


V/R


Josh Law

Re: [RFC] Coding style consequences for multi-line statements?

[Markus Elfring]

> The brackets *are* appropriate. The rule of omitting the brackets is for
> *single line* statements. The above return statement is long and there's a
> line break, which means, curly brackets *are* required for visibility reasons.

Would any contributors like to clarify and adjust development documentation accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.0-rc4#n197

Regards,
Markus