From: Wen Yang <[email protected]>
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 boundary checking forward to module loading,
thereby reporting errors in advance, and also adds a kunit test case.
Signed-off-by: Wen Yang <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Joel Granados <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: [email protected]
---
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 | 12 +++++++++++
kernel/sysctl-test.c | 49 +++++++++++++++++++++++++++++++++++++++++++
kernel/sysctl.c | 14 ++++---------
3 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 37cde0efee57..136e3f8966c3 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -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) {
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 81cc974913bb..3efe3a991743 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) {
- 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;
--
2.25.1
On Fri, Apr 05, 2024 at 10:40:59PM +0800, [email protected] wrote:
> From: Wen Yang <[email protected]>
>
Please move the text describing what was done to the top. So you would
start the message like this:
"
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 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:
This reads funny. Please change it to something like this:
"
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.
"
>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/sysctl.h>
Please indent the example code
>
> 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,
> },
> {}
don't include a sentinel in your example.
> };
>
> 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);
> }
This is just an example. Lets remove the cleanup_demo as it does not
give any additional info
>
> pr_info("test: cleanup module.\n");
> }
>
> module_init(init_demo);
> module_exit(cleanup_demo);
Lets remove this "module_exit" line.
> 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
You can do with just including either read or write. No need to include
both.
So the code would end up to something like this:
"
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;
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);
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
"
>
> This patch moves boundary checking forward to module loading,
> thereby reporting errors in advance, and also adds a kunit test case.
>
> Signed-off-by: Wen Yang <[email protected]>
> Cc: Luis Chamberlain <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Joel Granados <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: [email protected]
> ---
> 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 | 12 +++++++++++
> kernel/sysctl-test.c | 49 +++++++++++++++++++++++++++++++++++++++++++
> kernel/sysctl.c | 14 ++++---------
> 3 files changed, 65 insertions(+), 10 deletions(-)
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 37cde0efee57..136e3f8966c3 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -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");
The error message here does not make sense: It should be along the lines
of "Range value to large for proc_dou8vec_minmax"
> + }
> + if (table->extra2) {
> + extra = *(unsigned int *) table->extra2;
> + if (extra > 255U)
> + err |= sysctl_err(path, table, "array not allowed");
Same as previous.
> + }
> }
>
> 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 81cc974913bb..3efe3a991743 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) {
> - 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;
Please leave as (unsigned int *)
> + if (table->extra2)
> + max = *(unsigned char *) table->extra2;
Please leave as (unsigned int *)
>
> tmp = *table;
>
> --
> 2.25.1
>
--
Joel Granados