kernel object validation infrastructure #1276

andrewboie · 2017-08-28T18:17:50Z

This implements kernel object validation as discussed in https://jira.zephyrproject.org/browse/ZEP-2187

The actual validation will take place in forthcoming patches to implement system calls, and only done upon privilege elevation.

Not all kernel object types are connected to this mechanism; forthcoming work will enable the rest of them as well as device drivers.

lpereira · 2017-08-28T19:03:10Z

kernel/object_validate.c

+	struct _k_object *ko = _k_object_find(object);
+
+	if (!ko) {
+		printk("granting access to non-existent kernel object %p\n",


Leftover debugging printk()?

this is intentional, k_object_grant_access() is a public API and should informatively freak out if a bad object is passed in.

Something we need is a more "regular" way to print warning/error messages so they are easier to monitor for (not the subject of this patch though).

This should be guarded by CONFIG_PRINTK anyway (other places of the code do this)

that's not correct. printk() expands to a no-op inline function if CONFIG_PRINTK=n.
the reason why some other calls are guarded is because they reference the otype_to_str() function. this function is itself guarded because GCC's -ffunction/data-sections doesn't work the way you would expect with strings, and the strings would end up in the binary anyway even if otype_to_str was never used.

lpereira · 2017-08-28T20:02:30Z

scripts/process_gperf.py

+
+    # Take the strings with the binary information for the pointer values,
+    # and just turn them into pointers
+    line = re.sub(r'["].*["]', reformat_str, line)


More things that could be done here:

Remove the len parameter from hash() and _k_object_lookup()

len can be added somewhere in _k_object_lookup() as a constant value of 4. This should get rid of some branches inside that function

looking at the generated assembly code, the compiler appears to be smart enough to disregard len entirely since both hash and k_object_lookup are now static functions, called only from _k_object_find() which uses a constant parameter (4) for len,

I don't currently see a need for these two steps unless I'm missing something.

For example on x86, all of the code currently reduces to:

Dump of assembler code for function _k_object_find:
0x00002b84 <+0>: push %ebp
0x00002b85 <+1>: mov %esp,%ebp
0x00002b87 <+3>: push %eax
0x00002b88 <+4>: mov 0x8(%ebp),%ecx
0x00002b8b <+7>: movzbl %cl,%eax
0x00002b8e <+10>: movzbl 0x38c0(%eax),%edx
0x00002b95 <+17>: xor %eax,%eax
0x00002b97 <+19>: cmp $0x1,%edx
0x00002b9a <+22>: ja 0x2bac <_k_object_find+40>
0x00002b9c <+24>: cmp 0x40d000(,%edx,8),%ecx
0x00002ba3 <+31>: jne 0x2bac <_k_object_find+40>
0x00002ba5 <+33>: lea 0x40d000(,%edx,8),%eax
0x00002bac <+40>: leave
0x00002bad <+41>: ret
End of assembler dump.

I've made that comment before looking at the assembly output. My bad. Yeah, those changes are unnecessary.

lpereira · 2017-08-28T20:14:24Z

scripts/gen_kobject_list.py

+"""
+
+
+def write_gperf_table(fp, objs, static_begin, static_end):


Played around with this, and it seems we can reduce the table even further if we do some processing on these pointers. The caveat is that the lookup will be slightly slower; but worth checking out, maybe later:

Subtract the smaller pointer value from the list of all the pointers. In the obj_validation test for qemu_x86, this made the largest value keep well below what a 16-bit integer can hold

Look at all the values if they're aligned: if the least significant nibble for all the pointers is 0, these values can be shifted-right by 2. In the obj_validation test for qemu_x86, all the values were well below what an 8-bit integer can hold

The _k_object structure has to be changed based on these findings, and _k_object_find() has to undo the compression, but this reduces the overhead without too much work

lpereira · 2017-08-28T20:22:05Z

kernel/object_validate.c

+
+
+/**
+ * grant a thread access to a kernek object


s/kernek/kernel/

dbkinder

The only doc changes here is to add gperf to the development environment. Will there be some documentation (eventually) that talks about the validation mechanism for user-supplied kernel object pointers?

andrewboie · 2017-08-29T15:57:07Z

I don't think it's useful to write documentation for features that are not user-facing. All of this is 'behind the scenes' stuff to support userspace.
There will be formal documentation on userspace/thread protection in the fullness of time, but we are a long way away from that.

nashif · 2017-08-29T16:24:54Z

I don't think it's useful to write documentation for features that are not user-facing.

I think a (slightly modified) subset of the proposal text you have in the Jira makes a good introduction of the object verification feature in an overall chapter we should have about memory protection, thread isolation and "user-space" features to be introduced later.

lpereira · 2017-08-29T17:09:43Z

About the documentation: yeah, I agree. We say Zephyr is built with security in mind and having something like this in the documentation is helpful... specially since this isn't done in a common way.

andrewboie · 2017-08-29T17:38:27Z

Guys, it is way too early in the development of this feature to be writing documentation on how it works internally. Too many things can and will change as the design evolves.
Also I prefer such documentation to be extremely high level, if people want to know how something works in detail they need to look at the code, which should be well-commented.
I do not intend to start writing formal documentation for memory protection until implementation is almost complete.

dbkinder · 2017-08-29T18:29:46Z

I was hoping for an overview of requirements driving the implementation of various object verification capabilities (memory protection, thread isolation, etc.) rather than the specific implementation details.

andrewboie · 2017-08-29T18:58:52Z

@dbkinder what you are looking for is documented in an RFC in the comments for the JIRA linked in the PR description.
Formal documentation under the doc/ directory will not be written until the overall mechanism is almost complete. If I write it now it will just have to be re-written later.

nashif · 2017-08-29T20:30:49Z

I do not intend to start writing formal documentation for memory protection until implementation is almost complete.

That I fully agree with you on, I was not hoping for this documentation to be part of this PR or 1.9, should be part of the overall memory protection feature which is not complete yet for 1.9

chunlinhan · 2017-08-30T06:32:28Z

kernel/object_validate.c

+		 */
+	}
+
+	memset(ko->perms, 0, CONFIG_MAX_THREAD_BITS);


In line 181, we just grant the access permission to the caller thread, but clear all permission bits here?

yeah this isn't right. looks like i fixed it in some later work but I need to backport to this series.

andrewboie · 2017-08-30T20:09:16Z

New patch set differences:

No changes to the python scripts or any of the stuff which generates and links the gperf tables
CONFIG_KERNEL_OBJECT_VERIFY rolled into CONFIG_USERSPACE. Further work revealed that object validation doesn't make much sense on its own and it's tightly linked to userspace/system calls.
kernel/object_validation.c renamed to userspace.c. Additional code to support userspace will go in there, not just object validation.
Policy and logic fixes in kernel/userspace.c
No longer turning this on by default for QEMU yet. Userspace implementation currently proceeding on x86, so we will just test that for now in tests/kernel/object_validation.

andyross

A few notes & whines. Seems surprisingly small given what it does!

andyross · 2017-08-31T17:07:54Z

tests/kernel/gen_isr_table/prj.conf

@@ -4,4 +4,4 @@
 # A memory barrier does not help, we need an 'instruction barrier' but
 # GCC doesn't support this; we need to tell the compiler not to reorder
 # memory accesses to trigger_check around calls to trigger_irq.
-CONFIG_COMPILER_OPT="-O0"
+CONFIG_DEBUG=y


Pedantry: I'm not sure I like this any better. CONFIG_DEBUG disables optimization, but I don't know that "debug" necessarily requires that to everyone reading this, nor that it will alwyas mean -O0 in perpetuity.

If we want to fix this, maybe writing some simple assembly to call functions in the right order and/or using a attribute((optimize("O0"))) on the relevant function would be better?

attribute((optimize("O0"))) on the relevant function sounds better, I'll do that.

Even then, will O0 guarantee there won't be any instruction reordering?

andyross · 2017-08-31T18:02:18Z

include/kernel.h

+enum k_objects {
+	K_OBJ_ALERT,
+	K_OBJ_DELAYED_WORK,
+	K_OBJ_MEM_SLAB,


Are slabs really needed? Seems like (just like pools) the API should work just fine from user code when referring to memory inside userspace, and the utility of managing a kernel-side allocator from user code that can't touch it seems kinda weird.

in its current implementation, the mem_slab APIs will not work from userspace. they contain wait queues, they have other threads pending on them, etc...redesign would be needed to make them work entirely from userspace. have a look at the code for k_mem_slab_free() there's all kinds of stuff in there that userspace can't do.

also its worth noting that detailed analysis of kernel object APIs is for a later stage in development of this feature, currently I am not changing any of them and just trying to get the infrastructure in place.

andyross · 2017-08-31T18:06:34Z

kernel/userspace.c

+		return "k_pipe";
+	case K_OBJ_SEM:
+		return "k_sem";
+	case K_OBJ_STACK:


Oh right, k_stack. Remind me again why k_stack and k_fifo are different things (beyond the fact that one takes a void* and the other a u32_t)? Not relevant to review, just something we should consider reviewing and unifying.

yeah we have some issues with these APIs, additionally see https://jira.zephyrproject.org/browse/ZEP-2237

andyross · 2017-08-31T18:09:02Z

kernel/userspace.c

+struct _k_object *_k_object_find(void *obj)
+{
+	/* This is the __weak stub implementation, the version created by
+	 * gperf will replace it in build phase 2


FWIW: there's no need for this thing to actually be a function if it will never be called. Linker names are typeless. And no need to put it in a specific section if all you want is for the link to not fail. Just a:

__weak int _k_object_find;

...should be enough, no? Or alternativesly a PROVIDES() directive in the linker script that is #if'd appropriately so it shows up only in the early linkage.

i'll give that a try!

andyross · 2017-08-31T18:11:15Z

kernel/userspace.c

+	return NULL;
+}
+
+#ifdef CONFIG_PRINTK


No need for this #if. The function won't be linked if unreferenced, and it's not static so it won't generate an error.

yes, there is.
gc-sections is broken with C strings. even if gc-sections strips this function, the strings will still be in the binary :(
upstream fixed it, but i believe the fix landed in the GCC 7.x branch.

andyross · 2017-08-31T18:15:51Z

kernel/userspace.c

+		return;
+	}
+
+	memset(ko->perms, 0, CONFIG_MAX_THREAD_BITS);


Naming mistake or size goof? If that config value is in bits, then this is clobbering 8x too much memory. If it's not, it should be renamed.

oops, naming sucks, will fix this

andyross · 2017-08-31T18:25:27Z

scripts/gen_kobject_list.py

+            # FIXME: why does k_sys_work_q not resolve an address in the DWARF
+            # data??? Every single instance it finds is an extern definition
+            # but not the actual instance in system_work_q.c
+            # Is there something weird about how lib-y stuff is linked?


Yes, actually: the kernel directory is always presented to the link as an already-linked static library, to prevent the case where pulling symbols from one file pulls in static data too and bloats the image. It looks like k_sys_work_q is only ever referenced from inside the kernel, so that may be triggering some variant DWARF output. How does this look in e.g. gdb? Can you see the type there?

I can't seem to find it in the DWARF output at all except as an extern reference. It's really baffling. I'm going to file a bug for further investigation.

andyross · 2017-08-31T18:28:29Z

scripts/process_gperf.py

+We use gperf to build up a perfect hashtable of pointer values. The way gperf
+does this is to create a table 'wordlist' indexed by a string repreesentation
+of a pointer address, and then doing memcmp() on a string passed in for
+comparison


This is super clever, but I just have to whine:

How robust is this vs. changes to the code output in upstream gperf? I doubt they're doing much development these days, but if they do it's not unlikely that this script will break badly. It might be a good idea to put a version check somewhere (maybe right here: is there a "generated by gperv v${WHATEVER}" comment in the file?) to catch that circumstance and force us to revalidate the metacompiler.

good idea, I'll add a version string check and error out. so far this works in gperf 3.0.x and 3.1. agree this will fall over if the generated C code changes too much.

andyross · 2017-08-31T18:31:33Z

kernel/userspace.c

+	return 0;
+}
+
+void k_object_grant_access(void *object, struct k_thread *thread)


Design note: would be really nice to have a way to do this statically, given that the objects and threads are static objects. Would be even nicer to have it be implicit, along the lines of "a static thread has access to any static kernel object declared in the same C source file"

i'd like to punt on this for now, but that could be a future enhancement once all the implications have been analyzed.

https://jira.zephyrproject.org/browse/ZEP-2585 to look into this

andyross · 2017-08-31T18:32:20Z

scripts/sanitycheck

+                   "_k_mem_slab_area", "_k_mem_pool_area",
+                   "_k_sem_area", "_k_mutex_area", "_k_alert_area",
+                   "_k_fifo_area", "_k_lifo_area", "_k_stack_area",
+                   "_k_msgq_area", "_k_mbox_area", "_k_pipe_area",
                   "net_if", "net_if_event", "net_stack", "net_l2_data",


Unrelated whitespace muckery snuck into the patch.

doesn't the checkpatch script catch that?

@agross-linaro catch what, exactly?
at any rate, the whitespace changes are in their own patch in the series.

andrewboie · 2017-08-31T19:44:01Z

updated patch set:

sanitycheck whitespace changes moved into its own patch
gperf version checking in post-processing script
attribute((optimize("-O0"))) used in gen_isr_table test, plus some other fixes
use linker PROVIDE() for _k_object_find() instead of dummy function
CONFIG_MAX_THREAD_BITS now named CONFIG_MAX_THREAD_BYTES
make use of CONFIG_PRINTK in userspace.c much clearer

agross-oss · 2017-08-31T19:55:17Z

tests/kernel/gen_isr_table/src/main.c

+ * doesn't support this; we need to tell the compiler not to reorder memory
+ * accesses to trigger_check around calls to trigger_irq.
+ */
+__attribute__((optimize("-O0")))


This idea of using the optimize to somehow give an instruction barrier makes me uneasy. Why not just add a __ISB() for ARM architectures. I am sure other arches have similar solutions.

i don't want to get too sidetracked by this -- I needed to get rid of the global -O0 in the prj,conf because it messes up the padding size for the generated gperf code in the linker script.

I moved it to the function instead. if we want to further iterate on it, please file a bug.

agross-oss

The only questionable thing I saw was the optimization usage. I'd be more comfortable with a std instruction/data barrier being used.

andrewboie · 2017-08-31T20:04:33Z

@agross-linaro the optimize thing was already there, its really not in the scope of this patch, i'm just moving it. can you file a bug if you want it looked at further?

agross-oss · 2017-08-31T20:06:05Z

Fair enough. I'll write one up.

This is unnecessary. Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

This test doesn't work on x86, which doesn't use gen_isr_tables. Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

Instead, just set -O0 for the particular function which needs it. Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

All system calls made from userspace which involve pointers to kernel objects (including device drivers) will need to have those pointers validated; userspace should never be able to crash the kernel by passing it garbage. The actual validation with _k_object_validate() will be in the system call receiver code, which doesn't exist yet. - CONFIG_USERSPACE introduced. We are somewhat far away from having an end-to-end implementation, but at least need a Kconfig symbol to guard the incoming code with. Formal documentation doesn't exist yet either, but will appear later down the road once the implementation is mostly finalized. - In the memory region for RAM, the data section has been moved last, past bss and noinit. This ensures that inserting generated tables with addresses of kernel objects does not change the addresses of those objects (which would make the table invalid) - The DWARF debug information in the generated ELF binary is parsed to fetch the locations of all kernel objects and pass this to gperf to create a perfect hash table of their memory addresses. - The generated gperf code doesn't know that we are exclusively working with memory addresses and uses memory inefficently. A post-processing script process_gperf.py adjusts the generated code before it is compiled to work with pointer values directly and not strings containing them. - _k_object_init() calls inserted into the init functions for the set of kernel object types we are going to support so far Issue: ZEP-2187 Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

andyross

Can't see anything else worth whining about.

Note that (now that I'm thinkinga bout the root issue and not just how to do optimization pragmas) it's probably possible to get gcc to do the right thing with a noop in a asm() statement that declares memory as a clobber. But agreed, that's a separate problem.

andrewboie requested review from andyross and dbkinder as code owners August 28, 2017 18:17

andrewboie force-pushed the dwarf branch from ed63fb7 to 945d6ee Compare August 28, 2017 18:18

andrewboie requested review from lpereira, d3zd3z, nashif, ruuddw, MaureenHelm, inakypg, galak, AdithyaBaglody and agross-oss August 28, 2017 18:27

lpereira reviewed Aug 28, 2017

View reviewed changes

andrewboie force-pushed the dwarf branch from 945d6ee to fded2ef Compare August 28, 2017 21:29

nashif added this to the v1.10 milestone Aug 28, 2017

dbkinder previously approved these changes Aug 29, 2017

View reviewed changes

lpereira previously approved these changes Aug 29, 2017

View reviewed changes

chunlinhan reviewed Aug 30, 2017

View reviewed changes

andrewboie dismissed stale reviews from lpereira and dbkinder via e75b0ab August 30, 2017 20:02

andrewboie force-pushed the dwarf branch from fded2ef to e75b0ab Compare August 30, 2017 20:02

andyross previously approved these changes Aug 31, 2017

View reviewed changes

andrewboie dismissed andyross’s stale review via 1c67694 August 31, 2017 19:39

andrewboie force-pushed the dwarf branch from e75b0ab to 1c67694 Compare August 31, 2017 19:39

agross-oss reviewed Aug 31, 2017

View reviewed changes

Andrew Boie added 8 commits August 31, 2017 13:21

arm_irq_vector_table: don't change optimization

950a468

This is unnecessary. Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

tests: gen_isr_table: set default BOARD properly

48f2a75

This test doesn't work on x86, which doesn't use gen_isr_tables. Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

tests: gen_isr_table: don't set compiler opt

ac003a1

Instead, just set -O0 for the particular function which needs it. Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

doc: add gperf to workstation setup packages

7b30356

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

shippable: use new build slave configuration

aa236ba

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

sanitycheck: fix rw_sections whitespace

bdcc072

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

tests: add object validation test case

d157c4f

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

andrewboie force-pushed the dwarf branch from 1c67694 to d157c4f Compare August 31, 2017 20:21

andyross approved these changes Sep 7, 2017

View reviewed changes

andrewboie merged commit d76f0a9 into zephyrproject-rtos:master Sep 7, 2017

andrewboie deleted the dwarf branch September 7, 2017 23:33

This was referenced Sep 18, 2017

Validation mechanism for user-supplied kernel object pointers zephyriot/zep-jira14#2025

Closed

Validation mechanism for user-supplied kernel object pointers zephyriot/zephyr-issues#2025

Closed

zephyrbot mentioned this pull request Sep 23, 2017

Validation mechanism for user-supplied kernel object pointers #3625

Closed

nashif modified the milestones: v1.10, v1.10.0 Oct 3, 2017

		"""


		def write_gperf_table(fp, objs, static_begin, static_end):

kernel object validation infrastructure #1276

kernel object validation infrastructure #1276

Conversation

andrewboie commented Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

andrewboie commented Aug 29, 2017

nashif commented Aug 29, 2017

lpereira commented Aug 29, 2017

andrewboie commented Aug 29, 2017

dbkinder commented Aug 29, 2017

andrewboie commented Aug 29, 2017

nashif commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewboie commented Aug 30, 2017

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewboie commented Aug 31, 2017

Choose a reason for hiding this comment

andrewboie Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

agross-oss left a comment

Choose a reason for hiding this comment

andrewboie commented Aug 31, 2017

agross-oss commented Aug 31, 2017

andyross left a comment

Choose a reason for hiding this comment

andrewboie commented Aug 28, 2017 •

edited

Loading

andrewboie Aug 31, 2017 •

edited

Loading