kernel: validate kernel objects #834

andrewboie · 2017-07-18T22:36:37Z

This will be used in conjunction with the kernel/userspace
privilege split, when a user thread makes a system call with
a kernel object as a parameter, the validity of the kernel
object pointer passed in will be confirmed, and cause a kernel
oops if it wasn't valid.

This is done by randomly generating a set of XOR keys for each
kernel object type, and when an object is initialized its
pointer value will be XOR'd with the key and stored within
the object itself. When an object is being validated,
XORing this value again with the key for the object type should
pop out the original pointer again.

We validate most, but not all kernel objects. We currently
omit k_mbox, k_queue, k_mem_pool as they appear to need some
redesign before we can use them from userspace due to how they
store data. JIRAs have been opened for each of these.

Issue: ZEP-2187
Signed-off-by: Andrew Boie andrew.p.boie@intel.com

andrewboie · 2017-07-18T22:38:34Z

I want to to turn this on by default for all the QEMU targets, but waiting on some fixes for GH-3830 before I can do that.

lpereira

I still have to review this thoroughly, but there are some nits.

lpereira · 2017-07-18T22:40:15Z

include/kernel.h

@@ -2136,6 +2191,7 @@ extern int k_delayed_work_cancel(struct k_delayed_work *work);
 */
 static inline void k_work_submit(struct k_work *work)
 {
+	_k_object_validate(work, K_OBJ_WORK);


This validates work twice. (It should only call _k_object_validate() before dereferencing the kernel object; if it's just passing it forward, let the other function validate it.)

lpereira · 2017-07-18T22:46:56Z

kernel/object_validate.c

+#ifdef CONFIG_PRINTK
+char *otype_to_str(enum k_objects otype)
+{
+	switch (otype) {


Using a const char *otype_str[] array will generate tighter code.

andrewboie · 2017-07-19T16:58:08Z

rebased and addressed @lpereira review comments

andyross · 2017-07-19T18:25:14Z

kernel/object_validate.c

+
+	initialized = 1;
+
+	for (i = 0; i < K_OBJ_LAST; i++) {


Don't see why you can't fold these into the same loop. Initialize the metadata, then loop over the defined objects.

This will be used in conjunction with the kernel/userspace privilege split, when a user thread makes a system call with a kernel object as a parameter, the validity of the kernel object pointer passed in will be confirmed, and cause a kernel oops if it wasn't valid. This is done by randomly generating a set of XOR keys for each kernel object type, and when an object is initialized its pointer value will be XOR'd with the key and stored within the object itself. When an object is being validated, XORing this value again with the key for the object type should pop out the original pointer again. We validate most, but not all kernel objects. We currently omit k_mbox, k_queue, k_mem_pool as they appear to need some redesign before we can use them from userspace due to how they store data. JIRAs have been opened for each of these. Issue: ZEP-2187 Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

This will help catch bugs when running sanitycheck if an application or subsystem tries to make kernel calls on something that is not a valid kernel object. This adds a bit of overhead but we don't gather performance metrics in QEMU anyway, it's too dependent on the characteristics of the host system. Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

andrewboie · 2017-07-19T19:54:25Z

Addressed @andyross feedback and added a more helpful error message if an app defines a kernel object in non-kernel memory.

andyross · 2017-07-19T21:09:05Z

kernel/object_validate.c

+		struct object_metadata *o = &ometa[i];
+
+#if CONFIG_RANDOM_HAS_DRIVER
+		o->xor_key = sys_rand32_get();


Thinking about this, we don't want a random number here. Think of the requirements:

It should be "white" relative to the the space of pointers and the other xor_key values. A change in any bit of the pointer should be ~50% likely to invalidate the match. It shouldn't be "too close" to the other keys.

It should not be the same as any other xor_key

A RNG will meet those only statistically. And worse, because it's runtime-dependent (on systems with hardware entropy anyway) we're pretty much guaranteed that eventually a system will boot up having hit the birthday jackpot and have colliding (or even partially colliding, which is still bad) keys.

We can do better by precomputing it at build time. A quick python script that does the RNG and tests for validity before emitting C code to define the ometa[] array would be easy and simplify the boot code (heh) to boot.

@andyross good point, but I fear static keys only makes the situation worse, as soon as someone discovers the set of static XOR keys by just reading them out the flash, or JTAG, or whatever, that will enable every script kiddie to hack every single device out there.

not seeing a good way out of this :/

Well, yeah. It's not really a security feature anyway. But the complaint above was about robustness, it's possible to boot in a configuration where the validity check doesn't work right.

FWIW: you could do both: keep a single/global per-boot randomized key and a per-type value and xor with both (potentially pre-computing the xor_key values in place, assuming they're in RAM as they are now). Probably not worth it.

If we admit that this is totally insecure we might as well not have random numbers at all, and just use the loop counter (basically just the !CONFIG_RANDOM_HAS_DRIVER case) maybe with some math done on it to ensure not too close to other keys.

i guess let this patch be a prototype of the pointer-based API model. i'm going to prototype in another patch what a descriptor-based validation mechanism would look like, I think it will at least inform the discussion.

You could do multiple layers very easily
You could have keys per thread, per boot and per type ... and they all have to match in the right order. That significantly increases the difficulty of making a guess.

@inakypg that falls apart if you need to grant more than one thread access. if you grant exactly one thread access, sure you can just xor its struct k_thread pointer in. but multiple threads, not gonna work, you can't just XOR in all their pointers in. in practice we are going to need to grant multiple threads access to some object all the time.

For the record so we don't drive anyone crazy:

https://jira.zephyrproject.org/browse/ZEP-2408?focusedCommentId=19363&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-19363

where we are having that discussion :)

andrewboie · 2017-07-20T20:53:49Z

Investigating a different approach. Closing for now.

andrewboie requested review from nashif, lpereira, mbolivar, d3zd3z, andyross, jhedberg, ruuddw, inakypg, galak, AdithyaBaglody and agross-oss July 18, 2017 22:36

andrewboie force-pushed the kobject branch from 50bce8b to 5b6b5e3 Compare July 18, 2017 23:19

lpereira reviewed Jul 18, 2017

View reviewed changes

andrewboie force-pushed the kobject branch from 5b6b5e3 to 527992c Compare July 19, 2017 16:54

andrewboie force-pushed the kobject branch from 527992c to 6e54196 Compare July 19, 2017 17:44

andyross reviewed Jul 19, 2017

View reviewed changes

Andrew Boie added 2 commits July 19, 2017 12:52

andrewboie force-pushed the kobject branch from 6e54196 to f768509 Compare July 19, 2017 19:53

andyross reviewed Jul 19, 2017

View reviewed changes

andrewboie closed this Jul 20, 2017

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

andrewboie deleted the kobject branch May 31, 2018 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: validate kernel objects #834

kernel: validate kernel objects #834

andrewboie commented Jul 18, 2017

andrewboie commented Jul 18, 2017 •

edited by zephyrbot

Loading

lpereira left a comment

lpereira Jul 18, 2017

lpereira Jul 18, 2017

andrewboie commented Jul 19, 2017

andyross Jul 19, 2017

andrewboie commented Jul 19, 2017

andyross Jul 19, 2017

andrewboie Jul 19, 2017

andyross Jul 19, 2017

andrewboie Jul 19, 2017

inakypg Jul 19, 2017

andrewboie Jul 19, 2017

inakypg Jul 19, 2017

andrewboie commented Jul 20, 2017

kernel: validate kernel objects #834

kernel: validate kernel objects #834

Conversation

andrewboie commented Jul 18, 2017

andrewboie commented Jul 18, 2017 • edited by zephyrbot Loading

lpereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewboie commented Jul 19, 2017

Choose a reason for hiding this comment

andrewboie commented Jul 19, 2017

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 Jul 20, 2017

andrewboie commented Jul 18, 2017 •

edited by zephyrbot

Loading