[RFC] samples: Add a self-protection test suite #493

stephensmalley · 2017-06-14T15:19:28Z

Add a self-protection test suite with a set of options
to check whether one can overwrite read-only data
and text, and whether one can execute from data,
stack, or heap buffers. These tests are modeled after
a subset of the lkdtm tests in the Linux kernel.

On boards with MPU support, the disable_mpu and enable_mpu
commands allow one to enable and disable the MPU around other
commands, so that one can see how disabling the MPU affects the tests.

These tests are intentionally separate from mpu_test for the
following reasons:

This is intended to be a testsuite of self-protection features
rather than just a test of MPU functionality. It is envisioned that
these tests will be expanded to cover a wider range of protection
features beyond just memory protection, and the current tests
are independent of any particular enforcement mechanism (e.g.
they can just as easily be enforced by a MMU or other means).
The current tests also operate at a higher level of abstraction
than mpu_test, e.g. testing accessibility of rodata, text, stack,
heap, and data sections.
These tests are intended to be cross-platform. The tests have been
built and run on both x86-based and ARM-based boards. Obviously they
all fail on x86-based ones currently, but this is an accurate
reflection of current protections there and will hopefully change as
MMU support arrives. The mpu_test only builds on ARM-based boards and
embeds various architecture- and board-specific assumptions.
These tests provide clearly different results for success (bus fault
on NXP MPU, MPU fault with ARM MPU) versus failure (FAIL messages),
whereas the mpu_test tests generally fault regardless of success/fail,
only the nature of the fault changes (and sometimes even that is unchanged,
e.g. NXP MPU generates bus faults on read/write failures so you can't always
distinguish from a bus fault due to cause other than MPU).

That said, if the consensus is that these should be merged into
mpu_test, I can do that, but would appreciate guidance on how
much I should change mpu_test along the lines above.

My other open questions are as follows:

Does this really belong in samples (as with mpu_test) or should it
go somewhere else like tests/kernel?
This presently has a user-driven shell command interface (as with
mpu_test). Is that reasonable or should it be replaced/augmented with
a more automated test execution framework?

Signed-off-by: Stephen Smalley [email protected]

dbkinder · 2017-06-14T21:44:45Z

samples/prot_test/README.rst

+
+Overview
+********
+This test provides a set of shell commands to test


Your commit message asked whether this would be better placed in "tests" rather than "samples" -- I think it would be better in "tests" since this isn't really an example of how to do something useful.

dbkinder · 2017-06-14T21:46:30Z

samples/prot_test/README.rst

+* Execute from stack.
+* Execute from heap.
+
+Messages that begin with FAIL: are indicative of a lack of


@inakypg Should this be in a form that would make this test fit into the TCF infrastructure?

dbkinder · 2017-06-14T21:47:26Z

samples/prot_test/README.rst

+* Execute from stack.
+* Execute from heap.
+
+Messages that begin with FAIL: are indicative of a lack of


Can/should there be a SUCCESS: message too?

dbkinder

Some questions

dbkinder · 2017-06-15T04:55:45Z

I also notice this test/sample samples/mpu_stack_guard_test/src/main.c that might have some overlap?

stephensmalley · 2017-06-16T16:04:40Z

Moved from samples/ to tests/.
Rewritten to follow the example of tests/kernel/fatal, using the TC_* macros, dropping the shell command interface. This makes it more of a proper regression test.
Added a testcase.ini, although it isn't clear to me what its contents should be. Current filters seem to exclude any runs by sanitycheck since we don't have MPU support in qemu.
Added text to the commit message to note that these tests have twice caught bugs in the Zephyr NXP MPU implementation.

tbursztyka · 2017-06-16T16:08:34Z

tests/kernel/protection/src/main.c

+ */
+
+#include <zephyr.h>
+#include <tc_util.h>


tc_util is still being in use?

I thought ztest.h was superseding it. It's nicer and easier to use. Could you move towards that one instead?

I followed the example of tests/kernel/fatal. If ztest is preferred, I can move to it, but is it suitable for this kind of testing?

Rewrote it to use ztest.

stephensmalley · 2017-06-16T16:12:24Z

@MaureenHelm @galak please review, these are the tests that detected the recent NXP MPU driver bug.
@fvincenzo any thoughts on how/if this should be combined with mpu_test and mpu_stack_guard_test?
For comparison purposes, the original version of these tests can be seen at:
https://github.com/stephensmalley/zephyr/commit/1901045428a1d3bf5eef0da4e20045e41a88fb26
This pull request however has been updated with a rewrite per dbkinder's comments to relocate to tests and use the tests framework.

stephensmalley · 2017-06-16T21:09:03Z

tests/kernel/protection/src/main.c

+			f, NULL, NULL, NULL,
+			K_PRIO_COOP(PRIORITY), 0,
+			K_NO_WAIT);
+}


