Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > Add container ID support to ptrace and signals. In particular, the "op" > field provides a way to label the auxiliary record to which it is > associated. > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > --- > include/linux/audit.h | 16 +++++++++++----- > kernel/audit.c | 12 ++++++++---- > kernel/audit.h | 2 ++ > kernel/auditsc.c | 19 +++++++++++++++---- > 4 files changed, 36 insertions(+), 13 deletions(-) ... > diff --git a/kernel/audit.c b/kernel/audit.c > index a12f21f..b238be5 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -142,6 +142,7 @@ struct audit_net { > kuid_t audit_sig_uid = INVALID_UID; > pid_t audit_sig_pid = -1; > u32 audit_sig_sid = 0; > +u64 audit_sig_cid = INVALID_CID; > > /* Records can be lost in several ways: > 0) [suppressed in audit_alloc] > @@ -1438,6 +1439,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > memcpy(sig_data->ctx, ctx, len); > security_release_secctx(ctx, len); > } > + sig_data->cid = audit_sig_cid; > audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0, > sig_data, sizeof(*sig_data) + len); > kfree(sig_data); > @@ -2051,20 +2053,22 @@ void audit_log_session_info(struct audit_buffer *ab) > > /* > * audit_log_container_info - report container info > - * @tsk: task to be recorded > * @context: task or local context for record > + * @op: containerid string description > + * @containerid: container ID to report > */ > -int audit_log_container_info(struct task_struct *tsk, struct audit_context *context) > +int audit_log_container_info(struct audit_context *context, > + char *op, u64 containerid) > { > struct audit_buffer *ab; > > - if (!audit_containerid_set(tsk)) > + if (!cid_valid(containerid)) > return 0; > /* Generate AUDIT_CONTAINER_INFO with container ID */ > ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_INFO); > if (!ab) > return -ENOMEM; > - audit_log_format(ab, "contid=%llu", audit_get_containerid(tsk)); > + audit_log_format(ab, "op=%s contid=%llu", op, containerid); > audit_log_end(ab); > return 0; > } Let's get these changes into the first patch where audit_log_container_info() is defined. Why? This inserts a new field into the record which is a no-no. Yes, it is one single patchset, but they are still separate patches and who knows which patches a given distribution and/or tree may decide to backport.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 2bba324..2932ef1 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -113,6 +113,7 @@ struct audit_aux_data_pids { > kuid_t target_uid[AUDIT_AUX_PIDS]; > unsigned int target_sessionid[AUDIT_AUX_PIDS]; > u32 target_sid[AUDIT_AUX_PIDS]; > + u64 target_cid[AUDIT_AUX_PIDS]; > char target_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN]; > int pid_count; > }; > @@ -1422,21 +1423,27 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts > 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++) { > + char axsn[sizeof("aux0xN ")]; > + > + sprintf(axsn, "aux0x%x", i); > if (audit_log_pid_context(context, axs->target_pid[i], > axs->target_auid[i], > axs->target_uid[i], > axs->target_sessionid[i], > axs->target_sid[i], > - axs->target_comm[i])) > + axs->target_comm[i]) > + && audit_log_container_info(context, axsn, axs->target_cid[i])) Shouldn't this be an OR instead of an AND?

> call_panic = 1; > + } > } > > if (context->target_pid && > audit_log_pid_context(context, context->target_pid, > context->target_auid, context->target_uid, > context->target_sessionid, > - context->target_sid, context->target_comm)) > + context->target_sid, context->target_comm) > + && audit_log_container_info(context, "target", context->target_cid)) Same question.

> call_panic = 1; > > if (context->pwd.dentry && context->pwd.mnt) { -- paul moore www.paul-moore.com