Messages in this thread Patch in this message | | | Date | Wed, 17 Apr 2024 15:15:07 +0200 | From | Joel Granados <> | Subject | Re: [PATCH v3] sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array |
| |
On Fri, Apr 05, 2024 at 10:40:59PM +0800, wenyang.linux@foxmail.com wrote: > From: Wen Yang <wenyang.linux@foxmail.com> > 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@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 > --- > 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 [unhandled content-type:application/pgp-signature] | |