[PATCH 0/2] test modules

[PATCH 0/2] test modules

[Kees Cook]

Hi,

This is a pair of test modules I'd like to see in the tree. Instead
of putting these in lkdtm, where I've been adding various tests that
trigger crashes, these don't make sense there since they need to be
either distinctly separate, or their pass/fail state don't need to crash
the machine.

The first is entirely a no-op module, designed to allow simple testing
of the module loading and verification interface. It's useful to have
a module that has no other uses or dependencies so it can be reliably
used for just testing module loading and verification.

The second is a module that exercises the user memory access functions,
in an effort to make sure that we can quickly catch any regressions in
boundary checking (e.g. like what was recently fixed on ARM).

Thanks,

-Kees

[PATCH 1/2] test: add minimal module for verification testing

[Kees Cook]

When doing module loading verification tests (for example, with module
singing, or LSM hooks), it is very handy to have a module that can be
built on all systems under test, isn't auto-loaded at boot, and has
no device or similar dependencies. This creates the "test_module.ko"
module for that purpose, which only reports its load and unload to printk.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/Makefile      |    1 +
 kernel/test_module.c |   26 ++++++++++++++++++++++++++
 lib/Kconfig.debug    |   14 ++++++++++++++
 3 files changed, 41 insertions(+)
 create mode 100644 kernel/test_module.c

diff --git a/kernel/Makefile b/kernel/Makefile
index bbaf7d59c1bb..9de0f0e89937 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_IKCONFIG) += configs.o
 obj-$(CONFIG_RESOURCE_COUNTERS) += res_counter.o
 obj-$(CONFIG_SMP) += stop_machine.o
 obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
+obj-$(CONFIG_TEST_MODULE) += test_module.o
 obj-$(CONFIG_AUDIT) += audit.o auditfilter.o
 obj-$(CONFIG_AUDITSYSCALL) += auditsc.o
 obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o
diff --git a/kernel/test_module.c b/kernel/test_module.c
new file mode 100644
index 000000000000..f825589315d7
--- /dev/null
+++ b/kernel/test_module.c
@@ -0,0 +1,26 @@
+/*
+ * "hello world" kernel module
+ */
+
+#define pr_fmt(fmt) "test_module: " fmt
+
+#include <linux/module.h>
+
+static int __init test_module_init(void)
+{
+	pr_info("Hello, world\n");
+
+	return 0;
+}
+
+module_init(test_module_init);
+
+static void __exit test_module_exit(void)
+{
+	pr_info("Goodbye\n");
+}
+
+module_exit(test_module_exit);
+
+MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
+MODULE_LICENSE("GPL");
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index db25707aa41b..20abc92032e0 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1578,6 +1578,20 @@ config DMA_API_DEBUG
 	  This option causes a performance degredation.  Use only if you want
 	  to debug device drivers. If unsure, say N.
 
+config TEST_MODULE
+	tristate "Test module loading with 'hello world' module"
+	default n
+	depends on m
+	help
+	  This builds the "test_module" module that emits "Hello, world"
+	  on printk when loaded. It is designed to be used for basic
+	  evaluation of the module loading subsystem (for example when
+	  validating module verification). It lacks any extra dependencies,
+	  and will not normally be loaded by the system unless explicitly
+	  request by name.
+
+	  If unsure, say N.
+
 source "samples/Kconfig"
 
 source "lib/Kconfig.kgdb"
-- 
1.7.9.5

Re: [PATCH 1/2] test: add minimal module for verification testing

[Andrew Morton]

On Tue,  3 Dec 2013 13:27:33 -0800 Kees Cook <keescook@chromium.org> wrote:

> When doing module loading verification tests (for example, with module
> singing, or LSM hooks), it is very handy to have a module that can be
> built on all systems under test, isn't auto-loaded at boot, and has
> no device or similar dependencies. This creates the "test_module.ko"
> module for that purpose, which only reports its load and unload to printk.
> 
> --- /dev/null
> +++ b/kernel/test_module.c
> @@ -0,0 +1,26 @@
> +/*
> + * "hello world" kernel module
> + */
> +
> +#define pr_fmt(fmt) "test_module: " fmt
> +
> +#include <linux/module.h>

