[PATCH 0/3] binfmt_misc: only let punctuation delimit a register string

[PATCH 0/3] binfmt_misc: only let punctuation delimit a register string

[Christian Brauner]

Currently we accept arbitrary delimiters which really makes no sense and
from looking around it's an unused feature. Everytime we add a flag the
number of possible delimiter shrinks and it makes extensions much more
hazardous than they need to be. Stop it and only accept punctional as
delimiters.

This extends commit 8e85d50ba111 ("binfmt_misc: reject a flag character
as the field delimiter").

Letters, digits, whitespace, control characters, bytes above 0x7f and
the backslash now get -EINVAL. Everything deployed uses ':' anyway.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Christian Brauner (3):
      binfmt_misc: only let punctuation delimit a register string
      selftests/exec: test delimiter restrictions
      binfmt_misc: document the field delimiter

 Documentation/admin-guide/binfmt-misc.rst        |   3 +
 fs/binfmt_misc.c                                 |  12 ++-
 tools/testing/selftests/exec/Makefile            |   4 +
 tools/testing/selftests/exec/binfmt_misc_delim.c | 127 +++++++++++++++++++++++
 4 files changed, 143 insertions(+), 3 deletions(-)
---
base-commit: 66498c75b4f8017f62d720d9b59675bdf3abce91
change-id: 20260810-work-binfmt_misc-delim-83f62cbcfe23

[PATCH 1/3] binfmt_misc: only let punctuation delimit a register string

[Christian Brauner]

Currently we accept arbitrary delimiters which really makes no sense and
from looking around it's an unused feature. Everytime we add a flag the
number of possible delimiter shrinks and it makes extensions much more
hazardous than they need to be. Stop it and only accept punctional as
delimiters.

This extends commit 8e85d50ba111 ("binfmt_misc: reject a flag character
as the field delimiter").

Letters, digits, whitespace, control characters, bytes above 0x7f and
the backslash now get -EINVAL. Everything deployed uses ':' anyway.

Suggested-by: Farid Zakaria <farid.m.zakaria@gmail.com>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/binfmt_misc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index ddfd3aa57ac8..809c91d21b39 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -100,6 +100,13 @@ static const struct binfmt_misc_flag *misc_flag_by_char(const char c)
 	return NULL;
 }
 
+static bool misc_valid_delim(const char c)
+{
+	if (!isascii(c) || !ispunct(c))
+		return false;
+	return c != '\\';
+}
+
 struct binfmt_misc_entry {
 	struct hlist_node node;
 	unsigned long flags;		/* type, status, etc. */
@@ -872,10 +879,9 @@ static struct binfmt_misc_entry *create_entry(const char __user *buffer,
 
 	del = *p++;	/* delimiter */
 
-	pr_debug("register: delim: %#x {%c}\n", del, del);
+	pr_debug("register: delim: %#x\n", del);
 
-	/* A flag-char delimiter runs the flag scan off the buffer. */
-	if (misc_flag_by_char(del))
+	if (!misc_valid_delim(del))
 		return ERR_PTR(-EINVAL);
 
 	/* Pad the buffer with the delim to simplify parsing below. */

-- 
2.53.0

[PATCH 2/3] selftests/exec: test delimiter restrictions

[Christian Brauner]

Test that the new delimiter restrictions work.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 tools/testing/selftests/exec/Makefile            |   4 +
 tools/testing/selftests/exec/binfmt_misc_delim.c | 127 +++++++++++++++++++++++
 2 files changed, 131 insertions(+)

diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile
index b640af8f02b5..2220ed345e92 100644
--- a/tools/testing/selftests/exec/Makefile
+++ b/tools/testing/selftests/exec/Makefile
@@ -45,6 +45,10 @@ TEST_GEN_FILES += binfmt_transparent_interp
 TEST_GEN_PROGS += binfmt_misc_loader
 TEST_GEN_FILES += binfmt_loader_payload binfmt_loader_payload_static
 
+# Only ASCII punctuation delimits the fields of a register string, so a new
+# flag character cannot change which strings register. No bpf toolchain.
+TEST_GEN_PROGS += binfmt_misc_delim
+
 # binfmt_misc bpf-backed ('B') handler test: a libbpf harness plus its
 # struct_ops objects and the test interpreter/app it routes between. Only
 # built when clang, bpftool, the vmlinux BTF and libbpf are all present
diff --git a/tools/testing/selftests/exec/binfmt_misc_delim.c b/tools/testing/selftests/exec/binfmt_misc_delim.c
new file mode 100644
index 000000000000..ffc17cb78545
--- /dev/null
+++ b/tools/testing/selftests/exec/binfmt_misc_delim.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test which characters may delimit the fields of a register string.
+ */
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "binfmt_misc_common.h"
+#include "kselftest_harness.h"
+
+#define ENTRY	"bmdelim"
+/* Shares no character with the sets below, or a refusal proves nothing. */
+#define MAGIC	"bmmagic"
+#define INTERP	"/bin/true"
+
+/*
+ * ASCII punctuation without '\' and '/'. The backslash is refused because
+ * it would cut a magic that uses \x to escape short. '/' is accepted
+ * but cannot delimit a rule that names an absolute interpreter.
+ */
+#define PUNCTUATION	"!\"#$%&'()*+,-.:;<=>?@[]^_`{|}~"
+
+/* 'M', 'E' and 'B' name types, 'P' through 'D' are the flags. */
+#define LETTERS		"MEBPOCFTLDqz"
+#define DIGITS		"0157"
+#define WHITESPACE	" \t\n"
+#define CONTROL		"\001\033\177"
+#define NON_ASCII	"\200\244\377"
+
+/* ':bmdelim:E::bmmagic::/bin/true:' with @del in place of every ':'. */
+static int register_with(char del)
+{
+	char rule[128];
+
+	snprintf(rule, sizeof(rule), "%c%s%cE%c%c%s%c%c%s%c", del, ENTRY, del,
+		 del, del, MAGIC, del, del, INTERP, del);
+	return write_reg(rule);
+}
+
+/* No character of @set may delimit a register string. */
+static void expect_refused(struct __test_metadata *_metadata, const char *set)
+{
+	const char *d;
+
+	for (d = set; *d; d++) {
+		int rc = register_with(*d);
+
+		EXPECT_EQ(rc, -1)
+			TH_LOG("%#x delimited a register string",
+			       (unsigned char)*d);
+		if (rc == 0) {
+			unregister(ENTRY);
+			continue;
+		}
+		EXPECT_EQ(errno, EINVAL);
+	}
+}
+
+FIXTURE(delim) {
+};
+
+FIXTURE_SETUP(delim)
+{
+	if (getuid() != 0)
+		SKIP(return, "test must be run as root");
+	if (!binfmt_misc_available())
+		SKIP(return, "no binfmt_misc");
+
+	/* A kernel without the allow-list takes any character but a flag. */
+	if (register_with('q') == 0) {
+		unregister(ENTRY);
+		SKIP(return, "kernel without the delimiter allow-list");
+	}
+}
+
+FIXTURE_TEARDOWN(delim)
+{
+	unregister(ENTRY);
+}
+
+/* Punctuation delimits, which is all anything deployed ever uses. */
+TEST_F(delim, punctuation_accepted)
+{
+	const char *d;
+
+	for (d = PUNCTUATION; *d; d++) {
+		EXPECT_EQ(register_with(*d), 0)
+			TH_LOG("'%c' refused with errno %d", *d, errno);
+		unregister(ENTRY);
+	}
+}
+
+/* Letters name the types and the flags, so none of them can delimit. */
+TEST_F(delim, letters_refused)
+{
+	expect_refused(_metadata, LETTERS);
+}
+
+/* The offset field is written in digits. */
+TEST_F(delim, digits_refused)
+{
+	expect_refused(_metadata, DIGITS);
+}
+
+TEST_F(delim, whitespace_refused)
+{
+	expect_refused(_metadata, WHITESPACE);
+}
+
+TEST_F(delim, control_refused)
+{
+	expect_refused(_metadata, CONTROL);
+}
+
+TEST_F(delim, non_ascii_refused)
+{
+	expect_refused(_metadata, NON_ASCII);
+}
+
+/* The escape character would cut every magic that uses one short. */
+TEST_F(delim, backslash_refused)
+{
+	expect_refused(_metadata, "\\");
+}
+
+TEST_HARNESS_MAIN

-- 
2.53.0

[PATCH 3/3] binfmt_misc: document the field delimiter

[Christian Brauner]

Document the field delimiter.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 Documentation/admin-guide/binfmt-misc.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/admin-guide/binfmt-misc.rst b/Documentation/admin-guide/binfmt-misc.rst
index d26b63a27c25..9e84b877d06d 100644
--- a/Documentation/admin-guide/binfmt-misc.rst
+++ b/Documentation/admin-guide/binfmt-misc.rst
@@ -19,6 +19,9 @@ To actually register a new binary type, you have to set up a string looking like
 ``:name:type:offset:magic:mask:interpreter:flags`` (where you can choose the
 ``:`` upon your needs) and echo it to ``/proc/sys/fs/binfmt_misc/register``.
 
+The first character of the string is its field delimiter and can be any
+ASCII punctuation character other than the backslash ``\``.
+
 Here is what the fields mean:
 
 - ``name``

-- 
2.53.0