Re: [PATCH v2 4/9] test_sysctl: add dedicated proc sysctl test driver

[Kees Cook]

On Fri, Feb 10, 2017 at 4:36 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> Although we have had tools/testing/selftests/sysctl/ with two
> test cases these use existing kernel sysctl interfaces. We want
> to expand test coverage, we can't just be looking for random
> safe production values to poke at, instead just dedicate a test
> driver for debugging purposes and port the existing scripts to
> use it. This will make it easier for further tests to be added.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>

Yes please!

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

> ---
>  lib/Kconfig.debug                               |  11 +++
>  lib/Makefile                                    |   1 +
>  lib/test_sysctl.c                               | 106 ++++++++++++++++++++++++
>  tools/testing/selftests/sysctl/config           |   1 +
>  tools/testing/selftests/sysctl/run_numerictests |   4 +-
>  tools/testing/selftests/sysctl/run_stringtests  |   4 +-
>  6 files changed, 123 insertions(+), 4 deletions(-)
>  create mode 100644 lib/test_sysctl.c
>  create mode 100644 tools/testing/selftests/sysctl/config
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 64c03b07ad2f..d753fac41f78 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1975,6 +1975,17 @@ config TEST_FIRMWARE
>
>           If unsure, say N.
>
> +config TEST_SYSCTL
> +       tristate "sysctl test driver"
> +       default n
> +       depends on PROC_SYSCTL
> +       help
> +         This builds the "test_sysctl" module. This driver enables to test the
> +         proc sysctl interfaces available to drivers safely without affecting
> +         production knobs which might alter system functionality.
> +
> +         If unsure, say N.
> +
>  config TEST_UDELAY
>         tristate "udelay test driver"
>         default n
> diff --git a/lib/Makefile b/lib/Makefile
> index 445a39c21f46..ac832c440d41 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
>  obj-y += kstrtox.o
>  obj-$(CONFIG_TEST_BPF) += test_bpf.o
>  obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
> +obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
>  obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
>  obj-$(CONFIG_TEST_KASAN) += test_kasan.o
>  obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
> diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
> new file mode 100644
> index 000000000000..9b9ae1a95ab3
> --- /dev/null
> +++ b/lib/test_sysctl.c
> @@ -0,0 +1,106 @@
> +/*
> + * proc sysctl test driver
> + *
> + * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of copyleft-next (version 0.3.1 or later) as published
> + * at http://copyleft-next.org/.
> + */
> +
> +/*
> + * This module provides an interface to the the proc sysctl interfaces.  This
> + * driver requires CONFIG_PROC_SYSCTL. It will not normally be loaded by the
> + * system unless explicitly requested by name. You can also build this driver
> + * into your kernel.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/async.h>
> +#include <linux/delay.h>
> +#include <linux/vmalloc.h>
> +
> +static int i_zero;
> +static int i_one_hundred = 100;
> +
> +struct test_sysctl_data {
> +       int int_0001;
> +       char string_0001[65];

I wonder if it's worth adding a comment here that "65" is tied to the
sysctl string self-test.

> +};
> +
> +static struct test_sysctl_data test_data = {
> +       .int_0001 = 60,
> +       .string_0001 = "(none)",
> +};
> +
> +/* These are all under /proc/sys/debug/test_sysctl/ */
> +static struct ctl_table test_table[] = {
> +       {
> +               .procname       = "int_0001",
> +               .data           = &test_data.int_0001,
> +               .maxlen         = sizeof(int),
> +               .mode           = 0644,
> +               .proc_handler   = proc_dointvec_minmax,
> +               .extra1         = &i_zero,
> +               .extra2         = &i_one_hundred,
> +       },
> +       {
> +               .procname       = "string_0001",
> +               .data           = &test_data.string_0001,
> +               .maxlen         = sizeof(test_data.string_0001),
> +               .mode           = 0644,
> +               .proc_handler   = proc_dostring,
> +       },
> +       { }
> +};
> +
> +static struct ctl_table test_sysctl_table[] = {
> +       {
> +               .procname       = "test_sysctl",
> +               .maxlen         = 0,
> +               .mode           = 0555,
> +               .child          = test_table,
> +       },
> +       { }
> +};
> +
> +static struct ctl_table test_sysctl_root_table[] = {
> +       {
> +               .procname       = "debug",
> +               .maxlen         = 0,
> +               .mode           = 0555,
> +               .child          = test_sysctl_table,
> +       },
> +       { }
> +};
> +
> +static struct ctl_table_header *test_sysctl_header;
> +
> +static int __init test_sysctl_init(void)
> +{
> +       test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
> +       if (!test_sysctl_header)
> +               return -ENOMEM;
> +       return 0;
> +}
> +late_initcall(test_sysctl_init);
> +
> +static void __exit test_sysctl_exit(void)
> +{
> +       if (test_sysctl_header)
> +               unregister_sysctl_table(test_sysctl_header);
> +}
> +
> +module_exit(test_sysctl_exit);
> +
> +MODULE_AUTHOR("Luis R. Rodriguez <mcgrof@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> diff --git a/tools/testing/selftests/sysctl/config b/tools/testing/selftests/sysctl/config
> new file mode 100644
> index 000000000000..6ca14800d755
> --- /dev/null
> +++ b/tools/testing/selftests/sysctl/config
> @@ -0,0 +1 @@
> +CONFIG_TEST_SYSCTL=y

Technically, this works with =m too, but whatever reads "config" would
need to know to load it first...

> diff --git a/tools/testing/selftests/sysctl/run_numerictests b/tools/testing/selftests/sysctl/run_numerictests
> index 8510f93f2d14..cdfeef96568c 100755
> --- a/tools/testing/selftests/sysctl/run_numerictests
> +++ b/tools/testing/selftests/sysctl/run_numerictests
> @@ -1,7 +1,7 @@
>  #!/bin/sh
>
> -SYSCTL="/proc/sys"
> -TARGET="${SYSCTL}/vm/swappiness"
> +SYSCTL="/proc/sys/debug/test_sysctl/"
> +TARGET="${SYSCTL}/int_0001"
>  ORIG=$(cat "${TARGET}")
>  TEST_STR=$(( $ORIG + 1 ))
>
> diff --git a/tools/testing/selftests/sysctl/run_stringtests b/tools/testing/selftests/sysctl/run_stringtests
> index 90a9293d520c..15a646b2c527 100755
> --- a/tools/testing/selftests/sysctl/run_stringtests
> +++ b/tools/testing/selftests/sysctl/run_stringtests
> @@ -1,7 +1,7 @@
>  #!/bin/sh
>
> -SYSCTL="/proc/sys"
> -TARGET="${SYSCTL}/kernel/domainname"
> +SYSCTL="/proc/sys/debug/test_sysctl/"
> +TARGET="${SYSCTL}/string_0001"
>  ORIG=$(cat "${TARGET}")
>  TEST_STR="Testing sysctl"
>
> --
> 2.11.0
>

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

-Kees

-- 
Kees Cook
Pixel Security