It really should have a whole bunch more includes, rather than relying
on winning the nested include lottery.

> +static int __init test_module_init(void)
> +{
> +	pr_info("Hello, world\n");

pr_info is maybe too low a facility level?  Pretty much the whole point
of the module is to print this message, so let's ensure that it is visible.

> +	return 0;
> +}
> +
> +module_init(test_module_init);
> +
> +static void __exit test_module_exit(void)
> +{
> +	pr_info("Goodbye\n");
> +}
> +
> +module_exit(test_module_exit);
> +
> +MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
> +MODULE_LICENSE("GPL");
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index db25707aa41b..20abc92032e0 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1578,6 +1578,20 @@ config DMA_API_DEBUG
>  	  This option causes a performance degredation.  Use only if you want
>  	  to debug device drivers. If unsure, say N.
>  
> +config TEST_MODULE
> +	tristate "Test module loading with 'hello world' module"
> +	default n
> +	depends on m
> +	help
> +	  This builds the "test_module" module that emits "Hello, world"
> +	  on printk when loaded. It is designed to be used for basic
> +	  evaluation of the module loading subsystem (for example when
> +	  validating module verification). It lacks any extra dependencies,
> +	  and will not normally be loaded by the system unless explicitly
> +	  request by name.

Would be useful to have a comment along these lines in test_module.c,
so readers know why it exists.

Re: [PATCH 1/2] test: add minimal module for verification testing

[Rusty Russell]

Kees Cook <keescook@chromium.org> writes:
> When doing module loading verification tests (for example, with module
> singing, or LSM hooks), it is very handy to have a module that can be
> built on all systems under test, isn't auto-loaded at boot, and has
> no device or similar dependencies. This creates the "test_module.ko"
> module for that purpose, which only reports its load and unload to printk.

Bikeshed time!

> +#define pr_fmt(fmt) "test_module: " fmt

Perhaps:
        #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Cheers,
Rusty.

[PATCH 2/2] test: check copy_to/from_user boundary validation

[Kees Cook]

To help avoid an architecture failing to correctly check kernel/user
boundaries when handling copy_to_user, copy_from_user, put_user, or
get_user, perform some simple tests and fail to load if any of them
behave unexpectedly.

Specifically, this is to make sure there is a way to notice if things
like what was fixed in 8404663f81d212918ff85f493649a7991209fa04 ("ARM:
7527/1: uaccess: explicitly check __user pointer when !CPU_USE_DOMAINS")
ever regresses again, for any architecture.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/Makefile         |    1 +
 kernel/test_user_copy.c |  103 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/Kconfig.debug       |   13 ++++++
 3 files changed, 117 insertions(+)
 create mode 100644 kernel/test_user_copy.c

diff --git a/kernel/Makefile b/kernel/Makefile
index 9de0f0e89937..f8185152cbb8 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_RESOURCE_COUNTERS) += res_counter.o
 obj-$(CONFIG_SMP) += stop_machine.o
 obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
 obj-$(CONFIG_TEST_MODULE) += test_module.o
+obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
 obj-$(CONFIG_AUDIT) += audit.o auditfilter.o
 obj-$(CONFIG_AUDITSYSCALL) += auditsc.o
 obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o
