Originally introduced to sysctl-test.c by commit b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array"), it has been shown to lead to a panic under certain conditions related to a dangling registration.
This series moves the u8 test to lib/test_sysctl.c where the registration calls are kept and correctly removed on module exit. An additional 0012 test is added to selftests/sysctl/sysctl.sh in order to visualize the registration calls done in test_sysctl.c.
Very much related to adding tests to sysctl, the last two patches of this series reduce the places that need to be changed when tests are added by managing the initialization and closing of sysctl tables with a for loop.
Comments are greatly appreciated
Signed-off-by: Joel Granados joel.granados@kernel.org --- Joel Granados (4): sysctl: move u8 register test to lib/test_sysctl.c sysctl: Add 0012 to test the u8 range check sysctl: call sysctl tests with a for loop sysctl: Close test ctl_headers with a for loop
kernel/sysctl-test.c | 49 ------------ lib/test_sysctl.c | 133 +++++++++++++++++++++---------- tools/testing/selftests/sysctl/sysctl.sh | 30 +++++++ 3 files changed, 122 insertions(+), 90 deletions(-) --- base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6 change-id: 20250321-jag-test_extra_val-40954050a1f6
Best regards,
If the test added in commit b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array") is run as a module, a lingering reference to the module is left behind, and a 'sysctl -a' leads to a panic.
To reproduce CONFIG_KUNIT=y CONFIG_SYSCTL_KUNIT_TEST=m
Then run these commands: modprobe sysctl-test rmmod sysctl-test sysctl -a
The panic varies but generally looks something like this:
BUG: unable to handle page fault for address: ffffa4571c0c7db4 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 100000067 P4D 100000067 PUD 100351067 PMD 114f5e067 PTE 0 Oops: Oops: 0000 [#1] SMP NOPTI ... ... ... RIP: 0010:proc_sys_readdir+0x166/0x2c0 ... ... ... Call Trace: <TASK> iterate_dir+0x6e/0x140 __se_sys_getdents+0x6e/0x100 do_syscall_64+0x70/0x150 entry_SYSCALL_64_after_hwframe+0x76/0x7e
Move the test to lib/test_sysctl.c where the registration reference is handled on module exit
'Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array")'
Signed-off-by: Joel Granados joel.granados@kernel.org --- kernel/sysctl-test.c | 49 -------------------------------------- lib/test_sysctl.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 49 deletions(-)
diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c index eb2842bd055771ee8a72040f96260bdb0e754ad5..92f94ea28957f48893b0d0d947d73001428fecc7 100644 --- a/kernel/sysctl-test.c +++ b/kernel/sysctl-test.c @@ -367,54 +367,6 @@ 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; - const 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, - }, - }; - - const 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, - }, - }; - - const 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), @@ -426,7 +378,6 @@ 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/lib/test_sysctl.c b/lib/test_sysctl.c index 4249e0cc8aaff79d7da03aff81b3f9990e9eaac1..54a22e4b134677e022af05df3c75268e7a4a79e7 100644 --- a/lib/test_sysctl.c +++ b/lib/test_sysctl.c @@ -37,6 +37,7 @@ static struct { struct ctl_table_header *test_h_mnterror; struct ctl_table_header *empty_add; struct ctl_table_header *empty; + struct ctl_table_header *test_u8; } sysctl_test_headers;
struct test_sysctl_data { @@ -239,6 +240,65 @@ static int test_sysctl_run_register_empty(void) return 0; }
+static const struct ctl_table table_u8_over[] = { + { + .procname = "u8_over", + .data = &test_data.uint_0001, + .maxlen = sizeof(u8), + .mode = 0644, + .proc_handler = proc_dou8vec_minmax, + .extra1 = SYSCTL_FOUR, + .extra2 = SYSCTL_ONE_THOUSAND, + }, +}; + +static const struct ctl_table table_u8_under[] = { + { + .procname = "u8_under", + .data = &test_data.uint_0001, + .maxlen = sizeof(u8), + .mode = 0644, + .proc_handler = proc_dou8vec_minmax, + .extra1 = SYSCTL_NEG_ONE, + .extra2 = SYSCTL_ONE_HUNDRED, + }, +}; + +static const struct ctl_table table_u8_valid[] = { + { + .procname = "u8_valid", + .data = &test_data.uint_0001, + .maxlen = sizeof(u8), + .mode = 0644, + .proc_handler = proc_dou8vec_minmax, + .extra1 = SYSCTL_ZERO, + .extra2 = SYSCTL_TWO_HUNDRED, + }, +}; + +static int test_sysctl_register_u8_extra(void) +{ + /* should fail because it's over */ + sysctl_test_headers.test_u8 + = register_sysctl("debug/test_sysctl", table_u8_over); + if (sysctl_test_headers.test_u8) + return -ENOMEM; + + /* should fail because it's under */ + sysctl_test_headers.test_u8 + = register_sysctl("debug/test_sysctl", table_u8_under); + if (sysctl_test_headers.test_u8) + return -ENOMEM; + + /* should not fail because it's valid */ + sysctl_test_headers.test_u8 + = register_sysctl("debug/test_sysctl", table_u8_valid); + if (!sysctl_test_headers.test_u8) + return -ENOMEM; + + return 0; +} + static int __init test_sysctl_init(void) { int err; @@ -256,6 +316,10 @@ static int __init test_sysctl_init(void) goto out;
err = test_sysctl_run_register_empty(); + if (err) + goto out; + + err = test_sysctl_register_u8_extra();
out: return err; @@ -275,6 +339,8 @@ static void __exit test_sysctl_exit(void) unregister_sysctl_table(sysctl_test_headers.empty); if (sysctl_test_headers.empty_add) unregister_sysctl_table(sysctl_test_headers.empty_add); + if (sysctl_test_headers.test_u8) + unregister_sysctl_table(sysctl_test_headers.test_u8); }
module_exit(test_sysctl_exit);
On Fri, Mar 21, 2025 at 01:47:24PM +0100, Joel Granados wrote:
If the test added in commit b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array") is run as a module, a lingering reference to the module is left behind, and a 'sysctl -a' leads to a panic.
To reproduce CONFIG_KUNIT=y CONFIG_SYSCTL_KUNIT_TEST=m
Then run these commands: modprobe sysctl-test rmmod sysctl-test sysctl -a
The panic varies but generally looks something like this:
BUG: unable to handle page fault for address: ffffa4571c0c7db4 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 100000067 P4D 100000067 PUD 100351067 PMD 114f5e067 PTE 0 Oops: Oops: 0000 [#1] SMP NOPTI ... ... ... RIP: 0010:proc_sys_readdir+0x166/0x2c0 ... ... ... Call Trace: <TASK> iterate_dir+0x6e/0x140 __se_sys_getdents+0x6e/0x100 do_syscall_64+0x70/0x150 entry_SYSCALL_64_after_hwframe+0x76/0x7eMove the test to lib/test_sysctl.c where the registration reference is handled on module exit
'Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to
Typoe: drop leading '
sysctl_check_table_array")'
And avoid wrapping this line for the field.
Signed-off-by: Joel Granados joel.granados@kernel.org
Otherwise looks good to me.
Reviewed-by: Kees Cook kees@kernel.org
(And I should note that there is a push to move kunit tests into a "/tests/" subdir, but that's separate from this series.)
On Wed, Apr 09, 2025 at 10:26:56AM -0700, Kees Cook wrote:
On Fri, Mar 21, 2025 at 01:47:24PM +0100, Joel Granados wrote:
If the test added in commit b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array") is run as a module, a lingering reference to the module is left behind, and a 'sysctl -a' leads to a panic.
To reproduce CONFIG_KUNIT=y CONFIG_SYSCTL_KUNIT_TEST=m
Then run these commands: modprobe sysctl-test rmmod sysctl-test sysctl -a
The panic varies but generally looks something like this:
BUG: unable to handle page fault for address: ffffa4571c0c7db4 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 100000067 P4D 100000067 PUD 100351067 PMD 114f5e067 PTE 0 Oops: Oops: 0000 [#1] SMP NOPTI ... ... ... RIP: 0010:proc_sys_readdir+0x166/0x2c0 ... ... ... Call Trace: <TASK> iterate_dir+0x6e/0x140 __se_sys_getdents+0x6e/0x100 do_syscall_64+0x70/0x150 entry_SYSCALL_64_after_hwframe+0x76/0x7eMove the test to lib/test_sysctl.c where the registration reference is handled on module exit
'Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to
Typoe: drop leading '
sysctl_check_table_array")'
And avoid wrapping this line for the field.
Signed-off-by: Joel Granados joel.granados@kernel.org
Otherwise looks good to me.
Thx for the feedback; Changed this and took in your trailers, but wont resend.
Best
Add a sysctl test that uses the new u8 test ctl files in a created by the sysctl test module. Check that the u8 proc file that is valid is created and that there are two messages in dmesg for the files that were out of range.
Signed-off-by: Joel Granados joel.granados@kernel.org --- tools/testing/selftests/sysctl/sysctl.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh index 84472b436c07faf43e555999951e554f8e1a60c5..22b53c263dd4525ee8d4655272d6842e5249a7dc 100755 --- a/tools/testing/selftests/sysctl/sysctl.sh +++ b/tools/testing/selftests/sysctl/sysctl.sh @@ -36,6 +36,7 @@ ALL_TESTS="$ALL_TESTS 0008:1:1:match_int:1" ALL_TESTS="$ALL_TESTS 0009:1:1:unregister_error:0" ALL_TESTS="$ALL_TESTS 0010:1:1:mnt/mnt_error:0" ALL_TESTS="$ALL_TESTS 0011:1:1:empty_add:0" +ALL_TESTS="$ALL_TESTS 0012:1:1:u8_valid:0"
function allow_user_defaults() { @@ -851,6 +852,34 @@ sysctl_test_0011() return 0 }
+sysctl_test_0012() +{ + TARGET="${SYSCTL}/$(get_test_target 0012)" + echo -n "Testing u8 range check in sysctl table check in ${TARGET} ... " + if [ ! -f ${TARGET} ]; then + echo -e "FAIL\nCould not create ${TARGET}" >&2 + rc=1 + test_rc + fi + + local u8over_msg=$(dmesg | grep "u8_over range value" | wc -l) + if [ ! ${u8over_msg} -eq 1 ]; then + echo -e "FAIL\nu8 overflow not detected" >&2 + rc=1 + test_rc + fi + + local u8under_msg=$(dmesg | grep "u8_under range value" | wc -l) + if [ ! ${u8under_msg} -eq 1 ]; then + echo -e "FAIL\nu8 underflow not detected" >&2 + rc=1 + test_rc + fi + + echo "OK" + return 0 +} + list_tests() { echo "Test ID list:" @@ -870,6 +899,7 @@ list_tests() echo "0009 x $(get_test_count 0009) - tests sysct unregister" echo "0010 x $(get_test_count 0010) - tests sysct mount point" echo "0011 x $(get_test_count 0011) - tests empty directories" + echo "0012 x $(get_test_count 0012) - tests range check for u8 proc_handler" }
usage()
On Fri, Mar 21, 2025 at 01:47:25PM +0100, Joel Granados wrote:
Add a sysctl test that uses the new u8 test ctl files in a created by the sysctl test module. Check that the u8 proc file that is valid is created and that there are two messages in dmesg for the files that were out of range.
Signed-off-by: Joel Granados joel.granados@kernel.org
Reviewed-by: Kees Cook kees@kernel.org
As we add more test functions in lib/tests_sysctl the main test function (test_sysctl_init) grows. Condense the logic to make it easier to add/remove tests.
Signed-off-by: Joel Granados joel.granados@kernel.org --- lib/test_sysctl.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c index 54a22e4b134677e022af05df3c75268e7a4a79e7..4b3d56de6269b93220ecbeb3d3d4e42944b0ca78 100644 --- a/lib/test_sysctl.c +++ b/lib/test_sysctl.c @@ -301,27 +301,19 @@ static int test_sysctl_register_u8_extra(void)
static int __init test_sysctl_init(void) { - int err; + int err = 0;
- err = test_sysctl_setup_node_tests(); - if (err) - goto out; + int (*func_array[])(void) = { + test_sysctl_setup_node_tests, + test_sysctl_run_unregister_nested, + test_sysctl_run_register_mount_point, + test_sysctl_run_register_empty, + test_sysctl_register_u8_extra + };
- err = test_sysctl_run_unregister_nested(); - if (err) - goto out; + for (int i = 0; !err && i < ARRAY_SIZE(func_array); i++) + err = func_array[i]();
- err = test_sysctl_run_register_mount_point(); - if (err) - goto out; - - err = test_sysctl_run_register_empty(); - if (err) - goto out; - - err = test_sysctl_register_u8_extra(); - -out: return err; } module_init(test_sysctl_init);
On Fri, Mar 21, 2025 at 01:47:26PM +0100, Joel Granados wrote:
As we add more test functions in lib/tests_sysctl the main test function (test_sysctl_init) grows. Condense the logic to make it easier to add/remove tests.
Signed-off-by: Joel Granados joel.granados@kernel.org
Nice cleanup!
Reviewed-by: Kees Cook kees@kernel.org
As more tests are added, the exit function gets longer than it should be. Condense the un-register calls into a for loop to make it easier to add/remove tests.
Signed-off-by: Joel Granados joel.granados@kernel.org --- lib/test_sysctl.c | 65 +++++++++++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 36 deletions(-)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c index 4b3d56de6269b93220ecbeb3d3d4e42944b0ca78..c02aa9c868f2117606b24f114326bf1c396cd584 100644 --- a/lib/test_sysctl.c +++ b/lib/test_sysctl.c @@ -30,16 +30,17 @@ static int i_zero; static int i_one_hundred = 100; static int match_int_ok = 1;
+enum { + TEST_H_SETUP_NODE, + TEST_H_MNT, + TEST_H_MNTERROR, + TEST_H_EMPTY_ADD, + TEST_H_EMPTY, + TEST_H_U8, + TEST_H_SIZE /* Always at the end */ +};
-static struct { - struct ctl_table_header *test_h_setup_node; - struct ctl_table_header *test_h_mnt; - struct ctl_table_header *test_h_mnterror; - struct ctl_table_header *empty_add; - struct ctl_table_header *empty; - struct ctl_table_header *test_u8; -} sysctl_test_headers; - +static struct ctl_table_header *ctl_headers[TEST_H_SIZE] = {}; struct test_sysctl_data { int int_0001; int int_0002; @@ -168,8 +169,8 @@ static int test_sysctl_setup_node_tests(void) test_data.bitmap_0001 = kzalloc(SYSCTL_TEST_BITMAP_SIZE/8, GFP_KERNEL); if (!test_data.bitmap_0001) return -ENOMEM; - sysctl_test_headers.test_h_setup_node = register_sysctl("debug/test_sysctl", test_table); - if (!sysctl_test_headers.test_h_setup_node) { + ctl_headers[TEST_H_SETUP_NODE] = register_sysctl("debug/test_sysctl", test_table); + if (!ctl_headers[TEST_H_SETUP_NODE]) { kfree(test_data.bitmap_0001); return -ENOMEM; } @@ -203,12 +204,12 @@ static int test_sysctl_run_unregister_nested(void)
static int test_sysctl_run_register_mount_point(void) { - sysctl_test_headers.test_h_mnt + ctl_headers[TEST_H_MNT] = register_sysctl_mount_point("debug/test_sysctl/mnt"); - if (!sysctl_test_headers.test_h_mnt) + if (!ctl_headers[TEST_H_MNT]) return -ENOMEM;
- sysctl_test_headers.test_h_mnterror + ctl_headers[TEST_H_MNTERROR] = register_sysctl("debug/test_sysctl/mnt/mnt_error", test_table_unregister); /* @@ -226,15 +227,15 @@ static const struct ctl_table test_table_empty[] = { }; static int test_sysctl_run_register_empty(void) { /* Tets that an empty dir can be created */ - sysctl_test_headers.empty_add + ctl_headers[TEST_H_EMPTY_ADD] = register_sysctl("debug/test_sysctl/empty_add", test_table_empty); - if (!sysctl_test_headers.empty_add) + if (!ctl_headers[TEST_H_EMPTY_ADD]) return -ENOMEM;
/* Test that register on top of an empty dir works */ - sysctl_test_headers.empty + ctl_headers[TEST_H_EMPTY] = register_sysctl("debug/test_sysctl/empty_add/empty", test_table_empty); - if (!sysctl_test_headers.empty) + if (!ctl_headers[TEST_H_EMPTY]) return -ENOMEM;
return 0; @@ -279,21 +280,21 @@ static const struct ctl_table table_u8_valid[] = { static int test_sysctl_register_u8_extra(void) { /* should fail because it's over */ - sysctl_test_headers.test_u8 + ctl_headers[TEST_H_U8] = register_sysctl("debug/test_sysctl", table_u8_over); - if (sysctl_test_headers.test_u8) + if (ctl_headers[TEST_H_U8]) return -ENOMEM;
/* should fail because it's under */ - sysctl_test_headers.test_u8 + ctl_headers[TEST_H_U8] = register_sysctl("debug/test_sysctl", table_u8_under); - if (sysctl_test_headers.test_u8) + if (ctl_headers[TEST_H_U8]) return -ENOMEM;
/* should not fail because it's valid */ - sysctl_test_headers.test_u8 + ctl_headers[TEST_H_U8] = register_sysctl("debug/test_sysctl", table_u8_valid); - if (!sysctl_test_headers.test_u8) + if (!ctl_headers[TEST_H_U8]) return -ENOMEM;
return 0; @@ -321,18 +322,10 @@ module_init(test_sysctl_init); static void __exit test_sysctl_exit(void) { kfree(test_data.bitmap_0001); - if (sysctl_test_headers.test_h_setup_node) - unregister_sysctl_table(sysctl_test_headers.test_h_setup_node); - if (sysctl_test_headers.test_h_mnt) - unregister_sysctl_table(sysctl_test_headers.test_h_mnt); - if (sysctl_test_headers.test_h_mnterror) - unregister_sysctl_table(sysctl_test_headers.test_h_mnterror); - if (sysctl_test_headers.empty) - unregister_sysctl_table(sysctl_test_headers.empty); - if (sysctl_test_headers.empty_add) - unregister_sysctl_table(sysctl_test_headers.empty_add); - if (sysctl_test_headers.test_u8) - unregister_sysctl_table(sysctl_test_headers.test_u8); + for (int i = 0; i < TEST_H_SIZE; i++) { + if (ctl_headers[i]) + unregister_sysctl_table(ctl_headers[i]); + } }
module_exit(test_sysctl_exit);
On Fri, Mar 21, 2025 at 01:47:27PM +0100, Joel Granados wrote:
As more tests are added, the exit function gets longer than it should be. Condense the un-register calls into a for loop to make it easier to add/remove tests.
Signed-off-by: Joel Granados joel.granados@kernel.org
Much cleaner too. :)
Reviewed-by: Kees Cook kees@kernel.org
linux-kselftest-mirror@lists.linaro.org
-
Joel Granados -
Kees Cook