Messages in this thread Patch in this message | | | From | Wen Yang <> | Subject | [PATCH v4] sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array | Date | Fri, 19 Apr 2024 11:36:39 +0800 |
| |
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 --- v4: - commit log: move the text describing what was done to the top. - commit log: rework the buggy example module - proc_sysctl.c: print error messages that can indicate this specific issue - sysctl.c: leave as (unsigned int *) v3: - kunit: using register_sysctl, and thus unnecessary sentries were removed - kunit: using constant ctl_tables v2: - kunit: detect registration failure with KUNIT_EXPECT_NULL
fs/proc/proc_sysctl.c | 14 +++++++++++++ kernel/sysctl-test.c | 49 +++++++++++++++++++++++++++++++++++++++++++ kernel/sysctl.c | 10 ++------- 3 files changed, 65 insertions(+), 8 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index b1c2c0b82116..62d80f4d77d5 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1093,6 +1093,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) || @@ -1104,6 +1105,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 6ef887c19c48..4e7dcc9187e2 100644 --- 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 e0b917328cf9..c0a1164eaf59 100644 --- 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; -- 2.25.1
| |