Audit container ID patchset v11 comments

Hi Richard and audit aficionados, Now that the io_uring fixes are in linux-next and no one is screaming too loudly about it, it's time to turn the focus back to the audit container ID patchset. Unfortunately I've lost Richard's v11 draft from my inbox so I'm going to provide my feedback in this mail, broken up into sections for each patch; hopefully it isn't too painful to read ... *** [PATCH ghak90 v11 01/11] audit: collect audit task parameters - This would come out during the forward-port/rebase, but the io_uring specific accessors are no longer needed. - Minor stuff, but if audit_alloc() returns the tsk->audit pointer you could simplify things a bit, see the the audit_alloc() case below. I also moved the loginuid/sessionid setting into audit_alloc() so we do less work in the audit_set_loginuid() case. static audit_task_info *audit_alloc_task(struct task_struct *tsk) { if (!tsk->audit) tsk->audit = kmem_cache_alloc(audit_task_cache, GFP_KERNEL); return tsk->audit; } int audit_alloc(struct task_struct *tsk) { int rc; struct audit_task_info *ati; ati = audit_alloc_task(tsk); if (!ati) return -ENOMEM; ati->loginuid = audit_get_loginuid(current); ati->sessionid = audit_get_sessionid(current); rc = audit_alloc_syscall(tsk); if (rc) { tsk->audit = NULL; kmem_cache_free(audit_task_cache, ati); } return rc; } - I'm keeping this comment separate from the one above, although it is very much related and could simplify things further. Since we are calling audit_alloc_task() from copy_process (copy_process -> audit_alloc -> audit_alloc_task) and copy_process will fail to create the process if audit_alloc()/audit_alloc_task() fails, why not fold audit_alloc_task() into audit_alloc() and just drop all of the other audit_alloc_task() calls? Unless the kernel completely fails, all of the other audit_alloc_task() callers should already have the current->audit field allocated. Can you think of any case where this wouldn't work? Am I missing something (maybe in the later patches)? - Related to the above, if we assume that a valid task_struct *always* has a task_struct->audit field then there are a number of task_struct->audit checks in this patchset that can probably be removed. *** [PATCH ghak90 v11 02/11] audit: add container id - Perhaps it gets used more in later patches, but audit_get_contid() is only used by one caller and it seems like it could be easily replaced by _audit_contobj_get() and a dereference. Doing so would shrink/simplify the patch a bit. - I noticed that the audit_set_contid() function definition comment block mentions setting the contid on the "current" task, which isn't always correct, see proc_contid_write() and the fact that it takes an arbitrary task_struct as a parameter. - Another minor point, but when injecting the task into an existing container you can leverage the _audit_contobj_get() return value (and do some other simplifications), for example: /* task injection to existing container */ if (current != cont->owner) { spin_unlock(&_audit_contobj_list_lock); rc = -ENOTUNIQ; goto error; } newcont = _audit_contobj_get(cont); - I might change the "error" jump label in audit_set_contid() to "out" since that code is executed in both the normal and error paths. - You are calling _audit_contobj_put(oldcont) twice at the end of audit_set_contid() in the normal/non-error case; this doesn't seem correct. - The RCU read lock taken around _audit_contobj_get_bytask() in audit_alloc_task() could probably be moved inside audit_alloc_task() - although see my comments for patch #1 - as anyone getting a reference should probably be holding the RCU lock. - The locking around and inside of _audit_contobj_put() is interesting. We should consider moving the spinlock inside the function similar to the RCU lock and _audit_contobj_get_bytask() comment above, but I think we also might be able to help limit when the spinlock is taken as well. There are also some corner cases with respect to contobj lifetimes and owner references that need correction. I believe something like this should improve things: static void audit_contobj_free(struct rcu_head *entry) { struct audit_contobj *cont = container_of(entry, struct audit_contobj, rcu); put_task_struct(cont->owner); kfree(cont); } static void _audit_contobj_put(struct audit_contobj *cont) { if (!cont) return; if (!refcount_dec_and_lock(&cont->refcount, &_audit_contobj_list_lock)) return; list_del_rcu(&cont->list); spin_unlock(&_audit_contobj_list_lock); call_rcu(&cont->rcu, audit_contobj_free); } int audit_set_contid(struct task_struct *tsk, u64 contid) { /* ... */ /* NOTE: rcu lock, tasklist lock, and task lock already held */ h = audit_hash_contid(contid); list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) { if (refcount_read(&cont->refcount) == 0) continue; if (cont->id == contid) { /* task injection to existing container */ if (current != cont->owner) { rc = -ENOTUNIQ; /* cleanup, error handling, etc. */ } newcont = _audit_contobj_get(cont); break; } } if (!newcont) { newcont = kmalloc(sizeof(*newcont), GFP_ATOMIC); if (!newcont) { rc = -ENOMEM; /* cleanup, error handling, etc. */ } INIT_LIST_HEAD(&newcont->list); newcont->id = contid; newcont->owner = get_task_struct(current); refcount_set(&newcont->refcount, 1); spin_lock(&_audit_contobj_list_lock); list_add_rcu(&newcont->list, &audit_contid_hash[h]); spin_unlock(&_audit_contobj_list_lock); } info->count = newcont; /* ... */ } *** [PATCH ghak90 v11 03/11] audit: log container info of syscalls - It would be good to explain the significance of the "record=" field in the AUDIT_CONTAINER_ID record in this patch. Currently it is just an increasing counter with no linkage to anything, if it has meaning then it should be documented here, if not then we should consider dropping it. - It looks like you created audit_contobj_get_bytask() here because audit_contobj is defined in audit.c; if you defined audit_contobj and declared the necessary supporting functions in kernel/audit.h these shenanigans wouldn't be necessary, correct? I suspect this would improve/eliminate a number of other functions in this patch too. - Nit picky, but I wonder if audit_contobj_put() would make more sense named audit_contobj_mput()? - Why does audit_log_container_id_ctx() take an audit_context parameter when it always internally references the current task for the audit container ID? Why not simply take the audit_context from the current task_struct? *** [PATCH ghak90 v11 04/11] audit: add contid support for signalling the audit daemon - The signal caller reset in audit_free() is likely not what we want as a task could signal/kill auditd and exit, clearing it's info before auditd could request the signal caller info. We should be able to remove the audit_sig_adtsk variable as it is unnecessary. - I think the only time we should "put" the audit_sig_cid contobj is when auditd requests the AUDIT_SIGNAL_INFO or AUDIT_SIGNAL_INFO2 info (clear it in both cases as userspace might not be acid aware). - Locking and refcounting of the contobj in the signal case is tricky, we've talked about that in past drafts, and it's complicated here by some locking that doesn't seem correct in all cases (see my comments regarding patch #3). Working in the suggestions from patch #3 I end up with something like this: static void _audit_contobj_put(struct audit_contobj *cont) { if (!cont) return; if (!refcount_dec_and_lock(&cont->refcount, &_audit_contobj_list_lock)) return; list_del_rcu(&cont->list); spin_unlock(&_audit_contobj_list_lock); if (refcount_read(&cont->sigcount) == 0) call_rcu(&cont->rcu, audit_contobj_free); } static struct audit_contobj *_audit_contobj_get_sig(void) { struct audit_cont_obj *cont = current->audit->cont; refcount_inc(&cont->sigcount); return cont; } static void _audit_contobj_put_sig(struct audit_contobj *cont) { if (refcount_dec_and_test(&cont->sigcount) == 0 && refcount_read(&cont->refcount) == 0)) audit_contobj_free(cont->rcu); } *** [PATCH ghak90 v11 05/11] audit: add support for non-syscall auxiliary records - Porting needed due to the io_uring changes, but it seems okay otherwise. *** [PATCH ghak90 v11 06/11] audit: add containerid support for user records - Nothing to say here. *** [PATCH ghak90 v11 07/11] audit: add containerid filtering - Likely porting is needed, but okay otherwise. *** [PATCH ghak90 v11 08/11] audit: add support for containerid to network namespaces - While the atomicity of refcount is overkill here, it might be wise to use refcount_t in place of the integer counter in audit_contobj_netns so we get all the nice rollover, etc. protections. - There are some interesting locking choices going on here, but I think some of the changes in the prior patches are going to have an impact here so I'm not going to look too hard at that right now, I'll revisit it in the next revision. - See my previous comments about the "record=" field, they apply here as well. *** [PATCH ghak90 v11 09/11] audit: contid check descendancy and nesting - I realize it's tricky, but the commit description could use a bit more work to make the relationships between the current task, the target task, and the audit container ID owner more clear. Examples are never a bad thing. - The changes in audit_free() look to be misplaced? No? *** [PATCH ghak90 v11 10/11] audit: track container nesting - In the commit description the network namespace example talks about "... spawning peer tasks 2 and 3 ..." when I think it should read "... spawning peer tasks in audit container ID 2 and 3 ...". - Is the change in audit_receive_msg(AUDIT_SIGNAL_INFO2) misplaced? - AUDIT_MESSAGE_TEXT_MAX in audit_receive_msg() is *really* big, like multiple pages big on a 4k PAGE_SIZE system, perhaps start with something like "(AUDIT_MESSAGE_TEXT_MAX < PAGE_SIZE ? AUDIT_MESSAGE_TEXT_MAX : PAGE_SIZE)"? - In audit_set_contid() couldn't you use "current" in the call to _audit_contobj_get_bytask()? - It looks like you drop the audit_contobj->parent reference in the signal caller case, but I don't see where you drop it in the normal case? I suspect it would be in the same place where you drop audit_contobj->owner. - Perhaps I've been looking at these patches too long today, but can you explain why the change to audit_contid_comparator is necessary? It's not clear to me at the moment. *** [PATCH ghak90 v11 11/11] audit: add capcontid to set contid outside init_user_ns - The access control logic in audit_set_capcontid() doesn't seem to match the commit description, I was expecting something more like this: int audit_set_capcontid(...) { /* ... */ rc = -EPERM; if (tsk == current || !task_is_descendant(current, tsk)) rc = -EXDEV; else if (current_user_ns() == &init_user_ns && capable(CAP_AUDIT_CONTROL)) rc = 0; else if (audit_get_capcontid(current)) rc = 0; if (!rc) info->capcontid = !!enable; /* ... */ } -- paul moore www.paul-moore.com