Since ztest creates threads internally for each test, we could avoid the need to create this separate thread if ztest provided a ztest_test_pass() analogous to its ztest_test_fail() function, i.e. one that sets test_result = 0 and calls k_sem_give(&test_end_signal). Then we could call that from _SysFatalErrorHandler() upon the MPU fault. Otherwise, we end up hanging on the k_sem_take(&test_end_signal, K_FOREVER) cal in run_test(). If desired, I could add that and include it as part of this commit.

stephensmalley · 2017-06-19T13:58:29Z

Revised to add a ztest_test_pass() interface to ztest, and then rewrote protection tests to use it.
That's a definite improvement for the protection tests, and could likely be used in rewriting kernel/fatal to use ztest as well.
If preferred, I could submit the ztest_test_pass() commit as a separate pull request (it is already a separate commit, but part of this pull request at the moment).

ztest provides a ztest_test_fail() interface to fail the currently running test, but does not provide an equivalent ztest_test_pass(). Normally a test passes just by returning without an assertion failure or other call to ztest_test_fail(). However, if the correct behavior for a test is to trigger a fatal fault (as with tests/kernel/fatal or protection or MPU tests), then we need a way for the test to pass the currently running test before aborting the current thread. Otherwise, ztest hangs forever in run_test() on the k_sem_take(&test_end_signal, K_FOREVER) call. Add a ztest_test_pass() interface and implement it for kernel and userspace variants of ztest. This interface will be used in the protection tests. Signed-off-by: Stephen Smalley <[email protected]>

dbkinder · 2017-06-19T22:59:52Z

tests/kernel/protection/README.rst

@@ -0,0 +1,84 @@
+.. protection


needs a trailing colon, and would be better as:
.. protection_tests:

Thanks, done.

dbkinder

one little tweak, and LGTM

fvincenzo · 2017-06-20T12:24:31Z

@stephensmalley As you correctly analyzed in your RFC description mpu_test and mpu_stack_guard_test are simple example meant to help the development and the integration of the MPU on various platforms and to test the basic functionalities hence are categorized as samples.

What you are proposing here (and I am happy you are doing that 👍, because I had it in mind but due to a work change I am not able to work on Zephyr with continuity anymore...) is an extensible "high level" test suite that verifies the behavior of the MPU independently from the platform of choice.

I agree with the choice of putting it in tests and modelling it following what it has been done in the fatal test.

In the next few days I will look into the patch more in detail and if required I will provide more comments.

stephensmalley · 2017-06-20T12:28:02Z

In the next few days I will look into the patch more in detail and if required I will provide more comments.

Thank you very much! And thanks for the MPU driver implementation for Zephyr - it is definitely a big step forward for Zephyr security.

stephensmalley · 2017-06-20T12:54:49Z

@AdithyaBaglody This might be helpful for MMU support testing.

andrewboie · 2017-06-20T16:47:50Z

Yes, we can definitely use this to test memory protection support on x86 or really any architecture that has an MMU or MPU.

dbkinder · 2017-06-20T17:33:07Z

tests/kernel/protection/README.rst

@@ -0,0 +1,84 @@
+.. protection_tests:


Sorry, I missed that it also needs a leading underscore: .. _protection_tests:

Done. Is there an equivalent to checkpatch.pl checks for the .rst files?

dbkinder

sorry, one more little tweak to that first label I missed in my earlier comment

Add a self-protection test suite with a set of tests to check whether one can overwrite read-only data and text, and whether one can execute from data, stack, or heap buffers. These tests are modeled after a subset of the lkdtm tests in the Linux kernel. These tests have twice caught bugs in the Zephyr NXP MPU driver, once during initial testing/review of the code (in its earliest forms on gerrit, reported to the original author there) and most recently the regression introduced by commit bacbea6 ("arm: nxp: mpu: Rework handling of region descriptor 0"), which was fixed by commit a8aa9d4 ("arm: nxp: mpu: Fix region descriptor 0 attributes") after being reported. This is intended to be a testsuite of self-protection features rather than just a test of MPU functionality. It is envisioned that these tests will be expanded to cover a wider range of protection features beyond just memory protection, and the current tests are independent of any particular enforcement mechanism (e.g. MPU, MMU, or other). The tests are intended to be cross-platform, and have been built and run on both x86- and ARM-based boards. The tests currently fail on x86-based boards, but this is an accurate reflection of current protections and should change as MMU support arrives. The tests leverage the ztest framework, making them suitable for incorporation into automated regression testing for Zephyr. Signed-off-by: Stephen Smalley <[email protected]>

dbkinder

thanks!
Sadly, no. We don't have a checkpatch equivalent for .rst files. Generally .rst errors are caught by the doc generation process itself (and included in the test run). But there are some .rst errors that go unnoticed because, for example, directives (lines starting with .. ) with unknown names are treated as comments. I'll see what I can do.

AdithyaBaglody · 2017-06-21T05:20:01Z

@stephensmalley Agreed, this would help out with all memory protection testing.

MaureenHelm

