sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array

Move boundary checking for proc_dou8ved_minmax into module loading, thereby
reporting errors in advance. And add a kunit test case ensuring the
boundary check is done correctly.

The boundary check in proc_dou8vec_minmax done to the extra elements in
the ctl_table struct is currently performed at runtime. This allows buggy
kernel modules to be loaded normally without any errors only to fail
when used.

This is a buggy example module:
	#include <linux/kernel.h>
	#include <linux/module.h>
	#include <linux/sysctl.h>

	static struct ctl_table_header *_table_header = NULL;
	static unsigned char _data = 0;
	struct ctl_table table[] = {
		{
			.procname       = "foo",
			.data           = &_data,
			.maxlen         = sizeof(u8),
			.mode           = 0644,
			.proc_handler   = proc_dou8vec_minmax,
			.extra1         = SYSCTL_ZERO,
			.extra2         = SYSCTL_ONE_THOUSAND,
		},
	};

	static int init_demo(void) {
		_table_header = register_sysctl("kernel", table);
		if (!_table_header)
			return -ENOMEM;

		return 0;
	}

	module_init(init_demo);
	MODULE_LICENSE("GPL");

And this is the result:
        # insmod test.ko
        # cat /proc/sys/kernel/foo
        cat: /proc/sys/kernel/foo: Invalid argument

Suggested-by: Joel Granados <j.granados@samsung.com>
Signed-off-by: Wen Yang <wen.yang@linux.dev>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Joel Granados <j.granados@samsung.com>
Signed-off-by: Joel Granados <j.granados@samsung.com>

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index dd7b462387a00d9df66e86556720ca17d68824d2..c467e36741d03bb02d97e66f00ceefc57cf03c7b 100644 (file)
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1091,6 +1091,7 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...)
 
 static int sysctl_check_table_array(const char *path, struct ctl_table *table)
 {
+       unsigned int extra;
        int err = 0;
 
        if ((table->proc_handler == proc_douintvec) ||
@@ -1102,6 +1103,19 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
        if (table->proc_handler == proc_dou8vec_minmax) {
                if (table->maxlen != sizeof(u8))
                        err |= sysctl_err(path, table, "array not allowed");
+
+               if (table->extra1) {
+                       extra = *(unsigned int *) table->extra1;
+                       if (extra > 255U)
+                               err |= sysctl_err(path, table,
+                                               "range value too large for proc_dou8vec_minmax");
+               }
+               if (table->extra2) {
+                       extra = *(unsigned int *) table->extra2;
+                       if (extra > 255U)
+                               err |= sysctl_err(path, table,
+                                               "range value too large for proc_dou8vec_minmax");
+               }
        }
 
        if (table->proc_handler == proc_dobool) {

diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
index 6ef887c19c4887d28c5bc7f80c89abc947bd42d9..4e7dcc9187e2525850d0a602938bf893ad03b5e9 100644 (file)
--- a/kernel/sysctl-test.c
+++ b/kernel/sysctl-test.c
@@ -367,6 +367,54 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max(
        KUNIT_EXPECT_EQ(test, 0, *((int *)table.data));
 }
 
+/*
+ * Test that registering an invalid extra value is not allowed.
+ */
+static void sysctl_test_register_sysctl_sz_invalid_extra_value(
+               struct kunit *test)
+{
+       unsigned char data = 0;
+       struct ctl_table table_foo[] = {
+               {
+                       .procname       = "foo",
+                       .data           = &data,
+                       .maxlen         = sizeof(u8),
+                       .mode           = 0644,
+                       .proc_handler   = proc_dou8vec_minmax,
+                       .extra1         = SYSCTL_FOUR,
+                       .extra2         = SYSCTL_ONE_THOUSAND,
+               },
+       };
+
+       struct ctl_table table_bar[] = {
+               {
+                       .procname       = "bar",
+                       .data           = &data,
+                       .maxlen         = sizeof(u8),
+                       .mode           = 0644,
+                       .proc_handler   = proc_dou8vec_minmax,
+                       .extra1         = SYSCTL_NEG_ONE,
+                       .extra2         = SYSCTL_ONE_HUNDRED,
+               },
+       };
+
+       struct ctl_table table_qux[] = {
+               {
+                       .procname       = "qux",
+                       .data           = &data,
+                       .maxlen         = sizeof(u8),
+                       .mode           = 0644,
+                       .proc_handler   = proc_dou8vec_minmax,
+                       .extra1         = SYSCTL_ZERO,
+                       .extra2         = SYSCTL_TWO_HUNDRED,
+               },
+       };
+
+       KUNIT_EXPECT_NULL(test, register_sysctl("foo", table_foo));
+       KUNIT_EXPECT_NULL(test, register_sysctl("foo", table_bar));
+       KUNIT_EXPECT_NOT_NULL(test, register_sysctl("foo", table_qux));
+}
+
 static struct kunit_case sysctl_test_cases[] = {
        KUNIT_CASE(sysctl_test_api_dointvec_null_tbl_data),
        KUNIT_CASE(sysctl_test_api_dointvec_table_maxlen_unset),
@@ -378,6 +426,7 @@ static struct kunit_case sysctl_test_cases[] = {
        KUNIT_CASE(sysctl_test_dointvec_write_happy_single_negative),
        KUNIT_CASE(sysctl_test_api_dointvec_write_single_less_int_min),
        KUNIT_CASE(sysctl_test_api_dointvec_write_single_greater_int_max),
+       KUNIT_CASE(sysctl_test_register_sysctl_sz_invalid_extra_value),
        {}
 };
 

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e0b917328cf9963003a3b53a136e2b20fb36461c..c0a1164eaf59a9eca5aeeb284cd79d12c6100963 100644 (file)
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -977,16 +977,10 @@ int proc_dou8vec_minmax(struct ctl_table *table, int write,
        if (table->maxlen != sizeof(u8))
                return -EINVAL;
 
-       if (table->extra1) {
+       if (table->extra1)
                min = *(unsigned int *) table->extra1;
-               if (min > 255U)
-                       return -EINVAL;
-       }
-       if (table->extra2) {
+       if (table->extra2)
                max = *(unsigned int *) table->extra2;
-               if (max > 255U)
-                       return -EINVAL;
-       }
 
        tmp = *table;