bootconfig: targeted fixes for stable

Two bug fixes with Fixes tags, split out from the larger cleanup series (v7 patches 9/15 and 10/15) per Masami's request so they can go cleanly into bootconfig/fixes and from there into stable.

Changes since v7:
- Split fixes (this series) from cleanups/improvements into separate series per maintainer request, so fixes can go into bootconfig/fixes and the rest into bootconfig/for-next.

Bug fixes:
- 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 1).
- Fix file descriptor leak in tools/bootconfig load_xbc_file() when fstat() fails, and preserve errno across close() on that error path (patch 2).

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 (2):
lib/bootconfig: check xbc_init_node() return in override path tools/bootconfig: fix fd leak in load_xbc_file() on fstat failure

 lib/bootconfig.c        | 3 ++-
 tools/bootconfig/main.c | 7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

--
2.34.1

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.

---
 lib/bootconfig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 1adf592cc038..ecc4e8d93547 100644
@@ -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

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.

---
 tools/bootconfig/main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 55d59ed507d5..643f707b8f1d 100644
@@ -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

I find such a change description improvable.

Did anything hinder to use a corresponding goto chain?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.0-rc4#n526
https://elixir.bootlin.com/linux/v7.0-rc4/source/tools/bootconfig/main.c#L155-L173

Regards,
Markus