From 138303ec6ccbe38611931eeb955a722c6f78ec25 Mon Sep 17 00:00:00 2001 From: Joel Granados Date: Thu, 13 Mar 2025 22:35:25 +0100 Subject: sysctl: move u8 register test to lib/test_sysctl.c 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: 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") Reviewed-by: Kees Cook Signed-off-by: Joel Granados --- lib/test_sysctl.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) (limited to 'lib') diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c index 4249e0cc8aaf..54a22e4b1346 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); -- cgit v1.2.3 From 2bac112eaaf391f190905134cc8e7ffc02dd131c Mon Sep 17 00:00:00 2001 From: Joel Granados Date: Tue, 18 Mar 2025 22:04:28 +0100 Subject: sysctl: call sysctl tests with a for loop 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. Reviewed-by: Kees Cook Signed-off-by: Joel Granados --- lib/test_sysctl.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) (limited to 'lib') diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c index 54a22e4b1346..4b3d56de6269 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; - - err = test_sysctl_setup_node_tests(); - if (err) - goto out; - - err = test_sysctl_run_unregister_nested(); - if (err) - goto out; - - err = test_sysctl_run_register_mount_point(); - if (err) - goto out; - - err = test_sysctl_run_register_empty(); - if (err) - goto out; + int err = 0; + + 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_register_u8_extra(); + for (int i = 0; !err && i < ARRAY_SIZE(func_array); i++) + err = func_array[i](); -out: return err; } module_init(test_sysctl_init); -- cgit v1.2.3 From 23b8bacf154759ed922d25527dda434fbf57436a Mon Sep 17 00:00:00 2001 From: Joel Granados Date: Tue, 18 Mar 2025 22:30:09 +0100 Subject: sysctl: Close test ctl_headers with a for loop 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. Reviewed-by: Kees Cook Signed-off-by: Joel Granados --- lib/test_sysctl.c | 65 +++++++++++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 36 deletions(-) (limited to 'lib') diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c index 4b3d56de6269..c02aa9c868f2 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); -- cgit v1.2.3