Re: [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support
From: Kees Cook
Date: Mon Feb 13 2017 - 17:08:10 EST
On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> Add a few initial respective tests for an array:
>
> o Echoing values separated by spaces works
> o Echoing only first elements will set first elements
> o Confirm PAGE_SIZE limit still applies even if an array is used
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> ---
> lib/test_sysctl.c | 13 +++++
> tools/testing/selftests/sysctl/sysctl.sh | 89 ++++++++++++++++++++++++++++++++
> 2 files changed, 102 insertions(+)
>
> diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
> index 1654f41961b7..603c24b2f6cb 100644
> --- a/lib/test_sysctl.c
> +++ b/lib/test_sysctl.c
> @@ -35,6 +35,7 @@ static int i_one_hundred = 100;
> struct test_sysctl_data {
> int int_0001;
> int int_0002;
> + int int_0003[4];
>
> unsigned int uint_0001;
>
> @@ -45,6 +46,11 @@ static struct test_sysctl_data test_data = {
> .int_0001 = 60,
> .int_0002 = 1,
>
> + .int_0003[0] = 0,
> + .int_0003[1] = 1,
> + .int_0003[2] = 2,
> + .int_0003[3] = 3,
> +
> .uint_0001 = 314,
>
> .string_0001 = "(none)",
> @@ -69,6 +75,13 @@ static struct ctl_table test_table[] = {
> .proc_handler = proc_dointvec,
> },
> {
> + .procname = "int_0003",
> + .data = &test_data.int_0003,
> + .maxlen = sizeof(test_data.int_0003),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
> + {
> .procname = "uint_0001",
> .data = &test_data.uint_0001,
> .maxlen = sizeof(unsigned int),
> diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
> index eedfba6f0a57..963d572155b1 100755
> --- a/tools/testing/selftests/sysctl/sysctl.sh
> +++ b/tools/testing/selftests/sysctl/sysctl.sh
> @@ -26,6 +26,7 @@ ALL_TESTS="0001:1:1"
> ALL_TESTS="$ALL_TESTS 0002:1:1"
> ALL_TESTS="$ALL_TESTS 0003:1:1"
> ALL_TESTS="$ALL_TESTS 0004:1:1"
> +ALL_TESTS="$ALL_TESTS 0005:3:1"
>
> test_modprobe()
> {
> @@ -78,6 +79,10 @@ test_reqs()
> echo "$0: You need getconf installed"
> exit 1
> fi
> + if ! which diff 2> /dev/null > /dev/null; then
> + echo "$0: You need diff installed"
> + exit 1
> + fi
> }
>
> function load_req_mod()
> @@ -137,6 +142,12 @@ verify()
> return 0
> }
>
> +verify_diff_w()
> +{
> + echo "$TEST_STR" | diff -w -u - $1 2>&1 > /dev/null
Instead of shell redirection, just use -q ?
> + return $?
> +}
> +
> test_rc()
> {
> if [[ $rc != 0 ]]; then
> @@ -317,6 +328,74 @@ run_limit_digit_int()
> test_rc
> }
>
> +# You used an int array
> +run_limit_digit_int_array()
> +{
> + echo -n "Testing array works as expected ... "
> + TEST_STR="4 3 2 1"
> + echo -n $TEST_STR > $TARGET
> +
> + if ! verify_diff_w "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +
> + echo -n "Testing skipping trailing array elements works ... "
> + # Do not reset_vals, carry on the values from the last test.
> + # If we only echo in two digits the last two are left intact
> + TEST_STR="100 101"
> + echo -n $TEST_STR > $TARGET
> + # After we echo in, to help diff we need to set on TEST_STR what
> + # we expect the result to be.
> + TEST_STR="100 101 2 1"
> +
> + if ! verify_diff_w "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +
> + echo -n "Testing PAGE_SIZE limit on array works ... "
> + # Do not reset_vals, carry on the values from the last test.
> + # Even if you use an int array, you are still restricted to
> + # MAX_DIGITS, this is a known limitation. Test limit works.
> + LIMIT=$((MAX_DIGITS -1))
> + TEST_STR="9"
> + (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
> + dd of="${TARGET}" 2>/dev/null
> +
> + TEST_STR="9 101 2 1"
> + if ! verify_diff_w "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +
> + echo -n "Testing exceeding PAGE_SIZE limit fails as expected ... "
> + # Do not reset_vals, carry on the values from the last test.
> + # Now go over limit.
> + LIMIT=$((MAX_DIGITS))
> + TEST_STR="7"
> + (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
> + dd of="${TARGET}" 2>/dev/null
> +
> + TEST_STR="7 101 2 1"
> + if verify_diff_w "${TARGET}"; then
> + echo "FAIL" >&2
> + rc=1
> + else
> + echo "ok"
> + fi
> + test_rc
> +}
> +
> # You are using an unsigned int
> run_limit_digit_uint()
> {
> @@ -477,6 +556,15 @@ sysctl_test_0004()
> run_limit_digit_uint
> }
>
> +sysctl_test_0005()
> +{
> + TARGET="${SYSCTL}/int_0003"
> + reset_vals
> + ORIG=$(cat "${TARGET}")
> +
> + run_limit_digit_int_array
> +}
> +
> list_tests()
> {
> echo "Test ID list:"
> @@ -489,6 +577,7 @@ list_tests()
> echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
> echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
> echo "0004 x $(get_test_count 0004) - tests proc_douintvec()"
> + echo "0005 x $(get_test_count 0005) - tests proc_douintvec() array"
> }
>
> test_reqs
> --
> 2.11.0
>
I love seeing these tests added. I have one other change I'd like to
add to sysctl, but I haven't had time to make sure it doesn't break
stuff. I haven't been able to prove it to myself, but I think it's
safe; I just need to update the tests to handle it:
http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=sysctl/writes_strict&id=b63a38ca45bd9fb61545ce6ce66093147eb96a7c
It'd need an update for the uint handler...
-Kees
--
Kees Cook
Pixel Security