Re: [RFC v2 1/2] kernel/sysctl: support setting sysctl parameters from kernel command line

On 3/25/20 10:21 PM, Kees Cook wrote:

quoted

--- a/init/main.c
+++ b/init/main.c

@@ -1345,6 +1345,25 @@ void __weak free_initmem(void)
 	free_initmem_default(POISON_FREE_INITMEM);
 }
 
+static void do_sysctl_args(void)
+{
+#ifdef CONFIG_SYSCTL
+	size_t len = strlen(saved_command_line) + 1;
+	char *command_line;
+
+	command_line = kzalloc(len, GFP_KERNEL);
+	if (!command_line)
+		panic("%s: Failed to allocate %zu bytes\n", __func__, len);
+
+	strcpy(command_line, saved_command_line);

No need to open-code this:

	char *command_line;

	command_line = kstrdup(saved_command_line, GFP_KERNEL);
	if (!command_line)
		panic("%s: Failed to allocate %zu bytes\n", __func__, len);

Ah, right. I admit I basically copy_pasted some other parse_args user.

quoted

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ad5b88a53c5a..18c7f5606d55 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c

@@ -1980,6 +1980,68 @@ int __init sysctl_init(void)
 	return 0;
 }
 
+/* Set sysctl value passed on kernel command line. */
+int process_sysctl_arg(char *param, char *val,
+			       const char *unused, void *arg)
+{
+	size_t count;
+	char *remaining;
+	int err;
+	loff_t ppos = 0;
+	struct ctl_table *ctl, *found = NULL;
+
+	if (strncmp(param, "sysctl.", sizeof("sysctl.") - 1))
+		return 0;
+
+	param += sizeof("sysctl.") - 1;
+
+	remaining = param;
+	ctl = &sysctl_base_table[0];
+
+	while(ctl->procname != 0) {
+		int len = strlen(ctl->procname);
+		if (strncmp(remaining, ctl->procname, len)) {
+			ctl++;
+			continue;
+		}

I think you need to validate that "len" is within "remaining" here
first.

My reasoning was that if remaining terminates too early, the null byte would be
different from non-null byte in ctl->procname and thus strncmp will return it as
different?
And the reason I used len in strncmp there is only so it doesn't compare the
terminating null, because remaning can continue with ".foo" instead.

quoted

+		if (ctl->child) {
+			if (remaining[len] == '.') {
+				remaining += len + 1;

And that "len + 1" is still valid.

And since we passed strncmp(..., len), remaining[len] might be null byte, but
then we can still compare it with '.'.

But C strings are full of landmines.