[v2] sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array
Commit Message
From: Wen Yang <wenyang.linux@foxmail.com>
The boundary check of u8's extra is currently performed at runtime.
This may result in some kernel modules that can be loaded normally without
any errors, but not works, as follows:
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/sysctl.h>
static struct ctl_table_header *_table_header;
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) {
if (!_table_header)
_table_header = register_sysctl("kernel", table);
pr_info("test: init module.\n");
return 0;
}
static void cleanup_demo(void) {
if (_table_header) {
unregister_sysctl_table(_table_header);
}
pr_info("test: cleanup module.\n");
}
module_init(init_demo);
module_exit(cleanup_demo);
MODULE_LICENSE("GPL");
# insmod test.ko
# cat /proc/sys/kernel/foo
cat: /proc/sys/kernel/foo: Invalid argument
# echo 1 > /proc/sys/kernel/foo
-bash: echo: write error: Invalid argument
This patch moves the boundary check forward to module loading and
also adds a kunit test case.
Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
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
---
v2->v1:
- kunit: detect registration failure with KUNIT_EXPECT_NULL
fs/proc/proc_sysctl.c | 12 ++++++++++++
kernel/sysctl-test.c | 30 ++++++++++++++++++++++++++++++
kernel/sysctl.c | 14 ++++----------
3 files changed, 46 insertions(+), 10 deletions(-)
@@ -1096,6 +1096,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) ||
@@ -1107,6 +1108,17 @@ 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, "array not allowed");
+ }
+ if (table->extra2) {
+ extra = *(unsigned int *) table->extra2;
+ if (extra > 255U)
+ err |= sysctl_err(path, table, "array not allowed");
+ }
}
if (table->proc_handler == proc_dobool) {
@@ -367,6 +367,35 @@ 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[] = {
+ {
+ .procname = "foo",
+ .data = &data,
+ .maxlen = sizeof(u8),
+ .mode = 0644,
+ .proc_handler = proc_dou8vec_minmax,
+ .extra1 = SYSCTL_ZERO,
+ .extra2 = SYSCTL_ONE_THOUSAND,
+ },
+ {}
+ };
+ unsigned int size = ARRAY_SIZE(table);
+
+ KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
+ table[0].extra1 = SYSCTL_ONE_THOUSAND;
+ KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
+ table[0].extra1 = SYSCTL_ONE_HUNDRED;
+ table[0].extra2 = SYSCTL_TWO_HUNDRED;
+ KUNIT_EXPECT_NOT_NULL(test, register_sysctl_sz("foo", table, size));
+}
+
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 +407,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),
{}
};
@@ -977,16 +977,10 @@ int proc_dou8vec_minmax(struct ctl_table *table, int write,
if (table->maxlen != sizeof(u8))
return -EINVAL;
- if (table->extra1) {
- min = *(unsigned int *) table->extra1;
- if (min > 255U)
- return -EINVAL;
- }
- if (table->extra2) {
- max = *(unsigned int *) table->extra2;
- if (max > 255U)
- return -EINVAL;
- }
+ if (table->extra1)
+ min = *(unsigned char *) table->extra1;
+ if (table->extra2)
+ max = *(unsigned char *) table->extra2;
tmp = *table;