Fix an out of bounds access reported in https://lore.kernel.org/oe-lkp/202311201431.57aae8f3-oliver.sang@intel.com Make sure that the ctl_table header size is greater than 0 before evaluating if a ctl_table is permanently empty; this evaluation accesses the first element regardless of the size. Adjusted the ctl_table_size of Permanently empty ctl_table registers to 1 as they the check now requires them to have size greater than 0. Added tests for empty directory handling; in response to the path followed by empty ctl_tables changing slightly. Clarified the results of sysctl self tests to more easily identify which ones are OK, Skipped and Failed.
Comments are greatly appreciated
Best
Signed-off-by: Joel Granados j.granados@samsung.com --- Joel Granados (3): sysctl: Fix out of bounds access for empty sysctl registers sysctl: Add a selftest for handling empty dirs sysclt: Clarify the results of selftest run
fs/proc/proc_sysctl.c | 9 +- lib/test_sysctl.c | 29 ++++++ tools/testing/selftests/sysctl/sysctl.sh | 146 ++++++++++++++++++------------- 3 files changed, 121 insertions(+), 63 deletions(-) --- base-commit: 8b793bcda61f6c3ed4f5b2ded7530ef6749580cb change-id: 20231121-jag-fix_out_of_bounds_insert-86380d1bc95e
Best regards,
From: Joel Granados j.granados@samsung.com
When registering tables to the sysctl subsystem there is a check to see if header is a permanently empty directory (used for mounts). This check evaluates the first element of the ctl_table. This results in an out of bounds evaluation when registering empty directories.
The function register_sysctl_mount_point now passes a ctl_table of size 1 instead of size 0. It now relies solely on the type to identify a permanently empty register.
Make sure that the ctl_table has at least one element before testing for permanent emptiness.
Signed-off-by: Joel Granados j.granados@samsung.com Reported-by: kernel test robot oliver.sang@intel.com Closes: https://lore.kernel.org/oe-lkp/202311201431.57aae8f3-oliver.sang@intel.com --- fs/proc/proc_sysctl.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index de484195f49f..5b5cdc747cef 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -44,7 +44,7 @@ static struct ctl_table sysctl_mount_point[] = { */ struct ctl_table_header *register_sysctl_mount_point(const char *path) { - return register_sysctl_sz(path, sysctl_mount_point, 0); + return register_sysctl(path, sysctl_mount_point); } EXPORT_SYMBOL(register_sysctl_mount_point);
@@ -233,7 +233,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) return -EROFS;
/* Am I creating a permanently empty directory? */ - if (sysctl_is_perm_empty_ctl_table(header->ctl_table)) { + if (header->ctl_table_size > 0 && + sysctl_is_perm_empty_ctl_table(header->ctl_table)) { if (!RB_EMPTY_ROOT(&dir->root)) return -EINVAL; sysctl_set_perm_empty_ctl_header(dir_h); @@ -1213,6 +1214,10 @@ static bool get_links(struct ctl_dir *dir, struct ctl_table_header *tmp_head; struct ctl_table *entry, *link;
+ if (header->ctl_table_size == 0 || + sysctl_is_perm_empty_ctl_table(header->ctl_table)) + return true; + /* Are there links available for every entry in table? */ list_for_each_table_entry(entry, header) { const char *procname = entry->procname;
From: Joel Granados j.granados@samsung.com
Basic test to ensure that empty directories can be registered and that they in turn can serve as a base dir for other registrations.
Add one test to the sysctl selftest module. It first registers an empty directory under "empty_add" and then uses that as a base to register another empty dir. The sysctl bash script then checks that "empty_add" is present and that there an empty directory within it.
Signed-off-by: Joel Granados j.granados@samsung.com --- lib/test_sysctl.c | 29 +++++++++++++++++++++++++++++ tools/testing/selftests/sysctl/sysctl.sh | 23 +++++++++++++++++++++++ 2 files changed, 52 insertions(+)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c index 8036aa91a1cb..730908bc1c2b 100644 --- a/lib/test_sysctl.c +++ b/lib/test_sysctl.c @@ -35,6 +35,8 @@ 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; } sysctl_test_headers;
struct test_sysctl_data { @@ -220,6 +222,25 @@ static int test_sysctl_run_register_mount_point(void) return 0; }
+static 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 + = register_sysctl("debug/test_sysctl/empty_add", test_table_empty); + if (!sysctl_test_headers.empty_add) + return -ENOMEM; + + /* Test that register on top of an empty dir works */ + sysctl_test_headers.empty + = register_sysctl("debug/test_sysctl/empty_add/empty", test_table_empty); + if (!sysctl_test_headers.empty) + return -ENOMEM; + + return 0; +} + static int __init test_sysctl_init(void) { int err; @@ -233,6 +254,10 @@ static int __init test_sysctl_init(void) goto out;
err = test_sysctl_run_register_mount_point(); + if (err) + goto out; + + err = test_sysctl_run_register_empty();
out: return err; @@ -248,6 +273,10 @@ static void __exit test_sysctl_exit(void) 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); }
module_exit(test_sysctl_exit); diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh index 444b2befda82..e956d2c3b7e9 100755 --- a/tools/testing/selftests/sysctl/sysctl.sh +++ b/tools/testing/selftests/sysctl/sysctl.sh @@ -35,6 +35,7 @@ ALL_TESTS="$ALL_TESTS 0007:1:1:boot_int:1" 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"
function allow_user_defaults() { @@ -828,6 +829,27 @@ sysctl_test_0010() return 0 }
+sysctl_test_0011() +{ + TARGET="${SYSCTL}/$(get_test_target 0011)" + echo -n "Testing empty dir handling in ${TARGET} ... " + if [ ! -d ${TARGET} ]; then + echo -e "FAIL\nCould not create ${TARGET}" >&2 + rc=1 + test_rc + fi + + TARGET2="${TARGET}/empty" + if [ ! -d ${TARGET2} ]; then + echo -e "FAIL\nCould not create ${TARGET2}" >&2 + rc=1 + test_rc + fi + + echo "OK" + return 0 +} + list_tests() { echo "Test ID list:" @@ -846,6 +868,7 @@ list_tests() echo "0008 x $(get_test_count 0008) - tests sysctl macro values match" 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" }
usage()
From: Joel Granados j.granados@samsung.com
In some cases the result of test were hidden inside the stdout and it was difficult to identify when a test was skipped and why.
List of changes 1. Capitalize all the words that express a test result : "OK", "SKIPPED" and "FAIL". 2. Place all test result text at the end of the message. This will prevent the result from being hidden when stdout is verbose. 3. Any other explanation that comes after the result text will be placed in a new line. 4. All failures are marked as "FAIL" 5. Pipped the failure to stderr in tests 8, 9, 10. 6. Replaced bogus "FAIL" with "SKIPPED" in test 0007 7. All "..." are prefixed and followed by a space.
Signed-off-by: Joel Granados j.granados@samsung.com --- tools/testing/selftests/sysctl/sysctl.sh | 123 ++++++++++++++++--------------- 1 file changed, 62 insertions(+), 61 deletions(-)
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh index e956d2c3b7e9..84472b436c07 100755 --- a/tools/testing/selftests/sysctl/sysctl.sh +++ b/tools/testing/selftests/sysctl/sysctl.sh @@ -64,7 +64,7 @@ function check_production_sysctl_writes_strict() else old_strict=$(cat ${WRITES_STRICT}) if [ "$old_strict" = "1" ]; then - echo "ok" + echo "OK" else echo "FAIL, strict value is 0 but force to 1 to continue" >&2 echo "1" > ${WRITES_STRICT} @@ -226,7 +226,7 @@ run_numerictests() echo "FAIL" >&2 exit 1 else - echo "ok" + echo "OK" fi
echo -n "Checking sysctl is not set to test value ... " @@ -234,7 +234,7 @@ run_numerictests() echo "FAIL" >&2 exit 1 else - echo "ok" + echo "OK" fi
echo -n "Writing sysctl from shell ... " @@ -243,7 +243,7 @@ run_numerictests() echo "FAIL" >&2 exit 1 else - echo "ok" + echo "OK" fi
echo -n "Resetting sysctl to original value ... " @@ -252,7 +252,7 @@ run_numerictests() echo "FAIL" >&2 exit 1 else - echo "ok" + echo "OK" fi
# Now that we've validated the sanity of "set_test" and "set_orig", @@ -266,7 +266,7 @@ run_numerictests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
echo -n "Writing middle of sysctl after synchronized seek ... " @@ -276,7 +276,7 @@ run_numerictests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
echo -n "Writing beyond end of sysctl ... " @@ -286,7 +286,7 @@ run_numerictests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
echo -n "Writing sysctl with multiple long writes ... " @@ -297,14 +297,14 @@ run_numerictests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc }
check_failure() { - echo -n "Testing that $1 fails as expected..." + echo -n "Testing that $1 fails as expected ... " reset_vals TEST_STR="$1" orig="$(cat $TARGET)" @@ -315,7 +315,7 @@ check_failure() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc } @@ -357,7 +357,7 @@ run_wideint_tests() # Your test must accept digits 3 and 4 to use this run_limit_digit() { - echo -n "Checking ignoring spaces up to PAGE_SIZE works on write ..." + echo -n "Checking ignoring spaces up to PAGE_SIZE works on write ... " reset_vals
LIMIT=$((MAX_DIGITS -1)) @@ -369,11 +369,11 @@ run_limit_digit() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc
- echo -n "Checking passing PAGE_SIZE of spaces fails on write ..." + echo -n "Checking passing PAGE_SIZE of spaces fails on write ... " reset_vals
LIMIT=$((MAX_DIGITS)) @@ -385,7 +385,7 @@ run_limit_digit() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc } @@ -393,7 +393,7 @@ run_limit_digit() # You are using an int run_limit_digit_int() { - echo -n "Testing INT_MAX works ..." + echo -n "Testing INT_MAX works ... " reset_vals TEST_STR="$INT_MAX" echo -n $TEST_STR > $TARGET @@ -402,11 +402,11 @@ run_limit_digit_int() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc
- echo -n "Testing INT_MAX + 1 will fail as expected..." + echo -n "Testing INT_MAX + 1 will fail as expected ... " reset_vals let TEST_STR=$INT_MAX+1 echo -n $TEST_STR > $TARGET 2> /dev/null @@ -415,11 +415,11 @@ run_limit_digit_int() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc
- echo -n "Testing negative values will work as expected..." + echo -n "Testing negative values will work as expected ... " reset_vals TEST_STR="-3" echo -n $TEST_STR > $TARGET 2> /dev/null @@ -427,7 +427,7 @@ run_limit_digit_int() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc } @@ -443,7 +443,7 @@ run_limit_digit_int_array() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc
@@ -460,7 +460,7 @@ run_limit_digit_int_array() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc
@@ -478,7 +478,7 @@ run_limit_digit_int_array() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc
@@ -495,7 +495,7 @@ run_limit_digit_int_array() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc } @@ -503,7 +503,7 @@ run_limit_digit_int_array() # You are using an unsigned int run_limit_digit_uint() { - echo -n "Testing UINT_MAX works ..." + echo -n "Testing UINT_MAX works ... " reset_vals TEST_STR="$UINT_MAX" echo -n $TEST_STR > $TARGET @@ -512,11 +512,11 @@ run_limit_digit_uint() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc
- echo -n "Testing UINT_MAX + 1 will fail as expected..." + echo -n "Testing UINT_MAX + 1 will fail as expected ... " reset_vals TEST_STR=$(($UINT_MAX+1)) echo -n $TEST_STR > $TARGET 2> /dev/null @@ -525,11 +525,11 @@ run_limit_digit_uint() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc
- echo -n "Testing negative values will not work as expected ..." + echo -n "Testing negative values will not work as expected ... " reset_vals TEST_STR="-3" echo -n $TEST_STR > $TARGET 2> /dev/null @@ -538,7 +538,7 @@ run_limit_digit_uint() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi test_rc } @@ -552,7 +552,7 @@ run_stringtests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
echo -n "Writing middle of sysctl after unsynchronized seek ... " @@ -562,7 +562,7 @@ run_stringtests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
echo -n "Checking sysctl maxlen is at least $MAXLEN ... " @@ -573,7 +573,7 @@ run_stringtests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
echo -n "Checking sysctl keeps original string on overflow append ... " @@ -584,7 +584,7 @@ run_stringtests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
echo -n "Checking sysctl stays NULL terminated on write ... " @@ -595,7 +595,7 @@ run_stringtests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
echo -n "Checking sysctl stays NULL terminated on overwrite ... " @@ -606,7 +606,7 @@ run_stringtests() echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" fi
test_rc @@ -651,7 +651,7 @@ run_bitmaptest() { fi done
- echo -n "Checking bitmap handler... " + echo -n "Checking bitmap handler ... " TEST_FILE=$(mktemp) echo -n "$TEST_STR" > $TEST_FILE
@@ -666,7 +666,7 @@ run_bitmaptest() { echo "FAIL" >&2 rc=1 else - echo "ok" + echo "OK" rc=0 fi test_rc @@ -743,89 +743,90 @@ sysctl_test_0006() sysctl_test_0007() { TARGET="${SYSCTL}/$(get_test_target 0007)" + echo -n "Testing if $TARGET is set to 1 ... " + if [ ! -f $TARGET ]; then - echo "Skipping test for $TARGET as it is not present ..." + echo -e "SKIPPING\n$TARGET is not present" return $ksft_skip fi
if [ -d $DIR ]; then - echo "Boot param test only possible sysctl_test is built-in, not module:" + echo -e "SKIPPING\nTest only possible if sysctl_test is built-in, not module:" cat $TEST_DIR/config >&2 return $ksft_skip fi
- echo -n "Testing if $TARGET is set to 1 ..." ORIG=$(cat "${TARGET}")
if [ x$ORIG = "x1" ]; then - echo "ok" + echo "OK" return 0 fi - echo "FAIL" - echo "Checking if /proc/cmdline contains setting of the expected parameter ..." + if [ ! -f /proc/cmdline ]; then - echo "/proc/cmdline does not exist, test inconclusive" - return 0 + echo -e "SKIPPING\nThere is no /proc/cmdline to check for paramter" + return $ksft_skip fi
FOUND=$(grep -c "sysctl[./]debug[./]test_sysctl[./]boot_int=1" /proc/cmdline) if [ $FOUND = "1" ]; then - echo "Kernel param found but $TARGET is not 1, TEST FAILED" + echo -e "FAIL\nKernel param found but $TARGET is not 1." >&2 rc=1 test_rc fi
- echo "Skipping test, expected kernel parameter missing." - echo "To perform this test, make sure kernel is booted with parameter: sysctl.debug.test_sysctl.boot_int=1" + echo -e "SKIPPING\nExpected kernel parameter missing." + echo "Kernel must be booted with parameter: sysctl.debug.test_sysctl.boot_int=1" return $ksft_skip }
sysctl_test_0008() { TARGET="${SYSCTL}/$(get_test_target 0008)" + echo -n "Testing if $TARGET is matched in kernel ... " + if [ ! -f $TARGET ]; then - echo "Skipping test for $TARGET as it is not present ..." + echo -e "SKIPPING\n$TARGET is not present" return $ksft_skip fi
- echo -n "Testing if $TARGET is matched in kernel" ORIG_VALUE=$(cat "${TARGET}")
if [ $ORIG_VALUE -ne 1 ]; then - echo "TEST FAILED" + echo "FAIL" >&2 rc=1 test_rc fi
- echo "ok" + echo "OK" return 0 }
sysctl_test_0009() { TARGET="${SYSCTL}/$(get_test_target 0009)" - echo -n "Testing if $TARGET unregistered correctly ..." + echo -n "Testing if $TARGET unregistered correctly ... " if [ -d $TARGET ]; then - echo "TEST FAILED" + echo "FAIL" >&2 rc=1 test_rc fi
- echo "ok" + echo "OK" return 0 }
sysctl_test_0010() { TARGET="${SYSCTL}/$(get_test_target 0010)" - echo -n "Testing that $TARGET was not created ..." + echo -n "Testing that $TARGET was not created ... " if [ -d $TARGET ]; then - echo "TEST FAILED" + echo "FAIL" >&2 rc=1 test_rc fi
- echo "ok" + echo "OK" return 0 }
@@ -957,7 +958,7 @@ function skip_test() if target_exists $TEST_TARGET $TEST_ID; then TEST_SKIP=$(get_test_skip_no_target $TEST_ID) if [[ $TEST_SKIP -eq "1" ]]; then - echo "Target for test $TEST_ID: $TEST_TARGET not exist, skipping test ..." + echo "Target $TEST_TARGET for test $TEST_ID does not exist ... SKIPPING" return 0 fi fi
On Tue, Nov 21, 2023 at 12:02:17PM +0100, Joel Granados via B4 Relay wrote:
Fix an out of bounds access reported in https://lore.kernel.org/oe-lkp/202311201431.57aae8f3-oliver.sang@intel.com Make sure that the ctl_table header size is greater than 0 before evaluating if a ctl_table is permanently empty; this evaluation accesses the first element regardless of the size. Adjusted the ctl_table_size of Permanently empty ctl_table registers to 1 as they the check now requires them to have size greater than 0. Added tests for empty directory handling; in response to the path followed by empty ctl_tables changing slightly. Clarified the results of sysctl self tests to more easily identify which ones are OK, Skipped and Failed.
Comments are greatly appreciated
Applied, thanks!
Luis
linux-kselftest-mirror@lists.linaro.org
-
Joel Granados via B4 Relay -
Luis Chamberlain