Re: [PATCH v3] sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array

From: Joel Granados
Date: Wed Apr 17 2024 - 09:15:25 EST

On Fri, Apr 05, 2024 at 10:40:59PM +0800, wenyang.linux@xxxxxxxxxxx wrote:
> From: Wen Yang <wenyang.linux@xxxxxxxxxxx>
>
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 <wenyang.linux@xxxxxxxxxxx>
> Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Joel Granados <j.granados@xxxxxxxxxxx>
> Cc: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> 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

Attachment: signature.asc
Description: PGP signature