[PATCH v8 0/2] bootconfig: targeted fixes for stable

[PATCH v8 0/2] bootconfig: targeted fixes for stable

[Josh Law]

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

[PATCH v8 1/2] 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 v8 2/2] 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

Re: [PATCH v8 2/2] tools/bootconfig: fix fd leak in load_xbc_file() on fstat failure

[Markus Elfring]

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

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