Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls

On Wed, Jul 29, 2020 at 3:41 PM Richard Guy Briggs [off-list ref] wrote:

On 2020-07-05 11:10, Paul Moore wrote:

quoted

On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs [off-list ref] wrote:

...

quoted

quoted

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f03d3eb0752c..9e79645e5c0e 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c

@@ -1458,6 +1466,7 @@ static void audit_log_exit(void)
        struct audit_buffer *ab;
        struct audit_aux_data *aux;
        struct audit_names *n;
+       struct audit_contobj *cont;

        context->personality = current->personality;

@@ -1541,7 +1550,7 @@ static void audit_log_exit(void)
        for (aux = context->aux_pids; aux; aux = aux->next) {
                struct audit_aux_data_pids *axs = (void *)aux;

-               for (i = 0; i < axs->pid_count; i++)
+               for (i = 0; i < axs->pid_count; i++) {
                        if (audit_log_pid_context(context, axs->target_pid[i],
                                                  axs->target_auid[i],
                                                  axs->target_uid[i],

@@ -1549,14 +1558,20 @@ static void audit_log_exit(void)
                                                  axs->target_sid[i],
                                                  axs->target_comm[i]))
                                call_panic = 1;
+                       audit_log_container_id(context, axs->target_cid[i]);
+               }

It might be nice to see an audit event example including the
ptrace/signal information.  I'm concerned there may be some confusion
about associating the different audit container IDs with the correct
information in the event.

This is the subject of ghat81, which is a test for ptrace and signal
records.

This was the reason I had advocated for an op= field since there is a
possibility of multiple contid records per event.

I think an "op=" field is the wrong way to link audit container ID to
a particular record.  It may be convenient, but I fear that it would
be overloading the field too much.

Like I said above, I think it would be good to see an audit event
example including the ptrace/signal information.  This way we can talk
about it on-list and hash out the various solutions if it proves to be
a problem.

quoted

quoted

@@ -1575,6 +1590,14 @@ static void audit_log_exit(void)

        audit_log_proctitle();

+       rcu_read_lock();
+       cont = _audit_contobj_get(current);
+       rcu_read_unlock();
+       audit_log_container_id(context, cont);
+       rcu_read_lock();
+       _audit_contobj_put(cont);
+       rcu_read_unlock();

Do we need to grab an additional reference for the audit container
object here?  We don't create any additional references here that
persist beyond the lifetime of this function, right?

Why do we need another reference?  There's one for each pointer pointing
to it and so far we have just one from this task.  Or are you thinking
of the contid hash list, which is only added to when a task points to it
and gets removed from that list when the last task stops pointing to it.
Later that gets more complicated with network namespaces and nested
container objects.  For now we just needed it while generating the
record, then it gets freed.

I don't think we need to grab an additional reference here, that is
why I asked the question.  The code above grabs a reference for the
audit container ID object associated with the current task and then
drops it before returning; if the current task, and it's associated
audit container ID object, disappears in the middle of the function
we've got much bigger worries :)

-- 
paul moore
www.paul-moore.com