diff --git a/kernel/test_user_copy.c b/kernel/test_user_copy.c
new file mode 100644
index 000000000000..dbf62ec80e48
--- /dev/null
+++ b/kernel/test_user_copy.c
@@ -0,0 +1,103 @@
+/*
+ * Kernel module for testing copy_to/from_user infrastructure.
+ *
+ * Copyright 2013 Google Inc. All Rights Reserved
+ *
+ * Authors:
+ *      Kees Cook       <keescook@chromium.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) "test_user_copy: " fmt
+
+#include <linux/mman.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/vmalloc.h>
+
+#define failed(msg) {	\
+	pr_warn(msg);	\
+	ret = -EINVAL;	\
+}
+
+static int __init test_user_copy_init(void)
+{
+	int ret = 0;
+	char *kmem;
+	char __user *usermem;
+	unsigned long user_addr;
+	unsigned long value = 0x5A;
+
+	kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
+	if (!kmem) {
+		pr_warn("Failed to allocate kernel memory\n");
+		return -ENOMEM;
+	}
+
+	user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
+			    PROT_READ | PROT_WRITE | PROT_EXEC,
+			    MAP_ANONYMOUS | MAP_PRIVATE, 0);
+	if (user_addr >= TASK_SIZE) {
+		pr_warn("Failed to allocate user memory\n");
+		kfree(kmem);
+		return -ENOMEM;
+	}
+
+	usermem = (char __user *)user_addr;
+
+	/* Legitimate usage: none of these should fail. */
+	if (copy_from_user(kmem, usermem, PAGE_SIZE))
+		failed("legitimate copy_from_user failed");
+	if (copy_to_user(usermem, kmem, PAGE_SIZE))
+		failed("legitimate copy_to_user failed");
+	if (get_user(value, (unsigned long __user *)usermem))
+		failed("legitimate get_user failed");
+	if (put_user(value, (unsigned long __user *)usermem))
+		failed("legitimate put_user failed");
+
+	/* Invalid usage: none of these should succeed. */
+	if (!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+			    PAGE_SIZE))
+		failed("illegal all-kernel copy_from_user passed");
+	if (!copy_from_user((char *)usermem, (char __user *)kmem, PAGE_SIZE))
+		failed("illegal reversed copy_from_user passed");
+
+	if (!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE, PAGE_SIZE))
+		failed("illegal all-kernel copy_to_user passed");
+	if (!copy_to_user((char __user *)kmem, (char *)usermem, PAGE_SIZE))
+		failed("illegal reversed copy_to_user passed");
+
+	if (!get_user(value, (unsigned long __user *)kmem))
+		failed("illegal get_user passed");
+	if (!put_user(value, (unsigned long __user *)kmem))
+		failed("illegal put_user passed");
+
+	vm_munmap(user_addr, PAGE_SIZE * 2);
+	kfree(kmem);
+
+	if (ret == 0)
+		pr_info("tests passed.\n");
+
+	return ret;
+}
+
+module_init(test_user_copy_init);
+
+static void __exit test_user_copy_exit(void)
+{
+	pr_info("unloaded.\n");
+}
+
+module_exit(test_user_copy_exit);
+
+MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
+MODULE_LICENSE("GPL");
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 20abc92032e0..92d1a39f885e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1592,6 +1592,19 @@ config TEST_MODULE
 
 	  If unsure, say N.
 
+config TEST_USER_COPY
+	tristate "Test user/kernel boundary protections"
+	default n
+	depends on m
+	help
+	  This builds the "test_user_copy" module that runs sanity checks
+	  on the copy_to/from_user infrastructure, making sure basic
+	  user/kernel boundary testing is working. If it fails to load,
+	  a regression has been detected in the user/kernel memory boundary
+	  protections.
+
+	  If unsure, say N.
+
 source "samples/Kconfig"
 
 source "lib/Kconfig.kgdb"
-- 
1.7.9.5

Re: [PATCH 2/2] test: check copy_to/from_user boundary validation

[Andrew Morton]

On Tue,  3 Dec 2013 13:27:34 -0800 Kees Cook <keescook@chromium.org> wrote:

