Messages in this thread | | | Date | Tue, 2 Apr 2024 22:39:03 +0800 | Subject | Re: [PATCH v2] sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array | From | Wen Yang <> |
| |
On 2024/3/29 00:53, Joel Granados wrote: > On Sat, Mar 02, 2024 at 11:27:45PM +0800, wenyang.linux@foxmail.com wrote: >> From: Wen Yang <wenyang.linux@foxmail.com> >> > > <--- snip ---> > >> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c >> index 6ef887c19c48..84e759a8328f 100644 >> --- a/kernel/sysctl-test.c >> +++ b/kernel/sysctl-test.c >> @@ -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[] = { > I'm pretty sure that this is going to clash with the constification that > Thomas is working on. Please re-work this patch knowing that these > ctl_tables are going to have to change to const at some point. > >> + { >> + .procname = "foo", >> + .data = &data, >> + .maxlen = sizeof(u8), >> + .mode = 0644, >> + .proc_handler = proc_dou8vec_minmax, >> + .extra1 = SYSCTL_ZERO, >> + .extra2 = SYSCTL_ONE_THOUSAND, >> + }, >> + {} > Don't use the sentinel here. We are removing them and all new sysctl > tables (even the test ones) should be created without them > >> + }; >> + unsigned int size = ARRAY_SIZE(table); > You do not need size here. When you use register_sysctl, the size will > be automatically calculated for you. > >> + >> + 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)); > Replace all these by register_sysctl please. > Thanks for your review comments w.r.t [PATCH v2], which will be complied by in next patch version [PATCH v3].
-- Best wishes, Wen
>> +} >> + >> 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), >> {} >> }; >> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index f67b39d3d6e5..28888744626a 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 >> >
| |