On Tue, Apr 18, 2017 at 4:24 PM, Micka?l Sala?n [off-list ref] wrote:

On 19/04/2017 00:53, Kees Cook wrote:

quoted

On Tue, Mar 28, 2017 at 4:46 PM, Micka?l Sala?n [off-list ref] wrote:

quoted

+#ifdef CONFIG_SECCOMP_FILTER

Isn't CONFIG_SECCOMP_FILTER already required for landlock?

Yes it is, but Landlock could only/also be used through cgroups in the
future. :)

Hm, okay. I still feel like the configs could be more sensible. :)

quoted

quoted

@@ -1405,7 +1409,13 @@ static void copy_seccomp(struct task_struct *p)

        /* Ref-count the new filter user, and assign it. */
        get_seccomp_filter(current);
-       p->seccomp = current->seccomp;
+       p->seccomp.mode = current->seccomp.mode;
+       p->seccomp.filter = current->seccomp.filter;
+#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_SECURITY_LANDLOCK)
+       p->seccomp.landlock_events = current->seccomp.landlock_events;
+       if (p->seccomp.landlock_events)
+               atomic_inc(&p->seccomp.landlock_events->usage);
+#endif /* CONFIG_SECCOMP_FILTER && CONFIG_SECURITY_LANDLOCK */

Hrm. So, this needs config cleanup as above. Also, I'm going to need
some help understanding the usage tracking on landlock_events (which
should use a get/put rather than open-coding the _inc). I don't see
why individual assignments are needed here. The only thing that
matters is the usage bump. I would have expected no changes at all in
this code, actually. The filter and the events share the same usage
don't they?

Right, I can move the struct landlock_event into the struct
seccomp_filter. This should make the code cleaner.

No, that wasn't my point. I meant that since landlock_events is
already in ->seccomp, it's already copied by p->seccomp =
current->seccomp. The only thing you need is a
get_seccomp_landlock(current) call before the copy:

get_seccomp_filter(current);
get_seccomp_landlock(current);
p->seccomp = current->seccomp;

done! :)

And get_seccomp_landlock() can do a check for landlock_events existing, etc etc.

quoted

quoted

+       if (!new_events) {
+               /*
+                * If there is no Landlock events used by the current task,
+                * then create a new one.
+                */
+               new_events = new_landlock_events();
+               if (IS_ERR(new_events))
+                       goto put_rule;

Shouldn't bpf_prog_put() get called in the face of a rule failure too?
Why separate exit paths?

You're right but put_landlock_rule() call bpf_prog_put() by itself.

Ah! Missed that, thanks!

quoted

quoted

+       } else if (atomic_read(&current_events->usage) > 1) {
+               /*
+                * If the current task is not the sole user of its Landlock
+                * events, then duplicate them.
+                */
+               size_t i;
+
+               new_events = new_landlock_events();
+               if (IS_ERR(new_events))
+                       goto put_rule;
+               for (i = 0; i < ARRAY_SIZE(new_events->rules); i++) {
+                       new_events->rules[i] =
+                               lockless_dereference(current_events->rules[i]);
+                       if (new_events->rules[i])
+                               atomic_inc(&new_events->rules[i]->usage);

I was going to ask: isn't the top-level usage counter sufficient to
track rule lifetime? But I think I see how things are tracked now:
each task_struct points to an array of rule list pointers. These
tables are duplicated when additions are made (which means each table
needs to be refcounted for the processes using it), and when a new
table is created, all the refcounters on the rules are bumped (to
track each table that references the rule), and when a new rule is
added, it's just prepended to the list for the new table to point at.

That's right.

Okay, excellent. This should end up in a comment somewhere so when I
forget I can go read it again. ;)

quoted

Does this mean that rules are processed in reverse?

Yes, the rules are processed from the newest to the oldest, as
seccomp-bpf does with filters.

Cool. If not already mentioned, that should end up in the docs somewhere.

quoted

quoted

+       if (copy_from_user(&bpf_fd, user_bpf_fd, sizeof(bpf_fd)))
+               return -EFAULT;

I think this can just be get_user()?

Yes, I didn't know about that.

No worries. It's nice for small things. :)

quoted

quoted

+       prog = bpf_prog_get(bpf_fd);
+       if (IS_ERR(prog))
+               return PTR_ERR(prog);
+
+       /*
+        * We don't need to lock anything for the current process hierarchy,
+        * everything is guarded by the atomic counters.
+        */
+       new_events = landlock_append_prog(current->seccomp.landlock_events,
+                       prog);
+       /* @prog is managed/freed by landlock_append_prog() */

Does kmemcheck notice this "leak"? (i.e. is further annotation needed?)

I didn't enable kmemcheck, I will take a look at it.

Yeah, I'd turn on at least these while you're testing:

CONFIG_PROVE_LOCKING=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_DEBUG_KMEMLEAK=y

I'm sure people will suggest more, too. :)

-Kees

-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html