> To help avoid an architecture failing to correctly check kernel/user
> boundaries when handling copy_to_user, copy_from_user, put_user, or
> get_user, perform some simple tests and fail to load if any of them
> behave unexpectedly.
> 
> Specifically, this is to make sure there is a way to notice if things
> like what was fixed in 8404663f81d212918ff85f493649a7991209fa04 ("ARM:
> 7527/1: uaccess: explicitly check __user pointer when !CPU_USE_DOMAINS")
> ever regresses again, for any architecture.

I guess the challenge will be to get anyone to remember to run this.

Really, this could be viewed as a candidate for
tools/testing/selftests.  The tests in there are userspace tests, and
your userspace test would consist of "modprobe test_user_copy".  The
advantage of this is that your test will get included whenever someone
runs the selftest suite.  This is better than having it stranded over
in ./kernel/.

> ---
>  kernel/Makefile         |    1 +
>  kernel/test_user_copy.c |  103 +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/Kconfig.debug       |   13 ++++++

We already have a whole pile of test modules - seven of them reside in
lib/ and I think there's an RCU one somewhere.  Can we bring order to
all of this?  Some form of integration under tools/testing would be one
approach.

If you're disinclined to undertake such a project at this time, I'd
suggest these two go into lib/ so they are known about if/when someone
goes for the big cleanup.

Re: [PATCH 2/2] test: check copy_to/from_user boundary validation

[Kees Cook]

On Tue, Dec 3, 2013 at 1:47 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue,  3 Dec 2013 13:27:34 -0800 Kees Cook <keescook@chromium.org> wrote:
>
>> To help avoid an architecture failing to correctly check kernel/user
>> boundaries when handling copy_to_user, copy_from_user, put_user, or
>> get_user, perform some simple tests and fail to load if any of them
>> behave unexpectedly.
>>
>> Specifically, this is to make sure there is a way to notice if things
>> like what was fixed in 8404663f81d212918ff85f493649a7991209fa04 ("ARM:
>> 7527/1: uaccess: explicitly check __user pointer when !CPU_USE_DOMAINS")
>> ever regresses again, for any architecture.
>
> I guess the challenge will be to get anyone to remember to run this.

FWIW, I'll be running it. :) I'm sure Fengguang Wu could add it too.

> Really, this could be viewed as a candidate for
> tools/testing/selftests.  The tests in there are userspace tests, and
> your userspace test would consist of "modprobe test_user_copy".  The
> advantage of this is that your test will get included whenever someone
> runs the selftest suite.  This is better than having it stranded over
> in ./kernel/.

Sure, I'd be happy to add logic to that area too.

>> ---
>>  kernel/Makefile         |    1 +
>>  kernel/test_user_copy.c |  103 +++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/Kconfig.debug       |   13 ++++++
>
> We already have a whole pile of test modules - seven of them reside in
> lib/ and I think there's an RCU one somewhere.  Can we bring order to
> all of this?  Some form of integration under tools/testing would be one
> approach.

I expect to be adding more of these, and I'd like to see these
collected in a single place as well. There are things like test_nx.c
in the x86 tree (which actually doesn't work any more, but that's a
separate issue).

> If you're disinclined to undertake such a project at this time, I'd
> suggest these two go into lib/ so they are known about if/when someone
> goes for the big cleanup.

lib/ sounds good. I'll move them there.

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 2/2] test: check copy_to/from_user boundary validation

[Joe Perches]

On Tue, 2013-12-03 at 13:27 -0800, Kees Cook wrote:
> To help avoid an architecture failing to correctly check kernel/user
> boundaries when handling copy_to_user, copy_from_user, put_user, or
> get_user, perform some simple tests and fail to load if any of them
> behave unexpectedly.

> diff --git a/kernel/test_user_copy.c b/kernel/test_user_copy.c

> +#define failed(msg) {	\
> +	pr_warn(msg);	\
> +	ret = -EINVAL;	\
> +}

That's an ugly macro.

Maybe use something like:

#define test(func, msg)				\
({						\
	int ret = func;				\
	if (ret)				\
		pr_warn("%s\n", msg);		\
	ret;					\
})

> +static int __init test_user_copy_init(void)
> +{
> +	int ret = 0;
> +	char *kmem;
> +	char __user *usermem;
> +	unsigned long user_addr;
> +	unsigned long value = 0x5A;
> +
> +	kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> +	if (!kmem) {
> +		pr_warn("Failed to allocate kernel memory\n");

Unnecessary as there's a generic alloc OOM
with a dump_stack()

[]

> +	/* Legitimate usage: none of these should fail. */
> +	if (copy_from_user(kmem, usermem, PAGE_SIZE))
> +		failed("legitimate copy_from_user failed");

	ret |= test(copy_from_user(kmem, usermem, PAGE_SIZE),
		    "legitimate copy_from_user failed");

> +	if (copy_to_user(usermem, kmem, PAGE_SIZE))
> +		failed("legitimate copy_to_user failed");

	ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
		    "legitimate copy_to_user failed");

etc.