On 8/5/2025 12:39 PM, Paul Moore wrote:
> On Jul 16, 2025 Casey Schaufler <[email protected]> wrote:
>> Replace the single skb pointer in an audit_buffer with a list of
>> skb pointers. Add the audit_stamp information to the audit_buffer as
>> there's no guarantee that there will be an audit_context containing
>> the stamp associated with the event. At audit_log_end() time create
>> auxiliary records as have been added to the list. Functions are
>> created to manage the skb list in the audit_buffer.
>>
>> Create a new audit record AUDIT_MAC_TASK_CONTEXTS.
>> An example of the MAC_TASK_CONTEXTS record is:
>>
>> type=MAC_TASK_CONTEXTS
>> msg=audit(1600880931.832:113)
>> subj_apparmor=unconfined
>> subj_smack=_
>>
>> When an audit event includes a AUDIT_MAC_TASK_CONTEXTS record the
>> "subj=" field in other records in the event will be "subj=?".
>> An AUDIT_MAC_TASK_CONTEXTS record is supplied when the system has
>> multiple security modules that may make access decisions based on a
>> subject security context.
>>
>> Refactor audit_log_task_context(), creating a new audit_log_subj_ctx().
>> This is used in netlabel auditing to provide multiple subject security
>> contexts as necessary.
>>
>> Suggested-by: Paul Moore <[email protected]>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> ---
>> include/linux/audit.h | 16 +++
>> include/uapi/linux/audit.h | 1 +
>> kernel/audit.c | 207 +++++++++++++++++++++++++++++------
>> net/netlabel/netlabel_user.c | 9 +-
>> security/apparmor/lsm.c | 3 +
>> security/lsm.h | 4 -
>> security/lsm_init.c | 5 -
>> security/security.c | 3 -
>> security/selinux/hooks.c | 3 +
>> security/smack/smack_lsm.c | 3 +
>> 10 files changed, 202 insertions(+), 52 deletions(-)
> If there were no other issues with this patch I would have just fixed
> this up during the merge (I did it in my review branch already), but
> since you're no longer dependent on the LSM init rework changes (and
> I've dropped the subj/obj counting in the latest revision), just go
> ahead and base your next revision on the audit tree or Linus' tree as
> one normally would.
OK. Can I assume that the dependency on the init changes has dropped?
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 226c8ae00d04..c7dea6bfacdd 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
> ..
>
>> +/**
>> + * audit_log_subj_ctx - Add LSM subject information
>> + * @ab: audit_buffer
>> + * @prop: LSM subject properties.
>> + *
>> + * Add a subj= field and, if necessary, a AUDIT_MAC_TASK_CONTEXTS record.
>> + */
>> +int audit_log_subj_ctx(struct audit_buffer *ab, struct lsm_prop *prop)
>> {
>> - struct lsm_prop prop;
>> struct lsm_context ctx;
>> + char *space = "";
>> int error;
>> + int i;
>>
>> - security_current_getlsmprop_subj(&prop);
>> - if (!lsmprop_is_set(&prop))
>> + security_current_getlsmprop_subj(prop);
>> + if (!lsmprop_is_set(prop))
>> return 0;
>>
>> - error = security_lsmprop_to_secctx(&prop, &ctx, LSM_ID_UNDEF);
>> - if (error < 0) {
>> - if (error != -EINVAL)
>> - goto error_path;
>> + if (audit_subj_secctx_cnt < 2) {
>> + error = security_lsmprop_to_secctx(prop, &ctx, LSM_ID_UNDEF);
>> + if (error < 0) {
>> + if (error != -EINVAL)
>> + goto error_path;
>> + return 0;
>> + }
>> + audit_log_format(ab, " subj=%s", ctx.context);
>> + security_release_secctx(&ctx);
>> return 0;
>> }
>> -
>> - audit_log_format(ab, " subj=%s", ctx.context);
>> - security_release_secctx(&ctx);
>> + /* Multiple LSMs provide contexts. Include an aux record. */
>> + audit_log_format(ab, " subj=?");
>> + error = audit_buffer_aux_new(ab, AUDIT_MAC_TASK_CONTEXTS);
>> + if (error)
>> + goto error_path;
>> +
>> + for (i = 0; i < audit_subj_secctx_cnt; i++) {
>> + error = security_lsmprop_to_secctx(prop, &ctx,
>> + audit_subj_lsms[i]->id);
>> + if (error < 0) {
>> + /*
>> + * Don't print anything. An LSM like BPF could
>> + * claim to support contexts, but only do so under
>> + * certain conditions.
>> + */
>> + if (error == -EOPNOTSUPP)
>> + continue;
>> + if (error != -EINVAL)
>> + audit_panic("error in audit_log_task_context");
> Argh ... please read prior review comments a bit more carefully. As was
> pointed out in the v4 posting you're using the wrong function name here.
>
> https://lore.kernel.org/audit/[email protected]
Yeah, that was sloppy. Sorry. Will fix.
>> + } else {
>> + audit_log_format(ab, "%ssubj_%s=%s", space,
>> + audit_subj_lsms[i]->name, ctx.context);
>> + space = " ";
>> + security_release_secctx(&ctx);
>> + }
>> + }
>> + audit_buffer_aux_end(ab);
>> return 0;
>>
>> error_path:
>> - audit_panic("error in audit_log_task_context");
>> + audit_panic("error in audit_log_subj_ctx");
>> return error;
>> }
>> +EXPORT_SYMBOL(audit_log_subj_ctx);
> ..
>
>> @@ -2423,25 +2575,16 @@ int audit_signal_info(int sig, struct task_struct *t)
>> void audit_log_end(struct audit_buffer *ab)
>> {
>> struct sk_buff *skb;
>> - struct nlmsghdr *nlh;
>>
>> if (!ab)
>> return;
>>
>> - if (audit_rate_check()) {
>> - skb = ab->skb;
>> - ab->skb = NULL;
>> + while ((skb = skb_dequeue(&ab->skb_list)))
>> + __audit_log_end(skb);
>>
>> - /* setup the netlink header, see the comments in
>> - * kauditd_send_multicast_skb() for length quirks */
>> - nlh = nlmsg_hdr(skb);
>> - nlh->nlmsg_len = skb->len - NLMSG_HDRLEN;
>> -
>> - /* queue the netlink packet and poke the kauditd thread */
>> - skb_queue_tail(&audit_queue, skb);
>> + /* poke the kauditd thread */
>> + if (audit_rate_check())
>> wake_up_interruptible(&kauditd_wait);
>> - } else
>> - audit_log_lost("rate limit exceeded");
> .. here is another case where you've missed/ignored previous feedback.
> I believe this is the second revision in the history of this patchset
> where you've missed feedback; *please* try to do better Casey, stuff like
> this wastes time and drags things out longer than needed.
>
> https://lore.kernel.org/audit/[email protected]
My bad. I've been staring at this patch for over five years and sometimes
can't see where it has, hasn't or needs to be changed. I mucked this up
while pulling the indentation fixes into the separate patch.
>> audit_buffer_free(ab);
>> }
> --
> paul-moore.com
>
