On Tue, Feb 4, 2020 at 5:52 PM Richard Guy Briggs [off-list ref] wrote:

On 2020-01-22 16:28, Paul Moore wrote:

quoted

On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs [off-list ref] wrote:

quoted

Store the audit container identifier in a refcounted kernel object that
is added to the master list of audit container identifiers.  This will
allow multiple container orchestrators/engines to work on the same
machine without danger of inadvertantly re-using an existing identifier.
It will also allow an orchestrator to inject a process into an existing
container by checking if the original container owner is the one
injecting the task.  A hash table list is used to optimize searches.

Signed-off-by: Richard Guy Briggs <redacted>
---
 include/linux/audit.h | 14 ++++++--
 kernel/audit.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/audit.h        |  8 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)

...

quoted

quoted

@@ -232,7 +263,11 @@ int audit_alloc(struct task_struct *tsk)
        }
        info->loginuid = audit_get_loginuid(current);
        info->sessionid = audit_get_sessionid(current);
-       info->contid = audit_get_contid(current);
+       spin_lock(&audit_contobj_list_lock);
+       info->cont = _audit_contobj(current);
+       if (info->cont)
+               _audit_contobj_hold(info->cont);
+       spin_unlock(&audit_contobj_list_lock);

If we are taking a spinlock in order to bump the refcount, does it
really need to be a refcount_t or can we just use a normal integer?
In RCU protected lists a spinlock is usually used to protect
adds/removes to the list, not the content of individual list items.

My guess is you probably want to use the spinlock as described above
(list add/remove protection) and manipulate the refcount_t inside a
RCU read lock protected region.

Ok, I guess it could be an integer if it were protected by the spinlock,
but I think you've guessed my intent, so let us keep it as a refcount
and tighten the spinlock scope and use rcu read locking to protect _get
and _put in _alloc, _free, and later on when protecting the network
namespace contobj lists.  This should reduce potential contention for
the spinlock to one location over fewer lines of code in that place
while speeding up updates and slightly simplifying code in the others.

If it helps, you should be able to find plenty of rcu/spinlock
protected list examples in the kernel code.  It might be a good idea
if you spent some time looking at those implementations first to get
an idea of how it is usually done.

quoted

quoted

@@ -2381,9 +2425,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        }
        oldcontid = audit_get_contid(task);
        read_lock(&tasklist_lock);
-       /* Don't allow the audit containerid to be unset */
+       /* Don't allow the contid to be unset */
        if (!audit_contid_valid(contid))
                rc = -EINVAL;
+       /* Don't allow the contid to be set to the same value again */
+       else if (contid == oldcontid) {
+               rc = -EADDRINUSE;

First, is that brace a typo?  It looks like it.  Did this compile?

Yes, it was fixed in the later patch that restructured the if
statements.

Generic reminder that each patch should compile and function on it's
own without the need for any follow-up patches.  I know Richard is
already aware of that, and this was a mistake that slipped through the
cracks; this reminder is more for those who may be lurking on the
list.

quoted

Second, can you explain why (re)setting the audit container ID to the
same value is something we need to prohibit?  I'm guessing it has
something to do with explicitly set vs inherited, but I don't want to
assume too much about your thinking behind this.

It made the refcounting more complicated later, and besides, the
prohibition on setting the contid again if it is already set would catch
this case, so I'll remove it in this patch and ensure this action
doesn't cause a problem in later patches.

quoted

If it is "set vs inherited", would allowing an orchestrator to
explicitly "set" an inherited audit container ID provide some level or
protection against moving the task?

I can't see it helping prevent this since later descendancy checks will
stop this move anyways.

That's what I thought, but I was just trying to think of any reason
why you felt this might have been useful since it was in the patch.
If it's in the patch I tend to fall back on the idea that it must have
served a purpose ;)

quoted

quoted

@@ -2396,8 +2443,49 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        else if (audit_contid_set(task))
                rc = -ECHILD;
        read_unlock(&tasklist_lock);
-       if (!rc)
-               task->audit->contid = contid;
+       if (!rc) {
+               struct audit_contobj *oldcont = _audit_contobj(task);
+               struct audit_contobj *cont = NULL, *newcont = NULL;
+               int h = audit_hash_contid(contid);
+
+               rcu_read_lock();
+               list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
+                       if (cont->id == contid) {
+                               /* task injection to existing container */
+                               if (current == cont->owner) {

Do we have any protection against the task pointed to by cont->owner
going away and a new task with the same current pointer value (no
longer the legitimate audit container ID owner) manipulating the
target task's audit container ID?

Yes, the get_task_struct() call below.

Gotcha.

-- 
paul moore
www.paul-moore.com