Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes

On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler <casey(a)schaufler-ca.com&gt; wrote: >> On 8/20/2021 12:06 PM, Paul Moore wrote: >>> Unless you explicitly enable audit on the kernel cmdline, e.g. >>> "audit=1", processes started before userspace enables audit will not >>> have a properly allocated audit_context; see the "if >>> (likely(!audit_ever_enabled))" check at the top of audit_alloc() for >>> the reason why. > I found a hack-around that no one will like. I changed that check to be > > (likely(!audit_ever_enabled) && !lsm_multiple_contexts()) > > It probably introduces a memory leak and/or performance degradation, > but it has the desired affect. I can't speak for everyone, but I know I don't like that as a solution ;) I imagine such a change would also draw the ire of the never-audit crowd and the distros themselves. However, I understand the need to just get *something* in place so you can continue to test/develop; it's fine to keep that for now, but I'm going to be very disappointed if that line finds its way into the next posted patchset revision.

I'm very much open to ideas but my gut feeling is that the end solution is going to be changes to audit_log_start() and audit_log_end(). In my mind the primary reason for this hunch is that support for multiple LSMs[*] needs to be transparent to the various callers in the kernel; this means the existing audit pattern of ... audit_log_start(...); audit_log_format(...); audit_log_end(...); ... should be preserved and be unchanged from what it is now. We've already talked in some general terms about what such changes might look like, but to summarize the previous discussions, I think we would need to think about the following things:

* Adding a timestamp/serial field to the audit_buffer struct, it could even be in a union with the audit_context pointer as we would never need both at the same time: if the audit_context ptr is non-NULL you use that, otherwise you use the timestamp. The audit_buffer timestamp would not eliminate the need for the timestamp info in the audit_context struct for what I hope are obvious reasons. * Additional logic in audit_log_end() to generate additional ancillary records for LSM labels. This will likely mean storing the message "type" passed to audit_log_start() in the audit_record struct and using that information in audit_log_end() to decide if a LSM ancillary record is needed. Logistically this would likely mean converting the existing audit_log_end() function into a static/private __audit_log_end() and converting audit_log_end() into something like this: void audit_log_end(ab) { __audit_log_end(ab); // rm the ab cleanup from __audit_log_end if (ab->anc_mlsm) { // generate the multiple lsm record } audit_buffer_free(ab); } * Something else that I'm surely forgetting :) At the end of all this we may find that the "local" audit_context concept is no longer needed. It was originally created to deal with grouping ancillary records with non-syscall records into a single event; while what we are talking about above is different, I believe that our likely solution will also work to solve the original "local" audit_context use case as well. We'll have to see how this goes.

[*] I expect that the audit container ID work will have similar issues and need a similar solution, I'm surprised it hasn't come up yet.