If preferred, I could submit the ztest_test_pass() commit as a separate pull request (it is already a separate commit, but part of this pull request at the moment).

I don't see a need for a separate pull request as long as @inakypg +1's this one.

I said earlier that this test could be merged with mpu_test, but you make a good argument to keep separate given it can be used for other non-mpu protection mechanisms. The implementation LGTM. Thanks for contributing the test!

MaureenHelm · 2017-06-21T17:53:55Z

tests/kernel/protection/src/main.c

+ * Parts derived from tests/kernel/fatal/src/main.c, which has the
+ * following copyright and license:
+ *
+ * Copyright (c) 2017 Intel Corporation


You can use your own copyright

Only relevant copyright is that of tests/kernel/fatal/src/main.c, from which I copied the _SysFatalErrorHandler() definition (and originally a few other bits, but those got replaced in converting to using ztest). No copyright for anything I wrote, since USG works are public domain by statute.

nashif · 2017-06-22T01:50:35Z

@stephensmalley

Current filters seem to exclude any runs by sanitycheck since we don't have MPU support in qemu.

Right, but we do run those test on hardware using the same files, so that is all fine, at some point we need to look how to enable this type of tests in qemu as well.

…ect-rtos#493) * [scripts] New script 'trlite' to run Travis tests locally With no args, runs all the tests. "trlite zephyr" runs just the VM zephyrproject-rtos#1 "zephyr" tests. "trlite linux" runs just the VM zephyrproject-rtos#2 "linux" tests. "trlite ashell" runs just the VM zephyrproject-rtos#3 "ashell" tests. You can also just pass the VM# instead (1, 2, or 3). Signed-off-by: Geoff Gustafson <[email protected]> * [trlite] Add -v flag for verbose output, change to off by default Signed-off-by: Geoff Gustafson <[email protected]>

stephensmalley mentioned this pull request Jun 14, 2017

arm: nxp: mpu: Fix Coverity issue #441

Merged

dbkinder reviewed Jun 14, 2017

View reviewed changes

tbursztyka reviewed Jun 16, 2017

View reviewed changes

stephensmalley commented Jun 16, 2017

View reviewed changes

dbkinder reviewed Jun 19, 2017

View reviewed changes

dbkinder suggested changes Jun 19, 2017

View reviewed changes

dbkinder reviewed Jun 20, 2017

View reviewed changes

dbkinder suggested changes Jun 20, 2017

View reviewed changes

dbkinder approved these changes Jun 20, 2017

View reviewed changes

MaureenHelm reviewed Jun 21, 2017

View reviewed changes

MaureenHelm requested a review from inakypg June 21, 2017 18:11

nashif merged commit c997577 into zephyrproject-rtos:master Jun 22, 2017

zephyrbot mentioned this pull request Sep 16, 2019

arc: hsdk: bug fixes and new feature supports #19173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] samples: Add a self-protection test suite #493

[RFC] samples: Add a self-protection test suite #493

stephensmalley commented Jun 14, 2017

dbkinder Jun 14, 2017

dbkinder Jun 14, 2017

dbkinder Jun 14, 2017

dbkinder left a comment

dbkinder commented Jun 15, 2017

stephensmalley commented Jun 16, 2017

tbursztyka Jun 16, 2017

stephensmalley Jun 16, 2017

stephensmalley Jun 16, 2017

stephensmalley commented Jun 16, 2017

stephensmalley Jun 16, 2017

stephensmalley commented Jun 19, 2017

dbkinder Jun 19, 2017

stephensmalley Jun 20, 2017

dbkinder left a comment

fvincenzo commented Jun 20, 2017

stephensmalley commented Jun 20, 2017

stephensmalley commented Jun 20, 2017

andrewboie commented Jun 20, 2017

dbkinder Jun 20, 2017

stephensmalley Jun 20, 2017

dbkinder left a comment

dbkinder left a comment •

edited

Loading

AdithyaBaglody commented Jun 21, 2017

MaureenHelm left a comment

MaureenHelm Jun 21, 2017

stephensmalley Jun 21, 2017

nashif commented Jun 22, 2017

[RFC] samples: Add a self-protection test suite #493

[RFC] samples: Add a self-protection test suite #493

Conversation

stephensmalley commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkinder left a comment

Choose a reason for hiding this comment

dbkinder commented Jun 15, 2017

stephensmalley commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephensmalley commented Jun 16, 2017

Choose a reason for hiding this comment

stephensmalley commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkinder left a comment

Choose a reason for hiding this comment

fvincenzo commented Jun 20, 2017

stephensmalley commented Jun 20, 2017

stephensmalley commented Jun 20, 2017

andrewboie commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkinder left a comment

Choose a reason for hiding this comment

dbkinder left a comment • edited Loading

Choose a reason for hiding this comment

AdithyaBaglody commented Jun 21, 2017

MaureenHelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashif commented Jun 22, 2017

dbkinder left a comment •

edited

Loading