Re: [PATCH] tools/testing/selftests/sysctl: Add pre-check to the value of writes_strict

[Kees Cook]

On Fri, Jun 16, 2017 at 2:28 AM, Orson Zhai <orson.zhai@xxxxxxxxxx> wrote:
> Sysctl test will fail in some items if the value of /proc/sys/kernel
> /sysctrl_writes_strict is 0 as the default value in kernel older than v4.5.
>
> Make this test more robust and compatible with older kernels by checking and
> update sysctrl_writes_strict value and restore it when test is done.
>
> Signed-off-by: Orson Zhai <orson.zhai@xxxxxxxxxx>
> ---
>  tools/testing/selftests/sysctl/common_tests     | 14 ++++++++++++++
>  tools/testing/selftests/sysctl/run_numerictests |  3 +++
>  tools/testing/selftests/sysctl/run_stringtests  |  3 +++
>  3 files changed, 20 insertions(+)
>
> diff --git a/tools/testing/selftests/sysctl/common_tests
> b/tools/testing/selftests/sysctl/common_tests
> index 17d534b1b7b4..f5c5c51d16f3 100644
> --- a/tools/testing/selftests/sysctl/common_tests
> +++ b/tools/testing/selftests/sysctl/common_tests
> @@ -63,6 +63,20 @@ else
>         echo "ok"
>  fi
>
> +echo -n "Checking writes strict setting ... "
> +WRITES_STRICT="${SYSCTL}/kernel/sysctl_writes_strict"
> +if [ ! -e ${WRITES_STRICT} ]; then
> +       echo "FAIL, but skip in case of old kernel" >&2
> +else
> +       val=$(cat ${WRITES_STRICT})

Can you give this a more meaningful name instead of "val". Perhaps "old_strict"?

> +       if [ "$val" = "1" ]; then
> +               echo "ok"
> +       else
> +               echo "FAIL, strict value is 0 but force to 1 to continue" >&2
> +               echo "1" > ${WRITES_STRICT}
> +       fi
> +fi
> +
>  # Now that we've validated the sanity of "set_test" and "set_orig",
>  # we can use those functions to set starting states before running
>  # specific behavioral tests.
> diff --git a/tools/testing/selftests/sysctl/run_numerictests
> b/tools/testing/selftests/sysctl/run_numerictests
> index 8510f93f2d14..c0a98fd82c5c 100755
> --- a/tools/testing/selftests/sysctl/run_numerictests
> +++ b/tools/testing/selftests/sysctl/run_numerictests
> @@ -7,4 +7,7 @@ TEST_STR=$(( $ORIG + 1 ))
>
>  . ./common_tests
>
> +if [ ! -z ${val} ]; then
> +       echo ${val} > ${WRITES_STRICT}
> +fi
>  exit $rc
> diff --git a/tools/testing/selftests/sysctl/run_stringtests
> b/tools/testing/selftests/sysctl/run_stringtests
> index 90a9293d520c..ae98d66a9ec6 100755
> --- a/tools/testing/selftests/sysctl/run_stringtests
> +++ b/tools/testing/selftests/sysctl/run_stringtests
> @@ -74,4 +74,7 @@ else
>         echo "ok"
>  fi
>
> +if [ ! -z ${val} ]; then
> +       echo ${val} > ${WRITES_STRICT}
> +fi
>  exit $rc

And instead of open-coding these, can you instead add an "exit_test"
function to common_tests, which will do this and the exit $rc?

-Kees

-- 
Kees Cook
Pixel Security