Re: [PATCH ghak90 V9 04/13] audit: log drop of contid on exit of last task
On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
>
> Since we are tracking the life of each audit container indentifier, we
> can match the creation event with the destruction event. Log the
> destruction of the audit container identifier when the last process in
> that container exits.
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/audit.c | 20
> kernel/audit.h | 2 ++
> kernel/auditsc.c | 2 ++
> 3 files changed, 24 insertions(+)
If you end up respinning this patchset it seems like this should be
merged in with patch 2/13. This way patch 2/13 would include both the
"set" and "drop" records, making that patch a bit more useful on it's
own.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 6d387793f702..9e0b38ce1ead 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2558,6 +2558,26 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> return rc;
> }
>
> +void audit_log_container_drop(void)
> +{
> + struct audit_buffer *ab;
> + struct audit_contobj *cont;
> +
> + rcu_read_lock();
> + cont = _audit_contobj_get(current);
> + _audit_contobj_put(cont);
> + if (!cont || refcount_read(>refcount) > 1)
> + goto out;
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
You may want to check on sleeping with RCU locks held, or just use
GFP_ATOMIC to be safe.
> + if (!ab)
> + goto out;
> + audit_log_format(ab, "op=drop opid=%d contid=%llu old-contid=%llu",
> +task_tgid_nr(current), cont->id, cont->id);
> + audit_log_end(ab);
> +out:
> + rcu_read_unlock();
> +}
> +
> /**
> * audit_log_end - end one audit record
> * @ab: the audit_buffer
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 182fc76ea276..d07093903008 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -254,6 +254,8 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
> extern struct tty_struct *audit_get_tty(void);
> extern void audit_put_tty(struct tty_struct *tty);
>
> +extern void audit_log_container_drop(void);
> +
> /* audit watch/mark/tree functions */
> #ifdef CONFIG_AUDITSYSCALL
> extern unsigned int audit_serial(void);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f00c1da587ea..f03d3eb0752c 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1575,6 +1575,8 @@ static void audit_log_exit(void)
>
> audit_log_proctitle();
>
> + audit_log_container_drop();
> +
> /* Send end of event record to help user space know we are finished */
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
> if (ab)
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V9 11/13] audit: contid check descendancy and nesting
On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote:
>
> Require the target task to be a descendant of the container
> orchestrator/engine.
>
> You would only change the audit container ID from one set or inherited
> value to another if you were nesting containers.
>
> If changing the contid, the container orchestrator/engine must be a
> descendant and not same orchestrator as the one that set it so it is not
> possible to change the contid of another orchestrator's container.
>
> Since the task_is_descendant() function is used in YAMA and in audit,
> remove the duplication and pull the function into kernel/core/sched.c
>
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/sched.h| 3 +++
> kernel/audit.c | 23 +--
> kernel/sched/core.c | 33 +
> security/yama/yama_lsm.c | 33 -
> 4 files changed, 57 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 2213ac670386..06938d0b9e0c 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2047,4 +2047,7 @@ static inline void rseq_syscall(struct pt_regs *regs)
>
> const struct cpumask *sched_trace_rd_span(struct root_domain *rd);
>
> +extern int task_is_descendant(struct task_struct *parent,
> + struct task_struct *child);
> +
> #endif
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a862721dfd9b..efa65ec01239 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2713,6 +2713,20 @@ int audit_signal_info(int sig, struct task_struct *t)
> return audit_signal_info_syscall(t);
> }
>
> +static bool audit_contid_isnesting(struct task_struct *tsk)
> +{
> + bool isowner = false;
> + bool ownerisparent = false;
> +
> + rcu_read_lock();
> + if (tsk->audit && tsk->audit->cont) {
> + isowner = current == tsk->audit->cont->owner;
> + ownerisparent = task_is_descendant(tsk->audit->cont->owner,
> current);
I want to make sure I'm understanding this correctly and I keep
mentally tripping over something: it seems like for a given audit
container ID a task is either the owner or a descendent, there is no
third state, is that correct?
Assuming that is true, can the descendent check simply be a negative
owner check given they both have the same audit container ID?
> + }
> + rcu_read_unlock();
> + return !isowner && ownerisparent;
> +}
> +
> /*
> * audit_set_contid - set current task's audit contid
> * @task: target task
> @@ -2755,8 +2769,13 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> rc = -EBUSY;
> goto unlock;
> }
> - /* if contid is already set, deny */
> - if (audit_contid_set(task))
> + /* if task is not descendant, block */
> + if (task == current || !task_is_descendant(current, task)) {
I'm also still fuzzy on why we can't let a task set it's own audit
container ID, assuming it meets all the criteria established in patch
2/13. It somewhat made sense when you were tracking inherited vs
explicitly set audit container IDs, but that doesn't appear to be the
case so far in this patchset, yes?
> + rc = -EXDEV;
I'm fairly confident we had a discussion about not using all these
different error codes, but that may be a moot point given my next
comment.
> + goto unlock;
> + }
> + /* only allow contid setting again if nesting */
> + if (audit_contid_set(task) && !audit_contid_isnesting(task))
> rc = -EEXIST;
It seems like what we need in audit_set_contid() is a check to ensure
that the task being modified is only modified by the owner of the
audit container ID, yes? If so, I would think we could do this quite
easily with the following, or similar logic, (NOTE: assumes both
current and tsk are properly setup):
if ((current->audit->cont != tsk->audit->cont) ||
(current->audit->cont->owner != current))
return -EACCESS;
This is somewhat independent of the above issue, but we may also want
to add to the capability check. Patch 2 adds a
"capable(CAP_AUDIT_CONTROL)" which is good, but perhaps we also need a
"ns_capable(CAP_AUDIT_CONTROL)" to allow a given audit container ID
orchestrator/owner the ability to control which of it's descendants
can change their audit container ID, for example:
if (!capable(CAP_AUDIT_CONTROL) ||
!ns_capable(current->nsproxy->user_ns, CAP_AUDIT_CONTROL))
return -EPERM;
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls
gt;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.
> }
>
> - 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))
> + if (context->target_pid) {
> + if (audit_log_pid_context(context, context->target_pid,
> + context->target_auid,
> + context->target_uid,
> + context->target_sessionid,
> + context->target_sid,
> + context->target_comm))
> call_panic = 1;
> + audit_log_container_id(context, context->target_cid);
> + }
>
> if (context->pwd.dentry && context->pwd.mnt) {
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_CWD);
> @@ -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?
> audit_log_container_drop();
>
> /* Send end of event record to help user space know we are finished */
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V9 07/13] audit: add support for non-syscall auxiliary records
On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
>
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone. This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s). The
> context is discarded immediately after the local associated records are
> produced.
We've had some good discussions on the list about why we can't reuse
the "in_syscall" field and need to add a "local" field, I think it
would be good to address that here in the commit description.
> Signed-off-by: Richard Guy Briggs
> Acked-by: Serge Hallyn
> Acked-by: Neil Horman
> Reviewed-by: Ondrej Mosnacek
> ---
> include/linux/audit.h | 8
> kernel/audit.h| 1 +
> kernel/auditsc.c | 33 -
> 3 files changed, 37 insertions(+), 5 deletions(-)
...
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9e79645e5c0e..935eb3d2cde9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -908,11 +908,13 @@ static inline void audit_free_aux(struct audit_context
> *context)
> }
> }
>
> -static inline struct audit_context *audit_alloc_context(enum audit_state
> state)
> +static inline struct audit_context *audit_alloc_context(enum audit_state
> state,
> + gfp_t gfpflags)
> {
> struct audit_context *context;
>
> - context = kzalloc(sizeof(*context), GFP_KERNEL);
> + /* We can be called in atomic context via audit_tg() */
At this point I think it's clear we need a respin so I'm not going to
preface all of my nitpick comments as such, although this definitely
would qualify ...
I don't believe audit_tg() doesn't exist yet, likely coming later in
this patchset, so please remove this comment as it doesn't make sense
in this context.
To be frank, don't re-add the comment later in the patchset either.
Comments like these tend to be fragile and don't really add any great
insight. The audit_tg() function can, and most likely will, be
modified at some point in the future such that the comment above no
longer applies, and there is a reasonable chance that when it does the
above comment will not be updated. Further, anyone modifying the
audit_alloc_context() is going to look at the callers (rather they
*should* look at the callers) and will notice the no-sleep
requirements.
> @@ -960,8 +963,27 @@ int audit_alloc_syscall(struct task_struct *tsk)
> return 0;
> }
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(gfp_t gfpflags)
> {
> + struct audit_context *context = NULL;
> +
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT, gfpflags);
> + if (!context) {
> + audit_log_lost("out of memory in audit_alloc_local");
> + goto out;
You might as well just return NULL here, no need to jump and then return NULL.
> + }
> + context->serial = audit_serial();
> + ktime_get_coarse_real_ts64(>ctime);
> + context->local = true;
> +out:
> + return context;
> +}
> +EXPORT_SYMBOL(audit_alloc_local);
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V9 08/13] audit: add containerid support for user records
On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote: > > Add audit container identifier auxiliary record to user event standalone > records. > > Signed-off-by: Richard Guy Briggs > Acked-by: Neil Horman > Reviewed-by: Ondrej Mosnacek > --- > kernel/audit.c | 19 --- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 54dd2cb69402..997c34178ee8 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1507,6 +1504,14 @@ static int audit_receive_msg(struct sk_buff *skb, > struct nlmsghdr *nlh) > audit_log_n_untrustedstring(ab, str, > data_len); > } > audit_log_end(ab); > + 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(); > + audit_free_context(context); I haven't searched the entire patchset, but it seems like the pattern above happens a couple of times in this patchset, yes? If so would it make sense to wrap the above get/log/put in a helper function? Not a big deal either way, I'm pretty neutral on it at this point in the patchset but thought it might be worth mentioning in case you noticed the same and were on the fence. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V9 13/13] audit: add capcontid to set contid outside init_user_ns
On Sat, Jun 27, 2020 at 9:24 AM Richard Guy Briggs wrote: > > Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a > process in a non-init user namespace the capability to set audit > container identifiers of individual children. > > Provide the /proc/$PID/audit_capcontid interface to capcontid. > Valid values are: 1==enabled, 0==disabled > > Writing a "1" to this special file for the target process $PID will > enable the target process to set audit container identifiers of its > descendants. > > A process must already have CAP_AUDIT_CONTROL in the initial user > namespace or have had audit_capcontid enabled by a previous use of this > feature by its parent on this process in order to be able to enable it > for another process. The target process must be a descendant of the > calling process. > > Report this action in new message type AUDIT_SET_CAPCONTID 1022 with > fields opid= capcontid= old-capcontid= > > Signed-off-by: Richard Guy Briggs > --- > fs/proc/base.c | 57 > +- > include/linux/audit.h | 14 > include/uapi/linux/audit.h | 1 + > kernel/audit.c | 38 ++- > 4 files changed, 108 insertions(+), 2 deletions(-) This seems very similar to the capable/ns_capable combination I mentioned in patch 11/13; any reasons why you feel that this might be a better approach? My current thinking is that the capable/ns_capable approach is preferable as it leverages existing kernel mechanisms and doesn't require us to reinvent the wheel in the audit subsystem. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V9 10/13] audit: add support for containerid to network namespaces
> + contns->count--;
> + if (contns->count < 1) {
One could simplify this with "(--countns->count) < 1", although if it
is changed to a refcount_t (which seems like a smart thing), the
normal decrement/test would be the best choice.
> + list_del_rcu(>list);
> + kfree_rcu(contns, rcu);
> + }
> + break;
> + }
> + spin_unlock(>contobj_list_lock);
> + rcu_read_unlock();
> +}
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V9 12/13] audit: track container nesting
On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote:
>
> Track the parent container of a container to be able to filter and
> report nesting.
>
> Now that we have a way to track and check the parent container of a
> container, modify the contid field format to be able to report that
> nesting using a carrat ("^") modifier to indicate nesting. The
> original field format was "contid=" for task-associated records
> and "contid=[,[...]]" for network-namespace-associated
> records. The new field format is
> "contid=[,^[...]][,[...]]".
I feel like this is a case which could really benefit from an example
in the commit description showing multiple levels of nesting, with
some leaf audit container IDs at each level. This way we have a
canonical example for people who want to understand how to parse the
list and properly sort out the inheritance.
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h | 1 +
> kernel/audit.c| 60
> ++-
> kernel/audit.h| 2 ++
> kernel/auditfilter.c | 17 ++-
> kernel/auditsc.c | 2 +-
> 5 files changed, 70 insertions(+), 12 deletions(-)
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak84 v3] audit: purge audit_log_string from the intra-kernel audit API
, " fsuid=%d",
> diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
> index 4ecedffbdd33..fe431731883f 100644
> --- a/security/apparmor/ipc.c
> +++ b/security/apparmor/ipc.c
> @@ -20,25 +20,23 @@
>
> /**
> * audit_ptrace_mask - convert mask to permission string
> - * @buffer: buffer to write string to (NOT NULL)
> * @mask: permission mask to convert
> + *
> + * Returns: pointer to static string
> */
> -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask)
> +static const char *audit_ptrace_mask(u32 mask)
> {
> switch (mask) {
> case MAY_READ:
> - audit_log_string(ab, "read");
> - break;
> + return "read";
> case MAY_WRITE:
> - audit_log_string(ab, "trace");
> - break;
> + return "trace";
> case AA_MAY_BE_READ:
> - audit_log_string(ab, "readby");
> - break;
> + return "readby";
> case AA_MAY_BE_TRACED:
> - audit_log_string(ab, "tracedby");
> - break;
> + return "tracedby";
> }
> + return "";
> }
>
> /* call back to audit ptrace fields */
> @@ -47,12 +45,12 @@ static void audit_ptrace_cb(struct audit_buffer *ab, void
> *va)
> struct common_audit_data *sa = va;
>
> if (aad(sa)->request & AA_PTRACE_PERM_MASK) {
> - audit_log_format(ab, " requested_mask=");
> - audit_ptrace_mask(ab, aad(sa)->request);
> + audit_log_format(ab, " requested_mask=%s",
> +audit_ptrace_mask(aad(sa)->request));
>
> if (aad(sa)->denied & AA_PTRACE_PERM_MASK) {
> - audit_log_format(ab, " denied_mask=");
> - audit_ptrace_mask(ab, aad(sa)->denied);
> + audit_log_format(ab, " denied_mask=%s",
> +audit_ptrace_mask(aad(sa)->denied));
> }
Quotes. There are none.
... and it looks like there are more missing too, but I kinda stopped
seriously reading the patch here, please take a closer look at the
patch, make the necessary changes, and resubmit.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: use the proper gfp flags in the audit_log_nfcfg() calls
On Fri, Jul 3, 2020 at 4:26 PM Richard Guy Briggs wrote:
>
> On 2020-07-03 09:36, Paul Moore wrote:
> > Commit 142240398e50 ("audit: add gfp parameter to audit_log_nfcfg")
> > incorrectly passed gfp flags to audit_log_nfcfg() which were not
> > consistent with the calling function, this commit fixes that.
> >
> > Fixes: 142240398e50 ("audit: add gfp parameter to audit_log_nfcfg")
> > Reported-by: Jones Desougi
> > Signed-off-by: Paul Moore
>
> Looks good to me. For what it's worth:
Thanks, applied to audit/next. Also, for the record, reviews are
always welcome; I really dislike merging my own patches without
reviews. Sometimes it needs to be done to fix a serious fault, build
error, etc. but in general I'm of the opinion that maintainer patches
should be treated just the same as any other patch.
> Reviewed-by: Richard Guy Briggs
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak96 v3] audit: issue CWD record to accompany LSM_AUDIT_DATA_* records
On Fri, Jul 3, 2020 at 12:56 PM Richard Guy Briggs wrote: > > The LSM_AUDIT_DATA_* records for PATH, FILE, IOCTL_OP, DENTRY and INODE > are incomplete without the task context of the AUDIT Current Working > Directory record. Add it. > > This record addition can't use audit_dummy_context to determine whether > or not to store the record information since the LSM_AUDIT_DATA_* > records are initiated by various LSMs independent of any audit rules. > context->in_syscall is used to determine if it was called in user > context like audit_getname. > > Please see the upstream issue > https://github.com/linux-audit/audit-kernel/issues/96 > > Adapted from Vladis Dronov's v2 patch. > > Signed-off-by: Richard Guy Briggs > --- > Passes audit-testsuite. > > Changelog: > v3 > - adapt and refactor__audit_getname, don't key on dummy > > v2 > 2020-04-02 vdronov > https://www.redhat.com/archives/linux-audit/2020-April/msg4.html > - convert to standalone CWD record > > v1: > 2020-03-24 vdronov > https://github.com/nefigtut/audit-kernel/commit/df0b55b7ab84e1c9faa588b08e547e604bf25c87 > - add cwd= field to LSM record > > include/linux/audit.h | 9 - > kernel/auditsc.c | 17 +++-- > security/lsm_audit.c | 5 + > 3 files changed, 28 insertions(+), 3 deletions(-) Merged into audit/next, thanks. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak84 v3] audit: purge audit_log_string from the intra-kernel audit API
On Wed, Jul 8, 2020 at 7:15 PM Richard Guy Briggs wrote:
> On 2020-07-08 18:41, Paul Moore wrote:
> > On Fri, Jul 3, 2020 at 5:50 PM Richard Guy Briggs wrote:
> > >
> > > audit_log_string() was inteded to be an internal audit function and
> > > since there are only two internal uses, remove them. Purge all external
> > > uses of it by restructuring code to use an existing audit_log_format()
> > > or using audit_log_format().
> > >
> > > Please see the upstream issue
> > > https://github.com/linux-audit/audit-kernel/issues/84
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > Passes audit-testsuite.
> > >
> > > Changelog:
> > > v3
> > > - fix two warning: non-void function does not return a value in all
> > > control paths
> > > Reported-by: kernel test robot
> > >
> > > v2
> > > - restructure to piggyback on existing audit_log_format() calls, checking
> > > quoting needs for each.
> > >
> > > v1 Vlad Dronov
> > > -
> > > https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf
> > >
> > > include/linux/audit.h | 5 -
> > > kernel/audit.c| 4 ++--
> > > security/apparmor/audit.c | 10 --
> > > security/apparmor/file.c | 25 +++--
> > > security/apparmor/ipc.c | 46
> > > +++---
> > > security/apparmor/net.c | 14 --
> > > security/lsm_audit.c | 4 ++--
> > > 7 files changed, 46 insertions(+), 62 deletions(-)
> >
> > ...
> >
> > > diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
> > > index 597732503815..335b5b8d300b 100644
> > > --- a/security/apparmor/audit.c
> > > +++ b/security/apparmor/audit.c
> > > @@ -57,18 +57,16 @@ static void audit_pre(struct audit_buffer *ab, void
> > > *ca)
> > > struct common_audit_data *sa = ca;
> > >
> > > if (aa_g_audit_header) {
> > > - audit_log_format(ab, "apparmor=");
> > > - audit_log_string(ab, aa_audit_type[aad(sa)->type]);
> > > + audit_log_format(ab, "apparmor=%s",
> > > +aa_audit_type[aad(sa)->type]);
> > > }
> > >
> > > if (aad(sa)->op) {
> > > - audit_log_format(ab, " operation=");
> > > - audit_log_string(ab, aad(sa)->op);
> > > + audit_log_format(ab, " operation=%s", aad(sa)->op);
> > > }
> >
> > In the case below you've added the quotes around the string, but they
> > appear to be missing in the two cases above.
>
> They aren't needed above since they are text with no spaces or special
> characters. We don't usually include double quotes where they aren't
> needed. The one below contains spaces so needs double quotes.
>
> > > if (aad(sa)->info) {
> > > - audit_log_format(ab, " info=");
> > > - audit_log_string(ab, aad(sa)->info);
> > > + audit_log_format(ab, " info=\"%s\"", aad(sa)->info);
> > > if (aad(sa)->error)
> > > audit_log_format(ab, " error=%d", aad(sa)->error);
> > > }
> > > diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> > > index 9a2d14b7c9f8..70f27124d051 100644
> > > --- a/security/apparmor/file.c
> > > +++ b/security/apparmor/file.c
> > > @@ -35,20 +35,6 @@ static u32 map_mask_to_chr_mask(u32 mask)
> > > }
> > >
> > > /**
> > > - * audit_file_mask - convert mask to permission string
> > > - * @buffer: buffer to write string to (NOT NULL)
> > > - * @mask: permission mask to convert
> > > - */
> > > -static void audit_file_mask(struct audit_buffer *ab, u32 mask)
> > > -{
> > > - char str[10];
> > > -
> > > - aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs,
> > > - map_mask_to_chr_mask(mask));
> > > - audit_log_string(ab, str);
> > > -}
> > > -
> > > -/**
> > > * file_audit_cb - call back for file specific audit fields
> > > * @ab: audit_buffer (NOT NULL)
> > > * @va: audit stru
Re: [PATCH ghak122 v1] audit: store event sockaddr in case of no rules
On Fri, Jul 3, 2020 at 1:18 PM Richard Guy Briggs wrote:
>
> When there are no rules present, the event SOCKADDR record is not
> generated due to audit_dummy_context() generated at syscall entry from
> audit_n_rules. Store this information if there is a context present to
> store it so that mandatory events are more complete (startup, LSMs...).
>
> Please see the upstream issue
> https://github.com/linux-audit/audit-kernel/issues/122
>
> Signed-off-by: Richard Guy Briggs
> ---
> Passes audit-testsuite.
>
> include/linux/audit.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Do we have any certification requirements driving this change? I ask
because if we make this change, why not do the same for PATH records?
The problem of course is that doing this for both is going to be
costly, the PATH records in particular seem like they would raise a
performance regression.
I agree it would be nice to have this information, but I fear that
gating this on audit_dummy_context() is the right thing to do unless
there is a strong requirement that we always record this information.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 03c4035a532b..07fecd99741a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -448,7 +448,7 @@ static inline int audit_socketcall_compat(int nargs, u32
> *args)
>
> static inline int audit_sockaddr(int len, void *addr)
> {
> - if (unlikely(!audit_dummy_context()))
> + if (audit_context())
> return __audit_sockaddr(len, addr);
> return 0;
> }
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3] audit: report audit wait metric in audit status reply
t; @@ -1193,17 +1198,18 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh)
> case AUDIT_GET: {
> struct audit_status s;
> memset(, 0, sizeof(s));
> - s.enabled = audit_enabled;
> - s.failure = audit_failure;
> + s.enabled = audit_enabled;
> + s.failure = audit_failure;
> /* NOTE: use pid_vnr() so the PID is relative to the current
> * namespace */
> - s.pid = auditd_pid_vnr();
> - s.rate_limit= audit_rate_limit;
> - s.backlog_limit = audit_backlog_limit;
> - s.lost = atomic_read(_lost);
> - s.backlog = skb_queue_len(_queue);
> - s.feature_bitmap= AUDIT_FEATURE_BITMAP_ALL;
> - s.backlog_wait_time = audit_backlog_wait_time;
> + s.pid = auditd_pid_vnr();
> + s.rate_limit = audit_rate_limit;
> + s.backlog_limit= audit_backlog_limit;
> + s.lost = atomic_read(_lost);
> + s.backlog = skb_queue_len(_queue);
> + s.feature_bitmap = AUDIT_FEATURE_BITMAP_ALL;
> + s.backlog_wait_time= audit_backlog_wait_time;
> + s.backlog_wait_time_actual =
> atomic_read(_backlog_wait_time_actual);
> audit_send_reply(skb, seq, AUDIT_GET, 0, 0, , sizeof(s));
> break;
> }
> @@ -1307,6 +1313,12 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh)
> audit_log_config_change("lost", 0, lost, 1);
> return lost;
> }
> + if (s.mask == AUDIT_STATUS_BACKLOG_WAIT_TIME_ACTUAL) {
> + u32 actual =
> atomic_xchg(_backlog_wait_time_actual, 0);
> +
> + audit_log_config_change("backlog_wait_time_actual",
> 0, actual, 1);
> + return actual;
> + }
> break;
> }
> case AUDIT_GET_FEATURE:
> @@ -1778,12 +1790,15 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
> /* sleep if we are allowed and we haven't exhausted
> our
> * backlog wait limit */
> if (gfpflags_allow_blocking(gfp_mask) && (stime > 0))
> {
> + long rtime = stime;
> +
> DECLARE_WAITQUEUE(wait, current);
>
> add_wait_queue_exclusive(_backlog_wait,
> );
> set_current_state(TASK_UNINTERRUPTIBLE);
> - stime = schedule_timeout(stime);
> + stime = schedule_timeout(rtime);
> + atomic_add(rtime - stime,
> _backlog_wait_time_actual);
> remove_wait_queue(_backlog_wait, );
> } else {
> if (audit_rate_check() && printk_ratelimit())
> --
> 2.17.1
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: optionally print warning after waiting to enqueue record
On Tue, Jun 16, 2020 at 12:58 AM Max Englander wrote: > > In environments where security is prioritized, users may set > --backlog_wait_time to a high value in order to reduce the likelihood > that any audit event is lost, even though doing so may result in > unpredictable performance if the kernel schedules a timeout when the > backlog limit is exceeded. For these users, the next best thing to > predictable performance is the ability to quickly detect and react to > degraded performance. This patch proposes to aid the detection of kernel > audit subsystem pauses through the following changes: > > Add a variable named audit_backlog_warn_time. Enforce the value of this > variable to be no less than zero, and no more than the value of > audit_backlog_wait_time. > > If audit_backlog_warn_time is greater than zero and if the total time > spent waiting to enqueue an audit record is greater than or equal to > audit_backlog_warn_time, then print a warning with the total time > spent waiting. > > An example configuration: > > auditctl --backlog_warn_time 50 > > An example warning message: > > audit: sleep_time=52 >= audit_backlog_warn_time=50 > > Tested on Ubuntu 18.04.04 using complementary changes to the audit > userspace: https://github.com/linux-audit/audit-userspace/pull/131. > > Signed-off-by: Max Englander > --- > include/uapi/linux/audit.h | 7 ++- > kernel/audit.c | 35 +++ > 2 files changed, 41 insertions(+), 1 deletion(-) If an admin is prioritizing security, aka don't loose any audit records, and there is a concern over variable system latency due to an audit queue backlog, why not simply disable the backlog limit? -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: optionally print warning after waiting to enqueue record
On Thu, Jun 18, 2020 at 9:39 AM Steve Grubb wrote: > The kernel cannot grow the backlog unbounded. If you do nothing, the backlog > is 64 - which is too small to really use. Otherwise, you set the backlog to a > finite number with the -b option. If one were to set the backlog limit to 0, it is effectively disabled allowing the backlog to grow without any restrictions placed on it by the audit subsystem. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: Use struct_size() helper in alloc_chunk
On Mon, Jun 1, 2020 at 11:36 AM Paul Moore wrote:
> On Sun, May 24, 2020 at 4:47 PM Gustavo A. R. Silva
> wrote:
> > One of the more common cases of allocation size calculations is finding
> > the size of a structure that has a zero-sized array at the end, along
> > with memory for some number of elements for that array. For example:
> >
> > struct audit_chunk {
> > ...
> > struct node {
> > struct list_head list;
> > struct audit_tree *owner;
> > unsigned index; /* index; upper bit indicates 'will
> > prune' */
> > } owners[];
> > };
> >
> > Make use of the struct_size() helper instead of an open-coded version
> > in order to avoid any potential type mistakes.
> >
> > So, replace the following form:
> >
> > offsetof(struct audit_chunk, owners) + count * sizeof(struct node);
> >
> > with:
> >
> > struct_size(chunk, owners, count)
> >
> > This code was detected with the help of Coccinelle.
> >
> > Signed-off-by: Gustavo A. R. Silva
> > ---
> > kernel/audit_tree.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
>
> Thanks, this looks reasonable to me, but it came in too late for the
> v5.8 merge window (I dislike taking changes past -rc5/6 unless
> critical). Once the merge window closes I'll merge this into
> audit/next.
FYI, I just merged this into audit/next. Thanks!
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon
On Mon, Jun 8, 2020 at 2:04 PM Richard Guy Briggs wrote: > On 2020-04-22 13:24, Paul Moore wrote: > > On Fri, Apr 17, 2020 at 6:26 PM Eric W. Biederman > > wrote: > > > Paul Moore writes: > > > > On Thu, Apr 16, 2020 at 4:36 PM Eric W. Biederman > > > > wrote: > > > >> Paul Moore writes: > > > >> > On Mon, Mar 30, 2020 at 1:49 PM Richard Guy Briggs > > > >> > wrote: > > > >> >> On 2020-03-30 13:34, Paul Moore wrote: > > > >> >> > On Mon, Mar 30, 2020 at 12:22 PM Richard Guy Briggs > > > >> >> > wrote: > > > >> >> > > On 2020-03-30 10:26, Paul Moore wrote: > > > >> >> > > > On Mon, Mar 30, 2020 at 9:47 AM Richard Guy Briggs > > > >> >> > > > wrote: > > > >> >> > > > > On 2020-03-28 23:11, Paul Moore wrote: > > > >> >> > > > > > On Tue, Mar 24, 2020 at 5:02 PM Richard Guy Briggs > > > >> >> > > > > > wrote: > > > >> >> > > > > > > On 2020-03-23 20:16, Paul Moore wrote: > > > >> >> > > > > > > > On Thu, Mar 19, 2020 at 6:03 PM Richard Guy Briggs > > > >> >> > > > > > > > wrote: > > > >> >> > > > > > > > > On 2020-03-18 18:06, Paul Moore wrote: > > > >> > > > > >> > ... > > > >> > > > > >> >> > > Well, every time a record gets generated, *any* record gets > > > >> >> > > generated, > > > >> >> > > we'll need to check for which audit daemons this record is in > > > >> >> > > scope and > > > >> >> > > generate a different one for each depending on the content and > > > >> >> > > whether > > > >> >> > > or not the content is influenced by the scope. > > > >> >> > > > > >> >> > That's the problem right there - we don't want to have to > > > >> >> > generate a > > > >> >> > unique record for *each* auditd on *every* record. That is a > > > >> >> > recipe > > > >> >> > for disaster. > > > >> >> > > > > >> >> > Solving this for all of the known audit records is not something > > > >> >> > we > > > >> >> > need to worry about in depth at the moment (although giving it > > > >> >> > some > > > >> >> > casual thought is not a bad thing), but solving this for the audit > > > >> >> > container ID information *is* something we need to worry about > > > >> >> > right > > > >> >> > now. > > > >> >> > > > >> >> If you think that a different nested contid value string per daemon > > > >> >> is > > > >> >> not acceptable, then we are back to issuing a record that has only > > > >> >> *one* > > > >> >> contid listed without any nesting information. This brings us back > > > >> >> to > > > >> >> the original problem of keeping *all* audit log history since the > > > >> >> boot > > > >> >> of the machine to be able to track the nesting of any particular > > > >> >> contid. > > > >> > > > > >> > I'm not ruling anything out, except for the "let's just completely > > > >> > regenerate every record for each auditd instance". > > > >> > > > >> Paul I am a bit confused about what you are referring to when you say > > > >> regenerate every record. > > > >> > > > >> Are you saying that you don't want to repeat the sequence: > > > >> audit_log_start(...); > > > >> audit_log_format(...); > > > >> audit_log_end(...); > > > >> for every nested audit daemon? > > > > > > > > If it can be avoided yes. Audit performance is already not-awesome, > > > > this would make it even worse. > > > > > > As far as I can see not repeating sequences like that is fundamental > > > for making this work at all.
Re: [PATCH 2/2] integrity: Add errno field in audit message
On Wed, Jun 17, 2020 at 4:44 PM Lakshmi Ramasubramanian wrote: > > Error code is not included in the audit messages logged by > the integrity subsystem. Add "errno" field in the audit messages > logged by the integrity subsystem and set the value to the error code > passed to integrity_audit_msg() in the "result" parameter. > > Sample audit messages: > > [6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0 > auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate > cause=alloc_entry comm="swapper/0" name="boot_aggregate" res=0 errno=-12 > > [8.085456] audit: type=1802 audit(1592005947.297:9): pid=1 uid=0 > auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 > op=policy_update cause=completed comm="systemd" res=1 errno=0 > > Signed-off-by: Lakshmi Ramasubramanian > Suggested-by: Steve Grubb > --- > security/integrity/integrity_audit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Paul Moore > diff --git a/security/integrity/integrity_audit.c > b/security/integrity/integrity_audit.c > index 5109173839cc..a265024f82f3 100644 > --- a/security/integrity/integrity_audit.c > +++ b/security/integrity/integrity_audit.c > @@ -53,6 +53,6 @@ void integrity_audit_msg(int audit_msgno, struct inode > *inode, > audit_log_untrustedstring(ab, inode->i_sb->s_id); > audit_log_format(ab, " ino=%lu", inode->i_ino); > } > - audit_log_format(ab, " res=%d", !result); > + audit_log_format(ab, " res=%d errno=%d", !result, result); > audit_log_end(ab); > } > -- > 2.27.0 -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: optionally print warning after waiting to enqueue record
On Thu, Jun 18, 2020 at 10:36 AM Steve Grubb wrote: > On Thursday, June 18, 2020 9:46:54 AM EDT Paul Moore wrote: > > On Thu, Jun 18, 2020 at 9:39 AM Steve Grubb wrote: > > > The kernel cannot grow the backlog unbounded. If you do nothing, the > > > backlog is 64 - which is too small to really use. Otherwise, you set the > > > backlog to a finite number with the -b option. > > > > If one were to set the backlog limit to 0, it is effectively disabled > > allowing the backlog to grow without any restrictions placed on it by > > the audit subsystem. > > Then I'd say you asked for it. The cure is setting a number. I wasn't commenting on if it was wise or not, that is going to depend on the goals of the admin. I just wanted to correct some bad information you provided so those reading the mailing list were not ill-informed. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 1/2] integrity: Add errno field in audit message
On Thu, Jun 18, 2020 at 5:10 PM Lakshmi Ramasubramanian
wrote:
>
> Error code is not included in the audit messages logged by
> the integrity subsystem.
>
> Define a new function integrity_audit_message() that takes error code
> in the "errno" parameter. Add "errno" field in the audit messages logged
> by the integrity subsystem and set the value passed in the "errno"
> parameter.
>
> [6.303048] audit: type=1804 audit(1592506281.627:2): pid=1 uid=0
> auid=4294967295 ses=4294967295 subj=kernel op=measuring_key cause=ENOMEM
> comm="swapper/0" name=".builtin_trusted_keys" res=0 errno=-12
>
> [7.987647] audit: type=1802 audit(1592506283.312:9): pid=1 uid=0
> auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0
> op=policy_update cause=completed comm="systemd" res=1 errno=0
>
> [8.019432] audit: type=1804 audit(1592506283.344:10): pid=1 uid=0
> auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0
> op=measuring_kexec_cmdline cause=hashing_error comm="systemd"
> name="kexec-cmdline" res=0 errno=-22
>
> Signed-off-by: Lakshmi Ramasubramanian
> Suggested-by: Steve Grubb
> Suggested-by: Mimi Zohar
> ---
> security/integrity/integrity.h | 13 +
> security/integrity/integrity_audit.c | 11 ++-
> 2 files changed, 23 insertions(+), 1 deletion(-)
The audit record changes look good to me.
Acked-by: Paul Moore
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 16c1894c29bb..413c803c5208 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -239,6 +239,11 @@ void integrity_audit_msg(int audit_msgno, struct inode
> *inode,
> const unsigned char *fname, const char *op,
> const char *cause, int result, int info);
>
> +void integrity_audit_message(int audit_msgno, struct inode *inode,
> +const unsigned char *fname, const char *op,
> +const char *cause, int result, int info,
> +int errno);
> +
> static inline struct audit_buffer *
> integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int
> type)
> {
> @@ -253,6 +258,14 @@ static inline void integrity_audit_msg(int audit_msgno,
> struct inode *inode,
> {
> }
>
> +static inline void integrity_audit_message(int audit_msgno,
> + struct inode *inode,
> + const unsigned char *fname,
> + const char *op, const char *cause,
> + int result, int info, int errno)
> +{
> +}
> +
> static inline struct audit_buffer *
> integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int
> type)
> {
> diff --git a/security/integrity/integrity_audit.c
> b/security/integrity/integrity_audit.c
> index 5109173839cc..f25e7df099c8 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -28,6 +28,15 @@ __setup("integrity_audit=", integrity_audit_setup);
> void integrity_audit_msg(int audit_msgno, struct inode *inode,
> const unsigned char *fname, const char *op,
> const char *cause, int result, int audit_info)
> +{
> + integrity_audit_message(audit_msgno, inode, fname, op, cause,
> + result, audit_info, 0);
> +}
> +
> +void integrity_audit_message(int audit_msgno, struct inode *inode,
> +const unsigned char *fname, const char *op,
> +const char *cause, int result, int audit_info,
> +int errno)
> {
> struct audit_buffer *ab;
> char name[TASK_COMM_LEN];
> @@ -53,6 +62,6 @@ void integrity_audit_msg(int audit_msgno, struct inode
> *inode,
> audit_log_untrustedstring(ab, inode->i_sb->s_id);
> audit_log_format(ab, " ino=%lu", inode->i_ino);
> }
> - audit_log_format(ab, " res=%d", !result);
> + audit_log_format(ab, " res=%d errno=%d", !result, errno);
> audit_log_end(ab);
> }
> --
> 2.27.0
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 2/2] IMA: Add audit log for failure conditions
On Wed, Jun 24, 2020 at 1:25 PM Lakshmi Ramasubramanian wrote: > > On 6/23/20 12:58 PM, Mimi Zohar wrote: > > Hi Steve\Paul, > > >> Sample audit messages: > >> > >> [6.303048] audit: type=1804 audit(1592506281.627:2): pid=1 uid=0 > >> auid=4294967295 ses=4294967295 subj=kernel op=measuring_key > >> cause=ENOMEM comm="swapper/0" name=".builtin_trusted_keys" res=0 > >> errno=-12 > > > > My only concern is that auditing -ENOMEM will put additional memory > > pressure on the system. I'm not sure if this is a concern and, if so, > > how it should be handled. > > Do you have any concerns with respect to adding audit messages in low > memory conditions? Assuming the system is not completely toast, the allocation failure could be a very transient issue; I wouldn't worry too much about it. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 1/2] integrity: Add errno field in audit message
On Mon, Jun 15, 2020 at 6:23 PM Steve Grubb wrote: > On Friday, June 12, 2020 3:50:14 PM EDT Lakshmi Ramasubramanian wrote: > > On 6/12/20 12:25 PM, Mimi Zohar wrote: > > > The idea is a good idea, but you're assuming that "result" is always > > > errno. That was probably true originally, but isn't now. For > > > example, ima_appraise_measurement() calls xattr_verify(), which > > > compares the security.ima hash with the calculated file hash. On > > > failure, it returns the result of memcmp(). Each and every code path > > > will need to be checked. > > > > Good catch Mimi. > > > > Instead of "errno" should we just use "result" and log the value given > > in the result parameter? > > That would likely collide with another field of the same name which is the > operation's results. If it really is errno, the name is fine. It's generic > enough that it can be reused on other events if that mattered. Steve, what is the historical reason why we have both "res" and "result" for indicating a boolean success/fail? I'm just curious how we ended up this way, and who may still be using "result". -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 1/2] integrity: Add result field in audit message
On Fri, Jun 12, 2020 at 10:26 PM Lakshmi Ramasubramanian wrote: > Result code is not included in the audit messages logged by > the integrity subsystem. Add "result" field in the audit messages > logged by the integrity subsystem and set the value to the result code > passed to integrity_audit_msg() in the "result" parameter. > > Sample audit message: > > [6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0 > auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate > cause=alloc_entry comm="swapper/0" name="boot_aggregate" res=0 result=-12 > > [8.085456] audit: type=1802 audit(1592005947.297:9): pid=1 uid=0 > auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 > op=policy_update cause=completed comm="systemd" res=1 result=0 > > Signed-off-by: Lakshmi Ramasubramanian > Suggested-by: Steve Grubb > --- > security/integrity/integrity_audit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) If we can't use "res=" to carry more than 0/1 then this seems reasonable. Acked-by: Paul Moore > diff --git a/security/integrity/integrity_audit.c > b/security/integrity/integrity_audit.c > index 5109173839cc..84002d3d5a95 100644 > --- a/security/integrity/integrity_audit.c > +++ b/security/integrity/integrity_audit.c > @@ -53,6 +53,6 @@ void integrity_audit_msg(int audit_msgno, struct inode > *inode, > audit_log_untrustedstring(ab, inode->i_sb->s_id); > audit_log_format(ab, " ino=%lu", inode->i_ino); > } > - audit_log_format(ab, " res=%d", !result); > + audit_log_format(ab, " res=%d result=%d", !result, result); > audit_log_end(ab); > } > -- > 2.27.0 -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: optionally print warning after waiting to enqueue record
On Wed, Jun 17, 2020 at 6:54 PM Max Englander wrote: > On Wed, Jun 17, 2020 at 02:47:19PM -0400, Paul Moore wrote: > > On Tue, Jun 16, 2020 at 12:58 AM Max Englander > > wrote: > > > > > > In environments where security is prioritized, users may set > > > --backlog_wait_time to a high value in order to reduce the likelihood > > > that any audit event is lost, even though doing so may result in > > > unpredictable performance if the kernel schedules a timeout when the > > > backlog limit is exceeded. For these users, the next best thing to > > > predictable performance is the ability to quickly detect and react to > > > degraded performance. This patch proposes to aid the detection of kernel > > > audit subsystem pauses through the following changes: > > > > > > Add a variable named audit_backlog_warn_time. Enforce the value of this > > > variable to be no less than zero, and no more than the value of > > > audit_backlog_wait_time. > > > > > > If audit_backlog_warn_time is greater than zero and if the total time > > > spent waiting to enqueue an audit record is greater than or equal to > > > audit_backlog_warn_time, then print a warning with the total time > > > spent waiting. > > > > > > An example configuration: > > > > > > auditctl --backlog_warn_time 50 > > > > > > An example warning message: > > > > > > audit: sleep_time=52 >= audit_backlog_warn_time=50 > > > > > > Tested on Ubuntu 18.04.04 using complementary changes to the audit > > > userspace: https://github.com/linux-audit/audit-userspace/pull/131. > > > > > > Signed-off-by: Max Englander > > > --- > > > include/uapi/linux/audit.h | 7 ++- > > > kernel/audit.c | 35 +++ > > > 2 files changed, 41 insertions(+), 1 deletion(-) > > > > If an admin is prioritizing security, aka don't loose any audit > > records, and there is a concern over variable system latency due to an > > audit queue backlog, why not simply disable the backlog limit? > > > > -- > > paul moore > > www.paul-moore.com > > That’s good in some cases, but in other cases unbounded growth of the > backlog could result in memory issues. If the kernel runs out of memory > it would drop the audit event or possibly have other problems. It could > also also consume memory in a way that starves user workloads or causes > them to be killed by the OOMKiller. > > To refine my motivating use case a bit, if a Kubernetes admin wants to > prioritize security, and also avoid unbounded growth of the audit > backlog, they may set -b and --backlog_wait_time in a way that limits > kernel memory usage and reduces the likelihood that any audit event is > lost. Occasional performance degradation may be acceptable to the admin, > but they would like a way to be alerted to prolonged kernel pauses, so > that they can investigate and take corrective action (increase backlog, > increase server capacity, move some workloads to other servers, etc.). > > To state another way. The kernel currently can be configured to print a > message when the backlog limit is exceeded and it must discard the audit > event. This is a useful message for admins, which they can address with > corrective action. I think a message similar to the one proposed by this > patch would be equally useful when the backlog limit is exceeded and the > kernel is configured to wait for the backlog to drain. Admins could > address that message in the same way, but without the cost of lost audit > events. I'm still struggling to understand how this is any better than disabling the backlog limit, or setting it very high, and simply monitoring the audit size of the audit backlog. This way the admin doesn't have to worry about the latency issues of a full backlog, while still being able to trigger actions based on the state of the backlog. The userspace tooling/scripting to watch the backlog size would be trivial, and would arguably provide much better visibility into the backlog state than a single warning threshold in the kernel. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: optionally print warning after waiting to enqueue record
On Thu, Jun 18, 2020 at 8:30 PM Richard Guy Briggs wrote: > On 2020-06-18 23:48, Max Englander wrote: > > In case you’re any more receptive to the idea, I thought I’d mention > > that the need this patch addresses would be just as well fulfilled if > > wait times were reported in the audit status response along with other > > currently reported metrics like backlog length and lost events. Wait > > times could be reported as a cumulative sum, a moving average, or in > > some other way, and would help directly implicate or rule out backlog > > waiting as the cause in the event that an admin is faced with debugging > > degraded kernel performance. It would eliminate the need for a new flag, > > and fit well with the userspace tooling approach you suggested above. > > Such as is captured in this upstream issue from 3 years ago: > > https://github.com/linux-audit/audit-kernel/issues/63 > "RFE: add kernel audit queue statistics" I would be more open to the idea of reporting queue statistics as part of the audit status information, or similar. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak124 v3] audit: log nftables configuration change events
On Thu, Jun 4, 2020 at 9:21 AM Richard Guy Briggs wrote: > > iptables, ip6tables, arptables and ebtables table registration, > replacement and unregistration configuration events are logged for the > native (legacy) iptables setsockopt api, but not for the > nftables netlink api which is used by the nft-variant of iptables in > addition to nftables itself. > > Add calls to log the configuration actions in the nftables netlink api. > > This uses the same NETFILTER_CFG record format but overloads the table > field. > > type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=?:0;?:0 > family=unspecified entries=2 op=nft_register_gen pid=396 > subj=system_u:system_r:firewalld_t:s0 comm=firewalld > ... > type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : > table=firewalld:1;?:0 family=inet entries=0 op=nft_register_table pid=396 > subj=system_u:system_r:firewalld_t:s0 comm=firewalld > ... > type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : > table=firewalld:1;filter_FORWARD:85 family=inet entries=8 > op=nft_register_chain pid=396 subj=system_u:system_r:firewalld_t:s0 > comm=firewalld > ... > type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : > table=firewalld:1;filter_FORWARD:85 family=inet entries=101 > op=nft_register_rule pid=396 subj=system_u:system_r:firewalld_t:s0 > comm=firewalld > ... > type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : > table=firewalld:1;__set0:87 family=inet entries=87 op=nft_register_setelem > pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld > ... > type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : > table=firewalld:1;__set0:87 family=inet entries=0 op=nft_register_set pid=396 > subj=system_u:system_r:firewalld_t:s0 comm=firewalld > > For further information please see issue > https://github.com/linux-audit/audit-kernel/issues/124 > > Signed-off-by: Richard Guy Briggs > --- > Changelog: > v3: > - inline message type rather than table > > v2: > - differentiate between xtables and nftables > - add set, setelem, obj, flowtable, gen > - use nentries field as appropriate per type > - overload the "tables" field with table handle and chain/set/flowtable > > include/linux/audit.h | 18 > kernel/auditsc.c | 24 -- > net/netfilter/nf_tables_api.c | 103 > ++ > 3 files changed, 142 insertions(+), 3 deletions(-) I'm not seeing any additional comments from the netfilter folks so I've gone ahead and merged this into audit/next. Thanks Richard. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] IMA: Add log statements for failure conditions
On Fri, Jun 5, 2020 at 3:54 PM Lakshmi Ramasubramanian wrote: > On 6/5/20 12:37 PM, Paul Moore wrote: > > > If it's audit related, it's generally best to CC the linux-audit list, > > not just me (fixed). > > > > It's not clear to me what this pr_err() is trying to indicate other > > than *something* failed. Can someone provide some more background on > > this message? > > process_buffer_measurement() is currently used to measure > "kexec command line", "keys", and "blacklist-hash". If there was any > error in the measurement, this pr_err() will indicate which of the above > measurement failed and the related error code. > > Please let me know if you need more info on this one. That helps, thank you. > Since a pr_xyz() call was already present, I just wanted to change the > log level to keep the code change to the minimum. But if audit log is > the right approach for this case, I'll update. Generally we reserve audit for things that are required for various security certifications and/or "security relevant". From what you mentioned above, it seems like this would fall into the second category if not the first. Looking at your patch it doesn't look like you are trying to record anything special so you may be able to use the existing integrity_audit_msg(...) helper. Of course then the question comes down to the audit record type (the audit_msgno argument), the operation (op), and the comm/cause (cause). Do you feel that any of the existing audit record types are a good fit for this? -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] IMA: Add log statements for failure conditions
On Fri, Jun 5, 2020 at 2:46 PM Mimi Zohar wrote:
>
> [Cc'ing Paul Moore]
If it's audit related, it's generally best to CC the linux-audit list,
not just me (fixed).
It's not clear to me what this pr_err() is trying to indicate other
than *something* failed. Can someone provide some more background on
this message?
> Hi Lakshmi,
>
> On Thu, 2020-06-04 at 09:32 -0700, Lakshmi Ramasubramanian wrote:
> > The final log statement in process_buffer_measurement() for failure
> > condition is at debug level. This does not log the message unless
> > the system log level is raised which would significantly increase
> > the messages in the system log. Change this log message to error level,
> > and add eventname and ima_hooks enum to the message for better triaging
> > failures in the function.
> >
> > ima_alloc_key_entry() does not log a message for failure condition.
> > Add an error message for failure condition in this function.
> >
> > Signed-off-by: Lakshmi Ramasubramanian
>
> These messages should probably be turned into audit messages. Look at
> integerity_audit_msg().
>
> Mimi
>
> > ---
> > security/integrity/ima/ima_main.c | 3 ++-
> > security/integrity/ima/ima_queue_keys.c | 2 ++
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 9d0abedeae77..3b371f31597b 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -756,7 +756,8 @@ void process_buffer_measurement(const void *buf, int
> > size,
> >
> > out:
> > if (ret < 0)
> > - pr_devel("%s: failed, result: %d\n", __func__, ret);
> > + pr_err("%s failed. eventname: %s, func: %d, result: %d\n",
> > +__func__, eventname, func, ret);
> >
> > return;
> > }
> > diff --git a/security/integrity/ima/ima_queue_keys.c
> > b/security/integrity/ima/ima_queue_keys.c
> > index cb3e3f501593..e51d0eb08d8a 100644
> > --- a/security/integrity/ima/ima_queue_keys.c
> > +++ b/security/integrity/ima/ima_queue_keys.c
> > @@ -88,6 +88,8 @@ static struct ima_key_entry *ima_alloc_key_entry(struct
> > key *keyring,
> >
> > out:
> > if (rc) {
> > + pr_err("%s failed. keyring: %s, result: %d\n",
> > +__func__, keyring->description, rc);
> > ima_free_key_entry(entry);
> > entry = NULL;
> > }
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] IMA: Add audit log for failure conditions
On Mon, Jun 8, 2020 at 7:52 AM Mimi Zohar wrote: > Hi Lakshmi, > > On Sun, 2020-06-07 at 15:14 -0700, Lakshmi Ramasubramanian wrote: > > The final log statement in process_buffer_measurement() for failure > > condition is at debug level. This does not log the message unless > > the system log level is raised which would significantly increase > > the messages in the system log. Change this log message to an audit > > message for better triaging failures in the function. > > > > ima_alloc_key_entry() does not log a message for failure condition. > > Add an audit message for failure condition in this function. > > > > Signed-off-by: Lakshmi Ramasubramanian > > Audit messages should be at a higher level. For example, > "hashing_error", "collect_data", "invalid_pcr". In the "invalid_pcr" > case, the audit log contains the reason - "ToMToU" or "open_writers" - > as to why the measurement list doesn't match the PCR. > > Here you would want "measuring_keys", "measuring_boot_cmdline" with > the reason it failed, not the function name > "process_buffer_measurement". > > Userspace needs to be aware of the new audit messages. Maybe include > samples of them in the cover letter. Yes, examples of the audit record in the commit description (the cover letter isn't recorded in the git log), are encouraged. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 1/2] integrity: Add errno field in audit message
On Wed, Jun 10, 2020 at 9:58 PM Lakshmi Ramasubramanian
wrote:
> On 6/10/20 6:45 PM, Paul Moore wrote:
>
> Hi Paul,
>
> > I'm sorry I didn't get a chance to mention this before you posted this
> > patch, but for the past several years we have been sticking with a
> > policy of only adding new fields to the end of existing records;
> > please adjust this patch accordingly. Otherwise, this looks fine to
> > me.
> >
> >> audit_log_untrustedstring(ab, get_task_comm(name, current));
> >> if (fname) {
> >> audit_log_format(ab, " name=");
> >> --
>
> Steve mentioned that since this new field "errno" is not a searchable
> entry, it can be added anywhere in the audit log message.
Steve and I have a different opinion on this issue. I won't rehash
the long argument or drag you into it, but I will just say that the
*kernel* has had a policy of only adding fields to the end of existing
records unless under extreme cases (this is not an extreme case).
> But I have no problem moving this to the end of the audit record.
Great, please do that. Thank you.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 1/2] integrity: Add errno field in audit message
On Wed, Jun 10, 2020 at 8:04 PM Lakshmi Ramasubramanian
wrote:
>
> Error code is not included in the audit messages logged by
> the integrity subsystem. Add a new field namely "errno" in
> the audit message and set the value to the error code passed
> to integrity_audit_msg() in the "result" parameter.
>
> Sample audit message:
>
> [6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0
> auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate
> cause=alloc_entry errno=-12 comm="swapper/0" name="boot_aggregate" res=0
>
> Signed-off-by: Lakshmi Ramasubramanian
> Suggested-by: Steve Grubb
> ---
> security/integrity/integrity_audit.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/integrity_audit.c
> b/security/integrity/integrity_audit.c
> index 5109173839cc..8cbf415bb977 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -42,7 +42,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
> *inode,
> from_kuid(_user_ns,
> audit_get_loginuid(current)),
> audit_get_sessionid(current));
> audit_log_task_context(ab);
> - audit_log_format(ab, " op=%s cause=%s comm=", op, cause);
> + audit_log_format(ab, " op=%s cause=%s errno=%d comm=",
> +op, cause, result);
Hi Lakshmi,
I'm sorry I didn't get a chance to mention this before you posted this
patch, but for the past several years we have been sticking with a
policy of only adding new fields to the end of existing records;
please adjust this patch accordingly. Otherwise, this looks fine to
me.
> audit_log_untrustedstring(ab, get_task_comm(name, current));
> if (fname) {
> audit_log_format(ab, " name=");
> --
> 2.27.0
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: null pointer dereference regression in 5.7
On Tue, Jul 21, 2020 at 10:01 PM Richard Guy Briggs wrote:
> On 2020-07-21 18:45, Paul Moore wrote:
> > On Tue, Jul 21, 2020 at 6:30 PM Paul Moore wrote:
> > > Richard, you broke it, you bought it :) Did you want to take a closer
> > > look at this? If you can't let me know. Based on a quick look, my
> > > gut feeling is that either context->pwd is never set properly or it is
> > > getting free'd prematurely; I'm highly suspicious of the latter but
> > > the former seems like it might be a reasonable place to start.
> >
> > Actually, yes, I'm pretty certain the problem is that context->pwd is
> > never set in this case.
>
> Does the ghak96 upstream patch in audit/next on 5.8-rc1 fix it?
> d7481b24b816 ("audit: issue CWD record to accompany LSM_AUDIT_DATA_*
> records")
>
> The avc is generated by common_lsm_audit() which calls
> dump_common_audit_data() that now calls audit_getcwd() on the 5
> LSM_AUDIT_DATA_* types that deal with paths.
I would expect that it would resolve the problem being reported, which
is good, but I'm not sure it is a general solution to the problem. I
suspect there is bigger problem of context->pwd not always having a
"safe" value when the task exits or the syscall returns to userspace.
> > Normally context->pwd would be set by a call to
> > audit_getname()/__audit_getname(), but if there audit context is a
> > dummy context, that is skipped and context->pwd is never set.
> > Normally that is fine, expect with Richard's patch if the kernel
> > explicitly calls audit_log_start() we mark the context as ... not a
> > dummy? smart? I'm not sure of the right term here ... which then
> > triggers all the usual logging one would expect. In this particular
> > case, a SELinux AVC, the audit_log_start() happens *after* the
> > pathname has been resolved and the audit_getname() calls are made;
> > thus in this case context->pwd is not valid when the normal audit
> > logging takes place on exit and things explode in predictable fashion.
>
> The first two AVCs that were accompanied by syscalls had "items=0" but
> the one that blew up had "items=2" so it appears the paths were already
> present in the context, but missing the pwd.
Yes, the issue is with context->pwd, although I suppose other fields
could also be suspect.
> > Unfortunately, it is beginning to look like 1320a4052ea1 ("audit:
> > trigger accompanying records when no rules present") may be more
> > dangerous than initially thought. I'm borderline tempted to just
> > revert this patch, but I'll leave this open for discussion ...
> > Richard, I think you need to go through the code and audit all of the
> > functions that store data in an audit context that are skipped when
> > there is a dummy context to see which fields are potentially unset,
> > and then look at all the end of task/syscall code to make sure the
> > necessary set/unset checks are in place.
>
> Auditing all the callers is not a small task, but I agree it may be
> necessary.
Do you have a rough idea as to how long it would take to chase down
all the code paths? I'm asking not to rush you, but to figure out if
we should revert the patch now to resolve the problem and restore it
later once we are confident there are no additional issues lurking.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: null pointer dereference regression in 5.7
On Thu, Jul 23, 2020 at 8:57 AM Richard Guy Briggs wrote: > On 2020-07-22 21:01, Paul Moore wrote: > > Do you have a rough idea as to how long it would take to chase down > > all the code paths? I'm asking not to rush you, but to figure out if > > we should revert the patch now to resolve the problem and restore it > > later once we are confident there are no additional issues lurking. > > I figure 2-3 days. Okay. I think we need to submit a revert for v5.8 and -stable (which is pretty limited at this point); can you put that together and send it to the list? It should be trivial, if you can't do it let me know. > I'm trying to remember the name of the tool to build a function calling > tree, either up or down. Was it cscope? Or is there something more > modern? It will have some limitations due to op function pointers. I'm not sure what you're talking about, I always just walk the code by hand in my editor with cscope or lxr as tools on the side. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH] revert: 1320a4052ea1 ("audit: trigger accompanying records when no rules present")
Unfortunately the commit listed in the subject line above failed
to ensure that the task's audit_context was properly initialized/set
before enabling the "accompanying records". Depending on the
sitation, the resulting audit_context could have invalid values in
some of it's fields which could cause a kernel panic/oops when the
task/syscall exists and the audit records are generated.
We will revisit the original patch, with the necessary fixes, in a
future kernel but right now we just want to fix the kernel panic
with the least amount of added risk.
Cc: sta...@vger.kernel.org
Fixes: 1320a4052ea1 ("audit: trigger accompanying records when no rules
present")
Reported-by: j24...@googlemail.com
Signed-off-by: Paul Moore
---
kernel/audit.c |1 -
kernel/audit.h |8
kernel/auditsc.c |3 +++
3 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index e33460e01b3b..9bf2b08b051f 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1848,7 +1848,6 @@ struct audit_buffer *audit_log_start(struct audit_context
*ctx, gfp_t gfp_mask,
}
audit_get_stamp(ab->ctx, , );
- audit_clear_dummy(ab->ctx);
audit_log_format(ab, "audit(%llu.%03lu:%u): ",
(unsigned long long)t.tv_sec, t.tv_nsec/100,
serial);
diff --git a/kernel/audit.h b/kernel/audit.h
index f0233dc40b17..ddc22878433d 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -290,13 +290,6 @@ extern int audit_signal_info_syscall(struct task_struct
*t);
extern void audit_filter_inodes(struct task_struct *tsk,
struct audit_context *ctx);
extern struct list_head *audit_killed_trees(void);
-
-static inline void audit_clear_dummy(struct audit_context *ctx)
-{
- if (ctx)
- ctx->dummy = 0;
-}
-
#else /* CONFIG_AUDITSYSCALL */
#define auditsc_get_stamp(c, t, s) 0
#define audit_put_watch(w) {}
@@ -330,7 +323,6 @@ static inline int audit_signal_info_syscall(struct
task_struct *t)
}
#define audit_filter_inodes(t, c) AUDIT_DISABLED
-#define audit_clear_dummy(c) {}
#endif /* CONFIG_AUDITSYSCALL */
extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 468a23390457..fd840c40abf7 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1417,6 +1417,9 @@ static void audit_log_proctitle(void)
struct audit_context *context = audit_context();
struct audit_buffer *ab;
+ if (!context || context->dummy)
+ return;
+
ab = audit_log_start(context, GFP_KERNEL, AUDIT_PROCTITLE);
if (!ab)
return; /* audit_panic or being filtered */
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH V3fix ghak120] audit: initialize context values in case of mandatory events
On Tue, Jul 28, 2020 at 12:27 PM Richard Guy Briggs wrote: > On 2020-07-27 22:14, Paul Moore wrote: > > On Mon, Jul 27, 2020 at 5:30 PM Richard Guy Briggs wrote: > > > Issue ghak120 enabled syscall records to accompany required records when > > > no rules are present to trigger the storage of syscall context. A > > > reported issue showed that the cwd was not always initialized. That > > > issue was already resolved ... > > > > Yes and no. Yes, it appears to be resolved in v5.8-rc1 and above, but > > the problematic commit is in v5.7 and I'm not sure backporting the fix > > in v5.8-rcX plus this patch is the right thing to do for a released > > kernel. The lowest risk fix for v5.7 at this point is to do a revert; > > Ok, fair enough. I don't understand why you didn't do the revert since > it appears so trivial to you and this review and fix turned out to be > marginally more work. I didn't understand what you wanted when you > referred to stable. I held off on the revert because I thought you might want the chance to submit the revert with your authorship. I made an assumption that it meant the same to you as it does to me; that's my mistake, I should have known better. I'll do the revert myself for stable-5.8 (which should trickle down to v5.7.z with the right metadata), don't bother with it. > > regardless of what happens with this patch and v5.8-rcX please post a > > revert for the audit/stable-5.7 tree as soon as you can. > > (more below...) > > > > ... but a review of all other records that could > > > be triggered at the time of a syscall record revealed other potential > > > values that could be missing or misleading. Initialize them. > > > > > > The fds array is reset to -1 after the first syscall to indicate it > > > isn't valid any more, but was never set to -1 when the context was > > > allocated to indicate it wasn't yet valid. > > > > > > The audit_inode* functions can be called without going through > > > getname_flags() or getname_kernel() that sets audit_names and cwd, so > > > set the cwd if it has not already been done so due to audit_names being > > > valid. > > > > > > The LSM dump_common_audit_data() LSM_AUDIT_DATA_NET:AF_UNIX case was > > > missed with the ghak96 patch, so add that case here. > > > > > > Please see issue https://github.com/linux-audit/audit-kernel/issues/120 > > > Please see issue https://github.com/linux-audit/audit-kernel/issues/96 > > > Passes audit-testsuite. > > > > > > Signed-off-by: Richard Guy Briggs > > > --- > > > kernel/auditsc.c | 3 +++ > > > security/lsm_audit.c | 1 + > > > 2 files changed, 4 insertions(+) > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > index 6884b50069d1..2f97618e6a34 100644 > > > --- a/kernel/auditsc.c > > > +++ b/kernel/auditsc.c > > > @@ -929,6 +929,7 @@ static inline struct audit_context > > > *audit_alloc_context(enum audit_state state) > > > context->prio = state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0; > > > INIT_LIST_HEAD(>killed_trees); > > > INIT_LIST_HEAD(>names_list); > > > + context->fds[0] = -1; > > > return context; > > > } > > > > > > @@ -2076,6 +2077,7 @@ void __audit_inode(struct filename *name, const > > > struct dentry *dentry, > > > } > > > handle_path(dentry); > > > audit_copy_inode(n, dentry, inode, flags & AUDIT_INODE_NOEVAL); > > > + _audit_getcwd(context); > > > } > > > > > > void __audit_file(const struct file *file) > > > @@ -2194,6 +2196,7 @@ void __audit_inode_child(struct inode *parent, > > > audit_copy_inode(found_child, dentry, inode, 0); > > > else > > > found_child->ino = AUDIT_INO_UNSET; > > > + _audit_getcwd(context); > > > } > > > EXPORT_SYMBOL_GPL(__audit_inode_child); > > > > > > diff --git a/security/lsm_audit.c b/security/lsm_audit.c > > > index 53d0d183db8f..e93077612246 100644 > > > --- a/security/lsm_audit.c > > > +++ b/security/lsm_audit.c > > > @@ -369,6 +369,7 @@ static void dump_common_audit_data(struct > > > audit_buffer *ab, > > > audit_log_untrustedstring(ab, p); > > > else > > > audit_log_n_hex(ab, p, len); > > &
Re: [PATCH V3fix ghak120] audit: initialize context values in case of mandatory events
On Tue, Jul 28, 2020 at 10:01 PM Richard Guy Briggs wrote: > > On 2020-07-28 14:47, Paul Moore wrote: > > On Tue, Jul 28, 2020 at 12:27 PM Richard Guy Briggs wrote: > > > I know you like only really minimal fixes this late, but this seemed > > > pretty minimal to me... > > > > Minimal is a one (two?) line NULL check in audit_log_name(), this > > patch is not that. > > I didn't try and test that since I'm not sure that would have worked > because there appeared to be a low non-NULL value in it. brauer1's trace had > 0x60 and mine had 0xd0. Or am I missing something obvious? Well, you mentioned the obvious already: both 0x60 and 0xd0 are not NULL. We already have a NULL check for context->pwd elsewhere so there is precedence for this solving a similar problem, although without going through the git log I'm not sure what problem that solved, or if it was precautionary. I agree the low value looks suspicious, it almost looks like an offset to me, ideally it would be good to understand how/why that value is "off'. It could be that the audit_context is not being properly initialized, reset, or something unrelated is clobbering the value; all things that would be nice to know. > The patch provided the information rather than ignoring the problem ... I disagree. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] revert: 1320a4052ea1 ("audit: trigger accompanying records when no rules present")
On Tue, Jul 28, 2020 at 5:09 PM Paul Moore wrote:
>
> Unfortunately the commit listed in the subject line above failed
> to ensure that the task's audit_context was properly initialized/set
> before enabling the "accompanying records". Depending on the
> sitation, the resulting audit_context could have invalid values in
> some of it's fields which could cause a kernel panic/oops when the
> task/syscall exists and the audit records are generated.
>
> We will revisit the original patch, with the necessary fixes, in a
> future kernel but right now we just want to fix the kernel panic
> with the least amount of added risk.
>
> Cc: sta...@vger.kernel.org
> Fixes: 1320a4052ea1 ("audit: trigger accompanying records when no rules
> present")
> Reported-by: j24...@googlemail.com
> Signed-off-by: Paul Moore
> ---
> kernel/audit.c |1 -
> kernel/audit.h |8
> kernel/auditsc.c |3 +++
> 3 files changed, 3 insertions(+), 9 deletions(-)
William pointed out a misspelling in the patch description above,
which I just fixed. Unfortunately I had already pushed the patch to
audit/stable-5.8 so I did a force-push to correct the spelling;
normally I wouldn't do something like that for such a trivial matter,
but since it is unlikely anyone is based of the audit/stable-5.8
branch this seemed like an okay time to do that.
I'll be sending a PR to Linus shortly.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
[GIT PULL] Audit fixes for v5.8 (#1)
Hi Linus,
One small audit fix that you can hopefully merge before v5.8 is
released. Unfortunately it is a revert of a patch that went in during
the v5.7 window and we just recently started to see some bug reports
relating to that commit. We are working on a proper fix, but I'm not
yet clear on when that will be ready and we need to fix the v5.7
kernels anyway, so in the interest of time a revert seemed like the
best solution right now.
The patch passes our test suite, and as of right now it merges cleanly
against your tree. You may notice a force-push on the
audit/stable-5.8 branch, but that was to fix a spelling mistake in the
commit that was identified after the patch had been committed.
Generally I try to avoid force-pushes, but since no one really uses
the audit/stable-X.Y branches as a base for development it seemed
safe.
Thanks,
-Paul
--
The following changes since commit 9d44a121c5a79bc8a9d67c058456bd52a83c79e7:
audit: add subj creds to NETFILTER_CFG record to
(2020-05-20 18:09:19 -0400)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git
tags/audit-pr-20200729
for you to fetch changes up to 8ac68dc455d9d18241d44b96800d73229029ed34:
revert: 1320a4052ea1 ("audit: trigger accompanying records when no rules
present") (2020-07-29 10:00:36 -0400)
audit/stable-5.8 PR 20200729
----
Paul Moore (1):
revert: 1320a4052ea1 ("audit: trigger accompanying records when no
rules present")
kernel/audit.c | 1 -
kernel/audit.h | 8
kernel/auditsc.c | 3 +++
3 files changed, 3 insertions(+), 9 deletions(-)
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH V3fix ghak120] audit: initialize context values in case of mandatory events
On Mon, Jul 27, 2020 at 5:30 PM Richard Guy Briggs wrote: > > Issue ghak120 enabled syscall records to accompany required records when > no rules are present to trigger the storage of syscall context. A > reported issue showed that the cwd was not always initialized. That > issue was already resolved ... Yes and no. Yes, it appears to be resolved in v5.8-rc1 and above, but the problematic commit is in v5.7 and I'm not sure backporting the fix in v5.8-rcX plus this patch is the right thing to do for a released kernel. The lowest risk fix for v5.7 at this point is to do a revert; regardless of what happens with this patch and v5.8-rcX please post a revert for the audit/stable-5.7 tree as soon as you can. > ... but a review of all other records that could > be triggered at the time of a syscall record revealed other potential > values that could be missing or misleading. Initialize them. > > The fds array is reset to -1 after the first syscall to indicate it > isn't valid any more, but was never set to -1 when the context was > allocated to indicate it wasn't yet valid. > > The audit_inode* functions can be called without going through > getname_flags() or getname_kernel() that sets audit_names and cwd, so > set the cwd if it has not already been done so due to audit_names being > valid. > > The LSM dump_common_audit_data() LSM_AUDIT_DATA_NET:AF_UNIX case was > missed with the ghak96 patch, so add that case here. > > Please see issue https://github.com/linux-audit/audit-kernel/issues/120 > Please see issue https://github.com/linux-audit/audit-kernel/issues/96 > Passes audit-testsuite. > > Signed-off-by: Richard Guy Briggs > --- > kernel/auditsc.c | 3 +++ > security/lsm_audit.c | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 6884b50069d1..2f97618e6a34 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -929,6 +929,7 @@ static inline struct audit_context > *audit_alloc_context(enum audit_state state) > context->prio = state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0; > INIT_LIST_HEAD(>killed_trees); > INIT_LIST_HEAD(>names_list); > + context->fds[0] = -1; > return context; > } > > @@ -2076,6 +2077,7 @@ void __audit_inode(struct filename *name, const struct > dentry *dentry, > } > handle_path(dentry); > audit_copy_inode(n, dentry, inode, flags & AUDIT_INODE_NOEVAL); > + _audit_getcwd(context); > } > > void __audit_file(const struct file *file) > @@ -2194,6 +2196,7 @@ void __audit_inode_child(struct inode *parent, > audit_copy_inode(found_child, dentry, inode, 0); > else > found_child->ino = AUDIT_INO_UNSET; > + _audit_getcwd(context); > } > EXPORT_SYMBOL_GPL(__audit_inode_child); > > diff --git a/security/lsm_audit.c b/security/lsm_audit.c > index 53d0d183db8f..e93077612246 100644 > --- a/security/lsm_audit.c > +++ b/security/lsm_audit.c > @@ -369,6 +369,7 @@ static void dump_common_audit_data(struct audit_buffer > *ab, > audit_log_untrustedstring(ab, p); > else > audit_log_n_hex(ab, p, len); > + audit_getcwd(); > break; > } > } I understand the "fds[0] = -1" fix in audit_alloc_context() (ironically, the kzalloc() which is supposed to help with cases like this, hurts us with this particular field), but I'm still not quite seeing why we need to sprinkle audit_getcwd() calls everywhere to fix this bug (this seems more like a feature add than a bigfix). Yes, they may fix the problem but it seems like simply adding a context->pwd test in audit_log_name() similar to what we do in audit_log_exit() is the correct fix. We are currently at -rc7 and this really needs to land before v5.8 is released, presumably this weekend; this means a small and limited bug fix patch is what is needed. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak84 v4] audit: purge audit_log_string from the intra-kernel audit API
On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs wrote:
> On 2020-07-14 16:29, Paul Moore wrote:
> > On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs wrote:
> > > On 2020-07-14 12:21, Paul Moore wrote:
> > > > On Mon, Jul 13, 2020 at 3:52 PM Richard Guy Briggs
> > > > wrote:
> > > > >
> > > > > audit_log_string() was inteded to be an internal audit function and
> > > > > since there are only two internal uses, remove them. Purge all
> > > > > external
> > > > > uses of it by restructuring code to use an existing audit_log_format()
> > > > > or using audit_log_format().
> > > > >
> > > > > Please see the upstream issue
> > > > > https://github.com/linux-audit/audit-kernel/issues/84
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs
> > > > > ---
> > > > > Passes audit-testsuite.
> > > > >
> > > > > Changelog:
> > > > > v4
> > > > > - use double quotes in all replaced audit_log_string() calls
> > > > >
> > > > > v3
> > > > > - fix two warning: non-void function does not return a value in all
> > > > > control paths
> > > > > Reported-by: kernel test robot
> > > > >
> > > > > v2
> > > > > - restructure to piggyback on existing audit_log_format() calls,
> > > > > checking quoting needs for each.
> > > > >
> > > > > v1 Vlad Dronov
> > > > > -
> > > > > https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf
> > > > >
> > > > > include/linux/audit.h | 5 -
> > > > > kernel/audit.c| 4 ++--
> > > > > security/apparmor/audit.c | 10 --
> > > > > security/apparmor/file.c | 25 +++--
> > > > > security/apparmor/ipc.c | 46
> > > > > +++---
> > > > > security/apparmor/net.c | 14 --
> > > > > security/lsm_audit.c | 4 ++--
> > > > > 7 files changed, 46 insertions(+), 62 deletions(-)
> > > >
> > > > Thanks for restoring the quotes, just one question below ...
> > > >
> > > > > diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
> > > > > index 4ecedffbdd33..fe36d112aad9 100644
> > > > > --- a/security/apparmor/ipc.c
> > > > > +++ b/security/apparmor/ipc.c
> > > > > @@ -20,25 +20,23 @@
> > > > >
> > > > > /**
> > > > > * audit_ptrace_mask - convert mask to permission string
> > > > > - * @buffer: buffer to write string to (NOT NULL)
> > > > > * @mask: permission mask to convert
> > > > > + *
> > > > > + * Returns: pointer to static string
> > > > > */
> > > > > -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask)
> > > > > +static const char *audit_ptrace_mask(u32 mask)
> > > > > {
> > > > > switch (mask) {
> > > > > case MAY_READ:
> > > > > - audit_log_string(ab, "read");
> > > > > - break;
> > > > > + return "read";
> > > > > case MAY_WRITE:
> > > > > - audit_log_string(ab, "trace");
> > > > > - break;
> > > > > + return "trace";
> > > > > case AA_MAY_BE_READ:
> > > > > - audit_log_string(ab, "readby");
> > > > > - break;
> > > > > + return "readby";
> > > > > case AA_MAY_BE_TRACED:
> > > > > - audit_log_string(ab, "tracedby");
> > > > > - break;
> > > > > + return "tracedby";
> > > > > }
> > > > > + return "";
> > > >
> > > > Are we okay with this returning an empty string ("") in this case?
> > > > Should it be a question mark ("?")?
> > > >
> > > > My guess is that userspace parsing should be okay since it still has
> > > > quotes, I'm just not sure if we wanted to use a question mark as we do
> > > > in other cases where the field value is empty/unknown.
> > >
> > > Previously, it would have been an empty value, not even double quotes.
> > > "?" might be an improvement.
> >
> > Did you want to fix that now in this patch, or leave it to later? As
> > I said above, I'm not too bothered by it with the quotes so either way
> > is fine by me.
>
> I'd defer to Steve, otherwise I'd say leave it, since there wasn't
> anything there before and this makes that more evident.
>
> > John, I'm assuming you are okay with this patch?
With no comments from John or Steve in the past week, I've gone ahead
and merged the patch into audit/next.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3] audit: report audit wait metric in audit status reply
On Wed, Jul 15, 2020 at 9:30 PM Paul Moore wrote: > On Wed, Jul 8, 2020 at 7:13 PM Paul Moore wrote: > > On Sat, Jul 4, 2020 at 11:15 AM Max Englander > > wrote: > > > > > > In environments where the preservation of audit events and predictable > > > usage of system memory are prioritized, admins may use a combination of > > > --backlog_wait_time and -b options at the risk of degraded performance > > > resulting from backlog waiting. In some cases, this risk may be > > > preferred to lost events or unbounded memory usage. Ideally, this risk > > > can be mitigated by making adjustments when backlog waiting is detected. > > > > > > However, detection can be difficult using the currently available > > > metrics. For example, an admin attempting to debug degraded performance > > > may falsely believe a full backlog indicates backlog waiting. It may > > > turn out the backlog frequently fills up but drains quickly. > > > > > > To make it easier to reliably track degraded performance to backlog > > > waiting, this patch makes the following changes: > > > > > > Add a new field backlog_wait_time_total to the audit status reply. > > > Initialize this field to zero. Add to this field the total time spent > > > by the current task on scheduled timeouts while the backlog limit is > > > exceeded. Reset field to zero upon request via AUDIT_SET. > > > > > > Tested on Ubuntu 18.04 using complementary changes to the > > > audit-userspace and audit-testsuite: > > > - https://github.com/linux-audit/audit-userspace/pull/134 > > > - https://github.com/linux-audit/audit-testsuite/pull/97 > > > > > > Signed-off-by: Max Englander > > > --- > > > Patch changelogs between v1 and v2: > > > - Instead of printing a warning when backlog waiting occurs, add > > > duration of backlog waiting to cumulative sum, and report this > > > sum in audit status reply. > > > > > > Patch changelogs between v2 and v3: > > > - Rename backlog_wait_sum to backlog_wait_time_actual. > > > - Drop unneeded and unwanted header flags > > >AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM and > > >AUDIT_VERSION_BACKLOG_WAIT_SUM. > > > - Increment backlog_wait_time_actual counter after every call to > > >schedule_timeout rather than once after enqueuing (or losing) an > > >audit record. > > > - Add support for resetting backlog_wait_time_actual counter to zero > > >upon request via AUDIT_SET. > > > > > > include/uapi/linux/audit.h | 18 +++--- > > > kernel/audit.c | 35 +-- > > > 2 files changed, 36 insertions(+), 17 deletions(-) > > > > This looks okay to me, thanks for the fixes Max. > > > > Steve, does the associated userspace patch look okay to you? > > Steve, any comments on the userspace patch? Did I miss a reply in my > inbox perhaps? > > If I don't see any feedback by the end of the week I'll plan on > merging this into audit/next. It's been over two weeks with no comment, so I went ahead and merged this into audit/next. Thanks for your patience Max! -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH] audit: use the proper gfp flags in the audit_log_nfcfg() calls
Commit 142240398e50 ("audit: add gfp parameter to audit_log_nfcfg")
incorrectly passed gfp flags to audit_log_nfcfg() which were not
consistent with the calling function, this commit fixes that.
Fixes: 142240398e50 ("audit: add gfp parameter to audit_log_nfcfg")
Reported-by: Jones Desougi
Signed-off-by: Paul Moore
---
net/netfilter/nf_tables_api.c |8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f7ff91479647..886e64291f41 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5953,7 +5953,7 @@ static int nf_tables_dump_obj(struct sk_buff *skb, struct
netlink_callback *cb)
goto cont;
if (reset) {
- char *buf = kasprintf(GFP_KERNEL,
+ char *buf = kasprintf(GFP_ATOMIC,
"%s:%llu;?:0",
table->name,
table->handle);
@@ -5962,7 +5962,7 @@ static int nf_tables_dump_obj(struct sk_buff *skb, struct
netlink_callback *cb)
family,
obj->handle,
AUDIT_NFT_OP_OBJ_RESET,
- GFP_KERNEL);
+ GFP_ATOMIC);
kfree(buf);
}
@@ -6084,7 +6084,7 @@ static int nf_tables_getobj(struct net *net, struct sock
*nlsk,
family,
obj->handle,
AUDIT_NFT_OP_OBJ_RESET,
- GFP_KERNEL);
+ GFP_ATOMIC);
kfree(buf);
}
@@ -6172,7 +6172,7 @@ void nft_obj_notify(struct net *net, const struct
nft_table *table,
event == NFT_MSG_NEWOBJ ?
AUDIT_NFT_OP_OBJ_REGISTER :
AUDIT_NFT_OP_OBJ_UNREGISTER,
- GFP_KERNEL);
+ gfp);
kfree(buf);
if (!report &&
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak124 v3fix] audit: add gfp parameter to audit_log_nfcfg
On Fri, Jul 3, 2020 at 8:41 AM Jones Desougi wrote: > > Doesn't seem entirely consistent now either though. Two cases below. Yes, you're right, that patch was incorrect; thanks for catching that. I just posted a fix (lore link below) that fixes the two problems you pointed out as well as converts a call in a RCU protected section to an ATOMIC. https://lore.kernel.org/linux-audit/159378341669.5956.13490174029711421419.stgit@sifl -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [audit] c4dad0aab3: canonical_address#:#[##]
On Fri, Aug 14, 2020 at 7:09 PM Richard Guy Briggs wrote:
> On 2020-08-09 14:36, kernel test robot wrote:
> > Greeting,
> >
> > FYI, we noticed the following commit (built with clang-12):
> >
> > commit: c4dad0aab3fca0c1f0baa4cc84b6ec91b7ebf426 ("audit: tidy and extend
> > netfilter_cfg x_tables")
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> >
> >
> > in testcase: trinity
> > with following parameters:
> >
> > runtime: 300s
> >
> > test-description: Trinity is a linux system call fuzz tester.
> > test-url: http://codemonkey.org.uk/projects/trinity/
> >
> >
> > on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m
> > 16G
> >
> > caused below changes (please refer to attached dmesg/kmsg for entire
> > log/backtrace):
>
> This looks odd to me. I don't see any audit involved in this and I've
> heard other complaints that this bot has appeared to mis-attribute other
> errors. I had a quick look before I went on vacation a week ago and was
> back today briefly. I'll be away until the 24th and won't be able to
> look before then.
I am just getting back to normal network access myself, but I did have
a brief exchange with Richard about this and I agree it looks a bit
odd.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 1/3] dm: introduce audit event module for device mapper
gt;error); > + audit_log_format(ab, " res=%d", result); > + audit_log_end(ab); > +} Generally speaking we try to keep a consistent format and ordering within a given audit record type. However you appear to have at least three different formats for the AUDIT_DM record in this patch: "... module=%s dev=%d:%d op=%s sector=%llu res=%d" "... module=%s dev=%s op=%s error_msg='%s' res=%d" "... module=%s dev=%s op=%s res=%d" The first thing that jumps out is that some fields, e.g. "sector", are not always present in the record; we typically handle this by using a "?" for the field value in those cases where you would otherwise drop the field from the record, for example the following record: "... module=%s dev=%s op=%s res=%d" ... would be rewritten like this: "... module=%s dev=%s op=%s sector=? res=%d" The second thing that I noticed is that the "dev" field changes from a "major:minor" number representation to an arbitrary string value, e.g. "dev=%s". This generally isn't something we do with audit records, please stick to a single representation for a given audit record-type/field combination. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On Wed, Aug 18, 2021 at 5:59 PM Casey Schaufler wrote:
>
> On 8/16/2021 11:57 AM, Paul Moore wrote:
> > On Fri, Aug 13, 2021 at 5:47 PM Casey Schaufler
> > wrote:
> >> On 8/13/2021 1:43 PM, Paul Moore wrote:
> ...
> > Yeah, the thought occurred to me, but we are clearly already in the
> > maybe-the-assumptions-are-wrong stage so I'm not going to rely on that
> > being 100%. We definitely need to track this down before we start
> > making to many more guesses about what is working and what is not.
>
> I've been tracking down where the audit context isn't set where
> we'd expect it to be, I've identified 5 cases:
>
> 1000AUDIT_GET - Get Status
> 1001AUDIT_SET - Set status enable/disable/auditd
> 1010AUDIT_SIGNAL_INFO
> 1130AUDIT_SERVICE_START
> 1131AUDIT_SEVICE_STOP
>
> These are all events that relate to the audit system itself.
> It seems plausible that these really aren't syscalls and hence
> shouldn't be expected to have an audit_context. I will create a
> patch that treats these as the special cases I believe them to be.
Yes, all but two of these could be considered to be audit subsystem
control messages, but AUDIT_SERVICE_{START,STOP} I think definitely
fall outside the audit subsystem control message category. The
AUDIT_SERVICE_{START,STOP} records are used to indicate when a
service, e.g. NetworkManager, starts and stops; on my fedora test
system they are generated by systemd since it manages service state on
that system; a quick example is below, but I'm sure you've seen plenty
of these already.
% ausearch -m SERVICE_START
time->Wed Aug 18 20:13:00 2021
type=SERVICE_START msg=audit(1629331980.779:1186): pid=1 uid=0 auid=4294967295 s
es=4294967295 subj=system_u:system_r:init_t:s0 msg='unit=NetworkManager-dispatch
er comm="systemd" exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=? re
s=success'
However, regardless of if the message is related to controlling the
audit subsystem or not, we do want to be able to associate those
records with other related records, e.g. SYSCALL records. Looking at
the message types you listed above, they are all records that are
triggered by userspace via netlink messages; if you haven't already I
would start poking along that code path to see if something looks
awry.
I just spent a few minutes tracing the code paths up from audit
through netlink and then through the socket layer and I'm not seeing
anything obvious where the path differs from any other syscall;
current->audit_context *should* be valid just like any other syscall.
However, I do have to ask, are you only seeing these audit records
with a current->audit_context equal to NULL during early boot?
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler wrote: > On 8/12/2021 1:59 PM, Paul Moore wrote: > > On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler > > wrote: > >> Create a new audit record type to contain the subject information > >> when there are multiple security modules that require such data. ... > > The local > > audit context is a hack that is made necessary by the fact that we > > have to audit things which happen outside the scope of an executing > > task, e.g. the netfilter audit hooks, it should *never* be used when > > there is a valid task_struct. > > In the existing audit code a "current context" is only needed for > syscall events, so that's the only case where it's allocated. Would > you suggest that I track down the non-syscall events that include > subj= fields and add allocate a "current context" for them? I looked > into doing that, and it wouldn't be simple. This is why the "local context" was created. Prior to these stacking additions, and the audit container ID work, we never needed to group multiple audit records outside of a syscall context into a single audit event so passing a NULL context into audit_log_start() was reasonable. The local context was designed as a way to generate a context for use in a local function scope to group multiple records, however, for reasons I'll get to below I'm now wondering if the local context approach is really workable ... > > Hopefully that makes sense? > > Yes, it makes sense. Methinks you may believe that the current context > is available more regularly than it actually is. > > I instrumented the audit event functions with: > > WARN_ONCE(audit_context, "%s has context\n", __func__); > WARN_ONCE(!audit_context, "%s lacks context\n", __func__); > > I only used local contexts where the 2nd WARN_ONCE was hit. What does your audit config look like? Both the kernel command line and the output of 'auditctl -l' would be helpful. I'm beginning to suspect that you have the default we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime audit configuration that is de rigueur for many distros these days. If that is the case, there are many cases where you would not see a NULL current->audit_context simply because the config never allocated one, see kernel/auditsc.c:audit_alloc(). If that is the case, I'm honestly a little surprised we didn't realize that earlier, especially given all the work/testing that Richard has done with the audit container ID bits, but then again he surely had a proper audit config during his testing so it wouldn't have appeared. Good times. Regardless, assuming that is the case we probably need to find an alternative to the local context approach as it currently works. For reasons we already talked about, we don't want to use a local audit_context if there is the possibility for a proper current->audit_context, but we need to do *something* so that we can group these multiple events into a single record. Since this is just occurring to me now I need a bit more time to think on possible solutions - all good ideas are welcome - but the first thing that pops into my head is that we need to augment audit_log_end() to potentially generated additional, associated records similar to what we do on syscall exit in audit_log_exit(). Of course the audit_log_end() changes would be much more limited than audit_log_exit(), just the LSM subject and audit container ID info, and even then we might want to limit that to cases where the ab->ctx value is NULL and let audit_log_exit() handle it otherwise. We may need to store the event type in the audit_buffer during audit_log_start() so that we can later use that in audit_log_end() to determine what additional records are needed. Regardless, let's figure out why all your current->audit_context values are NULL first (report back on your audit config please), I may be worrying about a hypothetical that isn't real. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On Tue, Aug 24, 2021 at 11:20 AM Casey Schaufler wrote: > On 8/24/2021 7:45 AM, Paul Moore wrote: > > On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler > > 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. > > As I said, it's a hack-around that demonstrates the scope of the > problem. Had you expressed enthusiastic approval for it I'd have > been very surprised. That's okay, you can admit you were trying to catch me not paying attention ;) > > 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: > > I will give this a shot. Thanks. I'm sure I'm probably missing some detail, but if you get stuck let me know and I'll try to lend a hand. > > [*] 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. > > Hmm. That effort has been quiet lately. Too quiet. The current delay is intentional and is related to the io_uring work. When Richard and I first became aware of the io_uring issue Richard was in the process of readying his latest revision to the audit container ID patchset and some of the changes he was incorporating, in my opinion, hinted at the io_uring issue, or at least drew more attention to that than I was comfortable seeing posted publicly. Richard discussed this with his management and security response team, and they felt differently. I told Richard that I didn't want to block him posting an update to the patchset, but that I felt it would be The Wrong Thing To Do and if he did post the patchset I would likely ignore it until after the io_uring fix had been posted so as to not draw additional attention to his changes. I can't speak for Richard's mindset, but he appeared anxious to post his changes regardless of my concerns, and he did so shortly afterwards. That's why you haven't seen much progress on this for a while, and why you will see me comment on the latest patchset after the io_uring patches land in -next after the next merge window closes. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 RESEND] powerpc/audit: Convert powerpc to AUDIT_ARCH_COMPAT_GENERIC
On Tue, Aug 24, 2021 at 9:36 AM Christophe Leroy
wrote:
>
> Commit e65e1fc2d24b ("[PATCH] syscall class hookup for all normal
> targets") added generic support for AUDIT but that didn't include
> support for bi-arch like powerpc.
>
> Commit 4b58841149dc ("audit: Add generic compat syscall support")
> added generic support for bi-arch.
>
> Convert powerpc to that bi-arch generic audit support.
>
> Cc: Paul Moore
> Cc: Eric Paris
> Signed-off-by: Christophe Leroy
> ---
> Resending v2 with Audit people in Cc
>
> v2:
> - Missing 'git add' for arch/powerpc/include/asm/unistd32.h
> - Finalised commit description
> ---
> arch/powerpc/Kconfig| 5 +-
> arch/powerpc/include/asm/unistd32.h | 7 +++
> arch/powerpc/kernel/Makefile| 3 --
> arch/powerpc/kernel/audit.c | 84 -
> arch/powerpc/kernel/compat_audit.c | 44 ---
> 5 files changed, 8 insertions(+), 135 deletions(-)
> create mode 100644 arch/powerpc/include/asm/unistd32.h
> delete mode 100644 arch/powerpc/kernel/audit.c
> delete mode 100644 arch/powerpc/kernel/compat_audit.c
Can you explain, in detail please, the testing you have done to verify
this patch?
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler 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.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Thu, Aug 26, 2021 at 12:32 PM Richard Guy Briggs wrote: > I'm getting: > # ./iouring.2 > Kernel thread io_uring-sq is not running. > Unable to setup io_uring: Permission denied > > # ./iouring.3s > >>> server started, pid = 2082 > >>> memfd created, fd = 3 > io_uring_queue_init: Permission denied > > I have CONFIG_IO_URING=y set, what else is needed? I'm not sure how you tried to run those tests, but try running as root and with SELinux in permissive mode. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 RESEND] powerpc/audit: Convert powerpc to AUDIT_ARCH_COMPAT_GENERIC
On Thu, Aug 26, 2021 at 10:37 AM Michael Ellerman wrote:
> Paul Moore writes:
> > On Tue, Aug 24, 2021 at 1:11 PM Christophe Leroy
> > wrote:
> >> Le 24/08/2021 à 16:47, Paul Moore a écrit :
> >> > On Tue, Aug 24, 2021 at 9:36 AM Christophe Leroy
> >> > wrote:
> >> >>
> >> >> Commit e65e1fc2d24b ("[PATCH] syscall class hookup for all normal
> >> >> targets") added generic support for AUDIT but that didn't include
> >> >> support for bi-arch like powerpc.
> >> >>
> >> >> Commit 4b58841149dc ("audit: Add generic compat syscall support")
> >> >> added generic support for bi-arch.
> >> >>
> >> >> Convert powerpc to that bi-arch generic audit support.
> >> >>
> >> >> Cc: Paul Moore
> >> >> Cc: Eric Paris
> >> >> Signed-off-by: Christophe Leroy
> >> >> ---
> >> >> Resending v2 with Audit people in Cc
> >> >>
> >> >> v2:
> >> >> - Missing 'git add' for arch/powerpc/include/asm/unistd32.h
> >> >> - Finalised commit description
> >> >> ---
> >> >> arch/powerpc/Kconfig| 5 +-
> >> >> arch/powerpc/include/asm/unistd32.h | 7 +++
> >> >> arch/powerpc/kernel/Makefile| 3 --
> >> >> arch/powerpc/kernel/audit.c | 84 -
> >> >> arch/powerpc/kernel/compat_audit.c | 44 ---
> >> >> 5 files changed, 8 insertions(+), 135 deletions(-)
> >> >> create mode 100644 arch/powerpc/include/asm/unistd32.h
> >> >> delete mode 100644 arch/powerpc/kernel/audit.c
> >> >> delete mode 100644 arch/powerpc/kernel/compat_audit.c
> >> >
> >> > Can you explain, in detail please, the testing you have done to verify
> >> > this patch?
> >> >
> >>
> >> I built ppc64_defconfig and checked that the generated code is
> >> functionnaly equivalent.
> >>
> >> ppc32_classify_syscall() is exactly the same as
> >> audit_classify_compat_syscall() except that the
> >> later takes the syscall as second argument (ie in r4) whereas the former
> >> takes it as first argument
> >> (ie in r3).
> >>
> >> audit_classify_arch() and powerpc audit_classify_syscall() are slightly
> >> different between the
> >> powerpc version and the generic version because the powerpc version checks
> >> whether it is
> >> AUDIT_ARCH_PPC or not (ie value 20), while the generic one checks whether
> >> it has bit
> >> __AUDIT_ARCH_64BIT set or not (__AUDIT_ARCH_64BIT is the sign bit of a
> >> word), but taking into
> >> account that the abi is either AUDIT_ARCH_PPC, AUDIT_ARCH_PPC64 or
> >> AUDIT_ARCH_PPC64LE, the result is
> >> the same.
> >>
> >> If you are asking I guess you saw something wrong ?
> >
> > I was asking because I didn't see any mention of testing, and when you
> > are enabling something significant like this it is nice to see that it
> > has been verified to work :)
> >
> > While binary dumps and comparisons are nice, it is always good to see
> > verification from a test suite. I don't have access to the necessary
> > hardware to test this, but could you verify that the audit-testsuite
> > passes on your test system with your patches applied?
> >
> > * https://github.com/linux-audit/audit-testsuite
>
> I tested on ppc64le. Both before and after the patch I get the result
> below.
>
> So I guess the patch is OK, but maybe we have some existing issue.
>
> I had a bit of a look at the test code, but my perl is limited. I think
> it was running the command below, and it returned "", but
> not really sure what that means.
If it makes you feel any better, my perl is *very* limited; thankfully
this isn't my first time looking at that test :)
It's a little odd, but after some basic sanity tests at the top, the
test sets a watch on a file, /tmp/, and tells the kernel
to generate audit records for any syscall that operates on that file.
It then creates, and removes, a series of exclude audit filters to
check if the exclude filtering is working as expected, e.g. when
syscall filtering is excluded there should be no syscall records in
the audit log.
In the case you describe, it looks like it looks like the audit
exclude filter is removed (that's what line 147 does), the
/tmp/ file is removed (line 152), and then we check to
se
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Fri, Aug 27, 2021 at 9:36 AM Richard Guy Briggs wrote: > On 2021-08-26 15:14, Paul Moore wrote: > > On Thu, Aug 26, 2021 at 12:32 PM Richard Guy Briggs wrote: > > > I'm getting: > > > # ./iouring.2 > > > Kernel thread io_uring-sq is not running. > > > Unable to setup io_uring: Permission denied > > > > > > # ./iouring.3s > > > >>> server started, pid = 2082 > > > >>> memfd created, fd = 3 > > > io_uring_queue_init: Permission denied > > > > > > I have CONFIG_IO_URING=y set, what else is needed? > > > > I'm not sure how you tried to run those tests, but try running as root > > and with SELinux in permissive mode. > > Ok, they ran, including iouring.4. iouring.2 claimed twice: "Kernel > thread io_uring-sq is not running." and I didn't get any URING records > with ausearch. I don't know if any of this is expected. Now that I've written iouring.4, I would skip the others; while helpful at the time, they are pretty crap. I have no idea what kernel you are running, but I'm going to assume you've applied the v2 patches (if not, you obviously need to do that ). Beyond that you may need to set a filter for the io_uring_enter() syscall to force the issue; theoretically your audit userspace patches should allow a uring op specifically to be filtered but I haven't had a chance to try that yet so either the kernel or userspace portion could be broken. At this point if you are running into problems you'll probably need to spend some time debugging them, as I think you're the only person who has tested your audit userspace patches at this point (and the only one who has access to your latest bits). -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 9/9] Smack: Brutalist io_uring support with debug
On Wed, Aug 11, 2021 at 4:49 PM Paul Moore wrote:
>
> From: Casey Schaufler
>
> Add Smack privilege checks for io_uring. Use CAP_MAC_OVERRIDE
> for the override_creds case and CAP_MAC_ADMIN for creating a
> polling thread. These choices are based on conjecture regarding
> the intent of the surrounding code.
>
> Signed-off-by: Casey Schaufler
> [PM: make the smack_uring_* funcs static]
> Signed-off-by: Paul Moore
>
> ---
> v2:
> - made the smack_uring_* funcs static
> v1:
> - initial draft
> ---
> security/smack/smack_lsm.c | 64
>
> 1 file changed, 64 insertions(+)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 223a6da0e6dc..7fb094098f38 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4691,6 +4691,66 @@ static int smack_dentry_create_files_as(struct dentry
> *dentry, int mode,
> return 0;
> }
>
> +#ifdef CONFIG_IO_URING
> +/**
> + * smack_uring_override_creds - Is io_uring cred override allowed?
> + * @new: the target creds
> + *
> + * Check to see if the current task is allowed to override it's credentials
> + * to service an io_uring operation.
> + */
> +static int smack_uring_override_creds(const struct cred *new)
> +{
> + struct task_smack *tsp = smack_cred(current_cred());
> + struct task_smack *nsp = smack_cred(new);
> +
> +#if 1
> + if (tsp->smk_task == nsp->smk_task)
> + pr_info("%s: Smack matches %s\n", __func__,
> + tsp->smk_task->smk_known);
> + else
> + pr_info("%s: Smack override check %s to %s\n", __func__,
> + tsp->smk_task->smk_known, nsp->smk_task->smk_known);
> +#endif
Casey, with the idea of posting a v3 towards the end of the merge
window next week, without the RFC tag and with the intention of
merging it into -next during the first/second week of the -rcX phase,
do you have any objections to me removing the debug code (#if 1 ...
#endif) from your patch? Did you have any other changes?
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 9/9] Smack: Brutalist io_uring support with debug
On Tue, Aug 31, 2021 at 11:03 AM Casey Schaufler wrote: > On 8/31/2021 7:44 AM, Paul Moore wrote: > > > > Casey, with the idea of posting a v3 towards the end of the merge > > window next week, without the RFC tag and with the intention of > > merging it into -next during the first/second week of the -rcX phase, > > do you have any objections to me removing the debug code (#if 1 ... > > #endif) from your patch? Did you have any other changes? > > I have no other changes. And yes, the debug code should be stripped. > Thank you. Great, I'll remove that code for the v3 dump. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Wed, Aug 25, 2021 at 9:16 PM Richard Guy Briggs wrote: > > On 2021-08-24 16:57, Richard Guy Briggs wrote: > > On 2021-08-11 16:48, Paul Moore wrote: > > > Draft #2 of the patchset which brings auditing and proper LSM access > > > controls to the io_uring subsystem. The original patchset was posted > > > in late May and can be found via lore using the link below: > > > > > > https://lore.kernel.org/linux-security-module/162163367115.8379.8459012634106035341.stgit@sifl/ > > > > > > This draft should incorporate all of the feedback from the original > > > posting as well as a few smaller things I noticed while playing > > > further with the code. The big change is of course the selective > > > auditing in the io_uring op servicing, but that has already been > > > discussed quite a bit in the original thread so I won't go into > > > detail here; the important part is that we found a way to move > > > forward and this draft captures that. For those of you looking to > > > play with these patches, they are based on Linus' v5.14-rc5 tag and > > > on my test system they boot and appear to function without problem; > > > they pass the selinux-testsuite and audit-testsuite and I have not > > > noticed any regressions in the normal use of the system. If you want > > > to get a copy of these patches straight from git you can use the > > > "working-io_uring" branch in the repo below: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git > > > https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git > > > > > > Beyond the existing test suite tests mentioned above, I've cobbled > > > together some very basic, very crude tests to exercise some of the > > > things I care about from a LSM/audit perspective. These tests are > > > pretty awful (I'm not kidding), but they might be helpful for the > > > other LSM/audit developers who want to test things: > > > > > > https://drop.paul-moore.com/90.kUgq > > > > > > There are currently two tests: 'iouring.2' and 'iouring.3'; > > > 'iouring.1' was lost in a misguided and overzealous 'rm' command. > > > The first test is standalone and basically tests the SQPOLL > > > functionality while the second tests sharing io_urings across process > > > boundaries and the credential/personality sharing mechanism. The > > > console output of both tests isn't particularly useful, the more > > > interesting bits are in the audit and LSM specific logs. The > > > 'iouring.2' command requires no special arguments to run but the > > > 'iouring.3' test is split into a "server" and "client"; the server > > > should be run without argument: > > > > > > % ./iouring.3s > > > >>> server started, pid = 11678 > > > >>> memfd created, fd = 3 > > > >>> io_uring created; fd = 5, creds = 1 > > > > > > ... while the client should be run with two arguments: the first is > > > the PID of the server process, the second is the "memfd" fd number: > > > > > > % ./iouring.3c 11678 3 > > > >>> client started, server_pid = 11678 server_memfd = 3 > > > >>> io_urings = 5 (server) / 5 (client) > > > >>> io_uring ops using creds = 1 > > > >>> async op result: 36 > > > >>> async op result: 36 > > > >>> async op result: 36 > > > >>> async op result: 36 > > > >>> START file contents > > > What is this life if, full of care, > > > we have no time to stand and stare. > > > >>> END file contents > > > > > > The tests were hacked together from various sources online, > > > attribution and links to additional info can be found in the test > > > sources, but I expect these tests to die a fiery death in the not > > > to distant future as I work to add some proper tests to the SELinux > > > and audit test suites. > > > > > > As I believe these patches should spend a full -rcX cycle in > > > linux-next, my current plan is to continue to solicit feedback on > > > these patches while they undergo additional testing (next up is > > > verification of the audit filter code for io_uring). Assuming no > > > critical issues are found on the mailing lists or during testing, I > > > will post a proper patchset later with the idea of merging i
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Tue, Aug 24, 2021 at 4:57 PM Richard Guy Briggs wrote:
> Thanks for the tests. I have a bunch of userspace patches to add to the
> last set I posted and these tests will help exercise them. I also have
> one more kernel patch to post... I'll dive back into that now. I had
> wanted to post them before now but got distracted with AUDIT_TRIM
> breakage.
If it helps, last week I started working on a little test tool for the
audit-testsuite and selinux-testsuite (see attached). It may not be
final, but I don't expect too many changes to it before I post the
test suite patches; it is definitely usable now. It's inspired by the
previous tests, but it uses a much more test suite friendly fork/exec
model for testing the sharing of io_urings across process boundaries.
Would you mind sharing your latest userspace patches, if not publicly
I would be okay with privately off-list; I'm putting together the test
suite patches this week and it would be good to make sure I'm using
your latest take on the userspace changes.
Also, what is the kernel patch? Did you find a bug or is this some
new functionality you think might be useful? Both can be important,
but the bug is *really* important; even if you don't have a fix for
that, just a description of the problem would be good.
--
paul moore
www.paul-moore.com
/*
* io_uring test tool to exercise LSM/SELinux and audit kernel code paths
* Author: Paul Moore
*
* Copyright 2021 Microsoft Corporation
*
* At the time this code was written the best, and most current, source of info
* on io_uring seemed to be the liburing sources themselves (link below). The
* code below is based on the lessons learned from looking at the liburing
* code.
*
* -> https://github.com/axboe/liburing
*
* The liburing LICENSE file contains the following:
*
* Copyright 2020 Jens Axboe
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
* deal in the Software without restriction, including without limitation the
* rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
* sell copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.
*
*/
/*
* BUILDING:
*
* gcc -o -g -O0 -luring -lrt
*
* RUNNING:
*
* The program can be run using the following command lines:
*
* % prog sqpoll
* ... this invocation runs the io_uring SQPOLL test.
*
* % prog t1
* ... this invocation runs the parent/child io_uring sharing test.
*
* % prog t1
* ... this invocation runs the parent/child io_uring sharing test with the
* child process run in the specified SELinux domain.
*
*/
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
struct urt_config {
struct io_uring ring;
struct io_uring_params ring_params;
int ring_creds;
};
#define URING_ENTRIES8
#define URING_SHM_NAME"/iouring_test_4"
int selinux_state = -1;
#define SELINUX_CTX_MAX512
char selinux_ctx[SELINUX_CTX_MAX] = "\0";
/**
* Display an error message and exit
* @param msg the error message
*
* Output @msg to stderr and exit with errno as the exit value.
*/
void fatal(const char *msg)
{
const char *str = (msg ? msg : "unknown");
if (!errno) {
errno = 1;
fprintf(stderr, "%s: unknown error\n", msg);
} else
perror(str);
if (errno < 0)
exit(-errno);
exit(errno);
}
/**
* Determine if SELinux is enabled and set the internal state
*
* Attempt to read from /proc/self/attr/current and determine if SELinux is
* enabled, store the current context/domain in @selinux_ctx if SELinux is
* enabled. We avoid using the libselinux API in order to increase portability
* and make it easier for other LSMs to adopt this test.
*/
int selinux_enabled(void)
{
int fd = -1;
ssize_t ctx_len;
char ctx[SELINUX_CTX_MAX];
if (selinux_state >= 0)
return selinux_state;
/* attempt to get the current context */
fd = open("/proc/self/attr/current", O_RDONLY);
if (fd < 0)
goto err;
ctx_len = read(fd, ctx, SELINUX_CTX_MAX - 1);
if (ctx_len <= 0)
goto err;
close(fd);
/* save the current context */
ctx[ctx_len] = '\0';
Re: [ghak-trim PATCH v1] audit: move put_tree() to avoid trim_trees refcount underflow and UAF
On Mon, Aug 23, 2021 at 10:05 PM Richard Guy Briggs wrote:
>
> AUDIT_TRIM is expected to be idempotent, but multiple executions resulted in a
> refcount underflow and use-after-free.
>
> git bisect fingered commit fb041bb7c0a918b95c6889fc965cdc4a75b4c0ca (2019-11)
> ("locking/refcount: Consolidate implementations of refcount_t")
> but this patch with its more thorough checking that wasn't in the x86 assembly
> code merely exposed a previously existing tree refcount imbalance in the case
> of tree trimming code that was refactored with prune_one() to remove a tree
> introduced in commit 8432c70062978d9a57bde6715496d585ec520c3e (2018-11)
> ("audit: Simplify locking around untag_chunk()")
>
> Move the put_tree() to cover only the prune_one() case.
>
> Passes audit-testsuite and 3 passes of "auditctl -t" with at least one
> directory watch.
>
> Fixes: 8432c7006297 ("audit: Simplify locking around untag_chunk()")
> Signed-off-by: Richard Guy Briggs
> Cc: Jan Kara
> Cc: Will Deacon
> Cc: Alexander Viro
> Cc: Seiji Nishikawa
> ---
> kernel/audit_tree.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
First a quick comment about the commit description, when referencing
specific commits in the description please use the same format that
you used in the "Fixes:" tag; it makes the description easier to read.
No need to resend, I fixed it when I merged your patch, but something
to keep in mind for the future.
As for the patch itself, thanks for finding this and sending a fix.
Normally this is something I would send up to Linus at the end of the
week during the -rcX phase, but since we are currently at -rc7 I'm
going to simply add the -stable marking and merge it into audit/next
to get pushed up to Linus early next week, assuming we see v5.14
released this Sunday. If for some reason we see a v5.14-rc8 next week
I'll adjust things and send it to Linus as a -stable patch.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 RESEND] powerpc/audit: Convert powerpc to AUDIT_ARCH_COMPAT_GENERIC
On Tue, Aug 24, 2021 at 1:11 PM Christophe Leroy
wrote:
> Le 24/08/2021 à 16:47, Paul Moore a écrit :
> > On Tue, Aug 24, 2021 at 9:36 AM Christophe Leroy
> > wrote:
> >>
> >> Commit e65e1fc2d24b ("[PATCH] syscall class hookup for all normal
> >> targets") added generic support for AUDIT but that didn't include
> >> support for bi-arch like powerpc.
> >>
> >> Commit 4b58841149dc ("audit: Add generic compat syscall support")
> >> added generic support for bi-arch.
> >>
> >> Convert powerpc to that bi-arch generic audit support.
> >>
> >> Cc: Paul Moore
> >> Cc: Eric Paris
> >> Signed-off-by: Christophe Leroy
> >> ---
> >> Resending v2 with Audit people in Cc
> >>
> >> v2:
> >> - Missing 'git add' for arch/powerpc/include/asm/unistd32.h
> >> - Finalised commit description
> >> ---
> >> arch/powerpc/Kconfig| 5 +-
> >> arch/powerpc/include/asm/unistd32.h | 7 +++
> >> arch/powerpc/kernel/Makefile| 3 --
> >> arch/powerpc/kernel/audit.c | 84 -
> >> arch/powerpc/kernel/compat_audit.c | 44 ---
> >> 5 files changed, 8 insertions(+), 135 deletions(-)
> >> create mode 100644 arch/powerpc/include/asm/unistd32.h
> >> delete mode 100644 arch/powerpc/kernel/audit.c
> >> delete mode 100644 arch/powerpc/kernel/compat_audit.c
> >
> > Can you explain, in detail please, the testing you have done to verify
> > this patch?
> >
>
> I built ppc64_defconfig and checked that the generated code is functionnaly
> equivalent.
>
> ppc32_classify_syscall() is exactly the same as
> audit_classify_compat_syscall() except that the
> later takes the syscall as second argument (ie in r4) whereas the former
> takes it as first argument
> (ie in r3).
>
> audit_classify_arch() and powerpc audit_classify_syscall() are slightly
> different between the
> powerpc version and the generic version because the powerpc version checks
> whether it is
> AUDIT_ARCH_PPC or not (ie value 20), while the generic one checks whether it
> has bit
> __AUDIT_ARCH_64BIT set or not (__AUDIT_ARCH_64BIT is the sign bit of a word),
> but taking into
> account that the abi is either AUDIT_ARCH_PPC, AUDIT_ARCH_PPC64 or
> AUDIT_ARCH_PPC64LE, the result is
> the same.
>
> If you are asking I guess you saw something wrong ?
I was asking because I didn't see any mention of testing, and when you
are enabling something significant like this it is nice to see that it
has been verified to work :)
While binary dumps and comparisons are nice, it is always good to see
verification from a test suite. I don't have access to the necessary
hardware to test this, but could you verify that the audit-testsuite
passes on your test system with your patches applied?
* https://github.com/linux-audit/audit-testsuite
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Sat, Aug 28, 2021 at 11:04 AM Richard Guy Briggs wrote: > I did set a syscall filter for > -a exit,always -F arch=b64 -S > io_uring_enter,io_uring_setup,io_uring_register -F key=iouringsyscall > and that yielded some records with a couple of orphans that surprised me > a bit. Without looking too closely at the log you sent, you can expect URING records without an associated SYSCALL record when the uring op is being processed in the io-wq or sqpoll context. In the io-wq case the processing is happening after the thread finished the syscall but before the execution context returns to userspace and in the case of sqpoll the processing is handled by a separate kernel thread with no association to a process thread. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On Fri, Aug 13, 2021 at 2:48 PM Casey Schaufler wrote: > On 8/13/2021 8:31 AM, Paul Moore wrote: > > On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler > > wrote: > >> On 8/12/2021 1:59 PM, Paul Moore wrote: > >>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler > >>> wrote: > >>>> Create a new audit record type to contain the subject information > >>>> when there are multiple security modules that require such data. > > ... > > > >>> The local > >>> audit context is a hack that is made necessary by the fact that we > >>> have to audit things which happen outside the scope of an executing > >>> task, e.g. the netfilter audit hooks, it should *never* be used when > >>> there is a valid task_struct. > >> In the existing audit code a "current context" is only needed for > >> syscall events, so that's the only case where it's allocated. Would > >> you suggest that I track down the non-syscall events that include > >> subj= fields and add allocate a "current context" for them? I looked > >> into doing that, and it wouldn't be simple. > > > > This is why the "local context" was created. Prior to these stacking > > additions, and the audit container ID work, we never needed to group > > multiple audit records outside of a syscall context into a single > > audit event so passing a NULL context into audit_log_start() was > > reasonable. The local context was designed as a way to generate a > > context for use in a local function scope to group multiple records, > > however, for reasons I'll get to below I'm now wondering if the local > > context approach is really workable ... > > I haven't found a place where it didn't work. What is the concern? The concern is that use of a local context can destroy any hopes of linking with other related records, e.g. SYSCALL and PATH records, to form a single cohesive event. If the current task_struct is valid for a given function invocation then we *really* should be using current's audit_context. However, based on our discussion here it would seem that we may have some issues where current->audit_context is not being managed correctly. I'm not surprised, but I will admit to being disappointed. > > What does your audit config look like? Both the kernel command line > > and the output of 'auditctl -l' would be helpful. > > On the fedora system: > > BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+ > root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap > rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug > > -a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW > > On the Ubuntu system: > > BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+ > root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug > > No rules The Fedora system looks to have some audit-testsuite leftovers, but that shouldn't have an impact on what we are discussing; in both cases I would expect current->audit_context to be allocated and non-NULL. > > I'm beginning to suspect that you have the default > > we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime > > audit configuration that is de rigueur for many distros these days. > > Yes, but I've also fiddled about with it so as to get better event coverage. > I've run the audit-testsuite, which has got to fiddle about with the audit > configuration. Yes, it looks like my hunch was wrong. > > If that is the case, there are many cases where you would not see a > > NULL current->audit_context simply because the config never allocated > > one, see kernel/auditsc.c:audit_alloc(). > > I assume you mean that I *would* see a NULL current->audit_context > in the "event not enabled" case. Yep, typo. > > Regardless, assuming that is the case we probably need to find an > > alternative to the local context approach as it currently works. For > > reasons we already talked about, we don't want to use a local > > audit_context if there is the possibility for a proper > > current->audit_context, but we need to do *something* so that we can > > group these multiple events into a single record. > > I tried a couple things, but neither was satisfactory. > > > Since this is just occurring to me now I need a bit more time to think > > on possible solutions - all good ideas are welcome - but the first > > thing that pops into my head is that we need to augment > > audit_log_end() to potentially generated additional, associated > > records similar to what we do on syscall exit in audit_log_exit(
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On Fri, Aug 13, 2021 at 5:47 PM Casey Schaufler wrote: > On 8/13/2021 1:43 PM, Paul Moore wrote: > > On Fri, Aug 13, 2021 at 2:48 PM Casey Schaufler > > wrote: > >> On 8/13/2021 8:31 AM, Paul Moore wrote: > >>> On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler > >>> wrote: > >>>> On 8/12/2021 1:59 PM, Paul Moore wrote: > >>>>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler > >>>>> wrote: > >>>>>> Create a new audit record type to contain the subject information > >>>>>> when there are multiple security modules that require such data. > >>> ... > >>> > >>>>> The local > >>>>> audit context is a hack that is made necessary by the fact that we > >>>>> have to audit things which happen outside the scope of an executing > >>>>> task, e.g. the netfilter audit hooks, it should *never* be used when > >>>>> there is a valid task_struct. > >>>> In the existing audit code a "current context" is only needed for > >>>> syscall events, so that's the only case where it's allocated. Would > >>>> you suggest that I track down the non-syscall events that include > >>>> subj= fields and add allocate a "current context" for them? I looked > >>>> into doing that, and it wouldn't be simple. > >>> This is why the "local context" was created. Prior to these stacking > >>> additions, and the audit container ID work, we never needed to group > >>> multiple audit records outside of a syscall context into a single > >>> audit event so passing a NULL context into audit_log_start() was > >>> reasonable. The local context was designed as a way to generate a > >>> context for use in a local function scope to group multiple records, > >>> however, for reasons I'll get to below I'm now wondering if the local > >>> context approach is really workable ... > >> I haven't found a place where it didn't work. What is the concern? > > The concern is that use of a local context can destroy any hopes of > > linking with other related records, e.g. SYSCALL and PATH records, to > > form a single cohesive event. If the current task_struct is valid for > > a given function invocation then we *really* should be using current's > > audit_context. > > > > However, based on our discussion here it would seem that we may have > > some issues where current->audit_context is not being managed > > correctly. I'm not surprised, but I will admit to being disappointed. > > I'd believe that with syscall audit being a special case for other reasons > the multiple record situation got taken care of on a case-by-case basis > and no one really paid much attention to generality. It's understandable. > > >>> What does your audit config look like? Both the kernel command line > >>> and the output of 'auditctl -l' would be helpful. > >> On the fedora system: > >> > >> BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+ > >> root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap > >> rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug > >> > >> -a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW > >> > >> On the Ubuntu system: > >> > >> BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+ > >> root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug > >> > >> No rules > > The Fedora system looks to have some audit-testsuite leftovers, but > > that shouldn't have an impact on what we are discussing; in both cases > > I would expect current->audit_context to be allocated and non-NULL. > > As would I. > > > >>> I'm beginning to suspect that you have the default > >>> we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime > >>> audit configuration that is de rigueur for many distros these days. > >> Yes, but I've also fiddled about with it so as to get better event > >> coverage. > >> I've run the audit-testsuite, which has got to fiddle about with the audit > >> configuration. > > Yes, it looks like my hunch was wrong. > > > >>> If that is the case, there are many cases where you would not see a > >>> NULL current->audit_context simply because the config never allocated > >>> one, see kernel/auditsc.c:audit_alloc(). > >> I assume you mean that I *
Re: [RFC PATCH 2/9] audit, io_uring, io-wq: add some basic audit support to io_uring
On Tue, Aug 24, 2021 at 9:21 PM Richard Guy Briggs wrote: > > On 2021-06-02 13:46, Paul Moore wrote: > > On Wed, Jun 2, 2021 at 1:29 PM Richard Guy Briggs wrote: > > > On 2021-05-21 17:49, Paul Moore wrote: > > > > WARNING - This is a work in progress and should not be merged > > > > anywhere important. It is almost surely not complete, and while it > > > > probably compiles it likely hasn't been booted and will do terrible > > > > things. You have been warned. > > > > > > > > This patch adds basic auditing to io_uring operations, regardless of > > > > their context. This is accomplished by allocating audit_context > > > > structures for the io-wq worker and io_uring SQPOLL kernel threads > > > > as well as explicitly auditing the io_uring operations in > > > > io_issue_sqe(). The io_uring operations are audited using a new > > > > AUDIT_URINGOP record, an example is shown below: > > > > > > > > % > > > > > > > > Thanks to Richard Guy Briggs for review and feedback. > > > > > > > > Signed-off-by: Paul Moore > > > > --- > > > > fs/io-wq.c |4 + > > > > fs/io_uring.c | 11 +++ > > > > include/linux/audit.h | 17 > > > > include/uapi/linux/audit.h |1 > > > > kernel/audit.h |2 + > > > > kernel/auditsc.c | 173 > > > > > > > > 6 files changed, 208 insertions(+) ... > > > > + if (ctx->return_valid != AUDITSC_INVALID) > > > > + audit_log_format(ab, " success=%s exit=%ld", > > > > + (ctx->return_valid == AUDITSC_SUCCESS ? > > > > + "yes" : "no"), > > > > + ctx->return_code); > > > > + audit_log_format(ab, > > > > + " items=%d" > > > > + " ppid=%d pid=%d auid=%u uid=%u gid=%u" > > > > + " euid=%u suid=%u fsuid=%u" > > > > + " egid=%u sgid=%u fsgid=%u", > > > > + ctx->name_count, > > > > + task_ppid_nr(current), > > > > + task_tgid_nr(current), > > > > + from_kuid(_user_ns, > > > > audit_get_loginuid(current)), > > > > + from_kuid(_user_ns, cred->uid), > > > > + from_kgid(_user_ns, cred->gid), > > > > + from_kuid(_user_ns, cred->euid), > > > > + from_kuid(_user_ns, cred->suid), > > > > + from_kuid(_user_ns, cred->fsuid), > > > > + from_kgid(_user_ns, cred->egid), > > > > + from_kgid(_user_ns, cred->sgid), > > > > + from_kgid(_user_ns, cred->fsgid)); > > > > > > The audit session ID is still important, relevant and qualifies auid. > > > In keeping with the SYSCALL record format, I think we want to keep > > > ses=audit_get_sessionid(current) in here. > > > > This might be another case of syscall/io_uring confusion. An io_uring > > op doesn't necessarily have an audit session ID or an audit UID in the > > conventional sense; for example think about SQPOLL works, shared > > rings, etc. > > Right, but those syscalls are what instigate io_uring operations, so > whatever process starts that operation, or gets handed that handle > should be tracked with auid and sessionid (the two work together to > track) unless we can easily track io_uring ops to connect them to a > previous setup syscall. If we see a need to keep the auid, then the > sessionid goes with it. As a reminder, once the io_uring is created appropriately one can issue io_uring operations without making a syscall. Further, sharing a io_uring across process boundaries means that both the audit session ID and audit login UID used to create the io_uring might not be the same as the subject which issues operations to the io_uring. Any io_uring operations that happen synchronously as the result of a syscall should be associated with the SYSCALL record so the session ID and login UID will be part of the event. Asynchronous operations will not have that information because we don't have a way to get it. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Sun, Aug 29, 2021 at 11:18 AM Paul Moore wrote: > On Sat, Aug 28, 2021 at 11:04 AM Richard Guy Briggs wrote: > > I did set a syscall filter for > > -a exit,always -F arch=b64 -S > > io_uring_enter,io_uring_setup,io_uring_register -F key=iouringsyscall > > and that yielded some records with a couple of orphans that surprised me > > a bit. > > Without looking too closely at the log you sent, you can expect URING > records without an associated SYSCALL record when the uring op is > being processed in the io-wq or sqpoll context. In the io-wq case the > processing is happening after the thread finished the syscall but > before the execution context returns to userspace and in the case of > sqpoll the processing is handled by a separate kernel thread with no > association to a process thread. I spent some time this morning/afternoon playing with the io_uring audit filtering capability and with your audit userspace ghau-iouring-filtering.v1.0 branch it appears to work correctly. Yes, the userspace tooling isn't quite 100% yet (e.g. `auditctl -l` doesn't map the io_uring ops correctly), but I know you mentioned you have a number of fixes/improvements still as a work-in-progress there so I'm not too concerned. The important part is that the kernel pieces look to be working correctly. As usual, if you notice anything awry while playing with the userspace changes please let me know. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: Fix build failure by renaming struct node to struct audit_node
struct node *node = list_entry(p, struct node, list);
> + struct audit_node *node = list_entry(p, struct audit_node,
> list);
> q = p->next;
> if (node->index & (1U<<31)) {
> list_del_init(p);
> @@ -684,7 +684,7 @@ void audit_trim_trees(void)
> struct audit_tree *tree;
> struct path path;
> struct vfsmount *root_mnt;
> - struct node *node;
> + struct audit_node *node;
> int err;
>
> tree = container_of(cursor.next, struct audit_tree, list);
> @@ -839,7 +839,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
> drop_collected_mounts(mnt);
>
> if (!err) {
> - struct node *node;
> + struct audit_node *node;
> spin_lock(_lock);
> list_for_each_entry(node, >chunks, list)
> node->index &= ~(1U<<31);
> @@ -938,7 +938,7 @@ int audit_tag_tree(char *old, char *new)
> mutex_unlock(_filter_mutex);
>
> if (!failed) {
> - struct node *node;
> + struct audit_node *node;
> spin_lock(_lock);
> list_for_each_entry(node, >chunks, list)
> node->index &= ~(1U<<31);
> --
> 2.25.0
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 1/3] dm: introduce audit event module for device mapper
ntegrity
> op=integrity-checksum dev=254:3 sector 77480 res=0
> type=UNKNOWN[1337] msg=audit(1630425112.119:203): module=integrity
> op=integrity-checksum dev=254:3 sector 77480 res=0
>
> Signed-off-by: Michael Weiß
> ---
> drivers/md/Kconfig | 10 +
> drivers/md/Makefile| 4 ++
> drivers/md/dm-audit.c | 79 ++
> drivers/md/dm-audit.h | 62 ++
> include/uapi/linux/audit.h | 2 +
> 5 files changed, 157 insertions(+)
> create mode 100644 drivers/md/dm-audit.c
> create mode 100644 drivers/md/dm-audit.h
>
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index 0602e82a9516..48adbec12148 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -608,6 +608,7 @@ config DM_INTEGRITY
> select CRYPTO
> select CRYPTO_SKCIPHER
> select ASYNC_XOR
> + select DM_AUDIT if AUDIT
> help
> This device-mapper target emulates a block device that has
> additional per-sector tags that can be used for storing
> @@ -640,4 +641,13 @@ config DM_ZONED
>
> If unsure, say N.
>
> +config DM_AUDIT
> + bool "DM audit events"
> + depends on AUDIT
> + help
> + Generate audit events for device-mapper.
> +
> + Enables audit logging of several security relevant events in the
> + particular device-mapper targets, especially the integrity target.
> +
> endif # MD
> diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> index a74aaf8b1445..2f83d649500d 100644
> --- a/drivers/md/Makefile
> +++ b/drivers/md/Makefile
> @@ -103,3 +103,7 @@ endif
> ifeq ($(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG),y)
> dm-verity-objs += dm-verity-verify-sig.o
> endif
> +
> +ifeq ($(CONFIG_DM_AUDIT),y)
> +dm-mod-objs+= dm-audit.o
> +endif
> diff --git a/drivers/md/dm-audit.c b/drivers/md/dm-audit.c
> new file mode 100644
> index ..761ecfdcd49a
> --- /dev/null
> +++ b/drivers/md/dm-audit.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Creating audit records for mapped devices.
> + *
> + * Copyright (C) 2021 Fraunhofer AISEC. All rights reserved.
> + *
> + * Authors: Michael Weiß
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "dm-audit.h"
> +#include "dm-core.h"
> +
> +static struct audit_buffer *dm_audit_log_start(int audit_type,
> + const char *dm_msg_prefix,
> + const char *op)
> +{
> + struct audit_buffer *ab;
> +
> + if (audit_enabled == AUDIT_OFF)
> + return NULL;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, audit_type);
> + if (unlikely(!ab))
> + return NULL;
> +
> + audit_log_format(ab, "module=%s op=%s", dm_msg_prefix, op);
> + return ab;
> +}
> +
> +void dm_audit_log_ti(int audit_type, const char *dm_msg_prefix, const char
> *op,
> +struct dm_target *ti, int result)
> +{
> + struct audit_buffer *ab;
> + struct mapped_device *md = dm_table_get_md(ti->table);
> + int dev_major = dm_disk(md)->major;
> + int dev_minor = dm_disk(md)->first_minor;
> +
> + ab = dm_audit_log_start(audit_type, dm_msg_prefix, op);
> + if (unlikely(!ab))
> + return;
> +
> + switch (audit_type) {
> + case AUDIT_DM_CTRL:
> + audit_log_task_info(ab);
> + audit_log_format(ab, " dev=%d:%d error_msg='%s'", dev_major,
> +dev_minor, !result ? ti->error : "success");
> + break;
> + case AUDIT_DM_EVENT:
> + audit_log_format(ab, " dev=%d:%d sector=?", dev_major,
> +dev_minor);
> + break;
> + }
> + audit_log_format(ab, " res=%d", result);
> + audit_log_end(ab);
> +}
> +EXPORT_SYMBOL_GPL(dm_audit_log_ti);
Just checking, but are you okay when the inevitable happens and
someone passes an @audit_type that is not either AUDIT_CM_CTRL or
AUDIT_DM_EVENT? Right now that will succeed without error and give a
rather short audit record.
> +void dm_audit_log_bio(const char *dm_msg_prefix, const char *op,
> + struct bio *bio, sector_t sector, int result)
> +{
> + struct audit_buffer *ab;
> + int dev_major = MAJOR(bio->bi_bdev->bd_dev);
> + int dev_minor = MINOR(bio->bi_bdev->bd_dev);
> +
> + ab = dm_audit_log_start(AUDIT_DM_EVENT, dm_msg_prefix, op);
> + if (unlikely(!ab))
> + return;
> +
> + audit_log_format(ab, " dev=%d:%d sector %llu res=%d",
> +dev_major, dev_minor, sector, result);
I think you forgot the "=" after "sector", e.g. "sector=%llu".
> + audit_log_end(ab);
> +}
> +EXPORT_SYMBOL_GPL(dm_audit_log_bio);
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: Fix build failure by renaming struct node to struct audit_node
On Mon, Sep 6, 2021 at 2:41 AM LEROY Christophe
wrote:
> Le 03/09/2021 à 19:06, Paul Moore a écrit :
> > On Fri, Sep 3, 2021 at 11:48 AM Christophe Leroy
> > wrote:
> >>
> >> struct node defined in kernel/audit_tree.c conflicts with
> >> struct node defined in include/linux/node.h
> >>
> >>CC kernel/audit_tree.o
> >> kernel/audit_tree.c:33:9: error: redefinition of 'struct node'
> >> 33 | struct node {
> >>| ^~~~
> >> In file included from ./include/linux/cpu.h:17,
> >> from ./include/linux/static_call.h:102,
> >> from ./arch/powerpc/include/asm/machdep.h:10,
> >> from ./arch/powerpc/include/asm/archrandom.h:7,
> >> from ./include/linux/random.h:121,
> >> from ./include/linux/net.h:18,
> >> from ./include/linux/skbuff.h:26,
> >> from kernel/audit.h:11,
> >> from kernel/audit_tree.c:2:
> >> ./include/linux/node.h:84:8: note: originally defined here
> >> 84 | struct node {
> >>|^~~~
> >> make[2]: *** [kernel/audit_tree.o] Error 1
> >>
> >> Rename it audit_node.
> >>
> >> Signed-off-by: Christophe Leroy
> >> ---
> >> kernel/audit_tree.c | 20 ++--
> >> 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > That's interesting, I wonder why we didn't see this prior? Also as an
> > aside, there are evidently a good handful of symbols named "node". In
> > fact I don't see this now in the audit/stable-5.15 or Linus' tree as
> > of a right now, both using an allyesconfig:
> >
> > % git show-ref HEAD
> > a9c9a6f741cdaa2fa9ba24a790db8d07295761e3 refs/remotes/linus/HEAD
> > % touch kernel/audit_tree.c
> > % make C=1 kernel/
> > CALLscripts/checksyscalls.sh
> > CALLscripts/atomic/check-atomics.sh
> > DESCEND objtool
> > CHK kernel/kheaders_data.tar.xz
> > CC kernel/audit_tree.o
> > CHECK kernel/audit_tree.c
> > AR kernel/built-in.a
> >
> > What tree and config are you using where you see this error? Looking
> > at your error, I'm guessing this is limited to ppc builds, and if I
> > look at the arch/powerpc/include/asm/machdep.h file in Linus tree I
> > don't see a static_call.h include so I'm guessing this is a -next tree
> > for ppc? Something else?
> >
> > Without knowing the context, is adding the static_call.h include in
> > arch/powerpc/include/asm/machdep.h intentional or simply a bit of
> > include file creep?
>
> struct machdep_calls in asm/machdep.h is full of function pointers and
> I'm working on converting that to static_calls
> (https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=260878=*)
>
> So yes, adding static_call.h in asm/machdep.h is intentional and the
> issue was detected by CI build test
> (http://kisskb.ellerman.id.au/kisskb/buildresult/14628100/)
>
> I submitted this change to you because for me it make sense to not
> re-use globably defined struct names in local C files, and anybody may
> encounter the problem as soon as linux/node.h gets included directly or
> indirectly. But if you prefer I guess the fix may be merged through
> powerpc tree as part of this series.
Yes, this patch should go in via the audit tree, and while I don't
have an objection to the patch, whenever I see a patch to fix an issue
that is not visible in Linus' tree or the audit tree it raises some
questions. I usually hope to see those questions answered proactively
in the cover letter and/or patch description but that wasn't the case
here so you get to play a game of 20 questions.
Speaking of which, I don't recall seeing an answer to the "where do
these include file changes live?" question, is is the ppc -next tree,
or are they still unmerged and just on the ppc list?
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Wed, Sep 15, 2021 at 8:29 AM Richard Guy Briggs wrote: > I was in the middle of reviewing the v2 patchset to add my acks when I > forgot to add the comment that you still haven't convinced me that ses= > isn't needed or relevant if we are including auid=. [Side note: v3 was posted on Monday, it would be more helpful to see the Reviewed-by tags on the v3 patchset.] Ah, okay, it wasn't clear to me from your earlier comments that this was your concern. It sounded as if you were arguing that both session ID and audit ID needed to be logged for every io_uring op, which doesn't make sense (as previously discussed). However, I see your point, and in fact pulling the audit ID from @current in the audit_log_uring() function is just plain wrong ... likely a vestige of the original copy-n-paste or format matching, I'll drop that now. Thanks. While a small code change, it is somewhat significant so I'll post an updated v4 patchset later today once it passes through a round of testing. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Mon, Sep 13, 2021 at 9:50 PM Paul Moore wrote: > On Mon, Sep 13, 2021 at 3:23 PM Paul Moore wrote: > > On Thu, Sep 9, 2021 at 8:59 PM Richard Guy Briggs wrote: > > > On 2021-09-01 15:21, Paul Moore wrote: > > > > On Sun, Aug 29, 2021 at 11:18 AM Paul Moore wrote: > > > > > On Sat, Aug 28, 2021 at 11:04 AM Richard Guy Briggs > > > > > wrote: > > > > > > I did set a syscall filter for > > > > > > -a exit,always -F arch=b64 -S > > > > > > io_uring_enter,io_uring_setup,io_uring_register -F > > > > > > key=iouringsyscall > > > > > > and that yielded some records with a couple of orphans that > > > > > > surprised me > > > > > > a bit. > > > > > > > > > > Without looking too closely at the log you sent, you can expect URING > > > > > records without an associated SYSCALL record when the uring op is > > > > > being processed in the io-wq or sqpoll context. In the io-wq case the > > > > > processing is happening after the thread finished the syscall but > > > > > before the execution context returns to userspace and in the case of > > > > > sqpoll the processing is handled by a separate kernel thread with no > > > > > association to a process thread. > > > > > > > > I spent some time this morning/afternoon playing with the io_uring > > > > audit filtering capability and with your audit userspace > > > > ghau-iouring-filtering.v1.0 branch it appears to work correctly. Yes, > > > > the userspace tooling isn't quite 100% yet (e.g. `auditctl -l` doesn't > > > > map the io_uring ops correctly), but I know you mentioned you have a > > > > number of fixes/improvements still as a work-in-progress there so I'm > > > > not too concerned. The important part is that the kernel pieces look > > > > to be working correctly. > > > > > > Ok, I have squashed and pushed the audit userspace support for iouring: > > > > > > https://github.com/rgbriggs/audit-userspace/commit/e8bd8d2ea8adcaa758024cb9b8fa93895ae35eea > > > > > > https://github.com/linux-audit/audit-userspace/compare/master...rgbriggs:ghak-iouring-filtering.v2.1 > > > There are test rpms for f35 here: > > > http://people.redhat.com/~rbriggs/ghak-iouring/git-e8bd8d2-fc35/ > > > > > > userspace v2 changelog: > > > - check for watch before adding perm > > > - update manpage to include filesystem filter > > > - update support for the uring filter list: doc, -U op, op names > > > - add support for the AUDIT_URINGOP record type > > > - add uringop support to ausearch > > > - add uringop support to aureport > > > - lots of bug fixes > > > > > > "auditctl -a uring,always -S ..." will now throw an error and require > > > "-U" instead. > > > > Thanks Richard. > > > > FYI, I rebased the io_uring/LSM/audit patchset on top of v5.15-rc1 > > today and tested both with your v1.0 and with your v2.1 branch and the > > various combinations seemed to work just fine (of course the v2.1 > > userspace branch was more polished, less warts, etc.). I'm going to > > go over the patch set one more time to make sure everything is still > > looking good, write up an updated cover letter, and post a v3 revision > > later tonight with the hope of merging it into -next later this week. > > Best laid plans of mice and men ... > > It turns out the LSM hook macros are full of warnings-now-errors that > should likely be resolved before sending anything LSM related to > Linus. I'll post v3 once I fix this, which may not be until tomorrow. > > (To be clear, the warnings/errors aren't new to this patchset, I'm > likely just the first person to notice them.) Actually, scratch that ... I'm thinking that might just be an oddity of the Intel 0day test robot building for the xtensa arch. I'll post the v3 patchset tonight. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 2/8] audit, io_uring, io-wq: add some basic audit support to io_uring
This patch adds basic auditing to io_uring operations, regardless of
their context. This is accomplished by allocating audit_context
structures for the io-wq worker and io_uring SQPOLL kernel threads
as well as explicitly auditing the io_uring operations in
io_issue_sqe(). Individual io_uring operations can bypass auditing
through the "audit_skip" field in the struct io_op_def definition for
the operation; although great care must be taken so that security
relevant io_uring operations do not bypass auditing; please contact
the audit mailing list (see the MAINTAINERS file) with any questions.
The io_uring operations are audited using a new AUDIT_URINGOP record,
an example is shown below:
type=UNKNOWN[1336] msg=audit(1630523381.288:260):
uring_op=19 success=yes exit=0 items=0 ppid=853 pid=1204
auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
key=(null)
AUID="root" UID="root" GID="root" EUID="root" SUID="root"
FSUID="root" EGID="root" SGID="root" FSGID="root"
Thanks to Richard Guy Briggs for review and feedback.
Signed-off-by: Paul Moore
---
v3:
- removed work-in-progress warning from the description
v2:
- added dummy funcs for audit_uring_{entry,exit}()
- replaced opcode checks in io_issue_sqe() with audit_skip checks
- moved fastpath checks into audit_uring_{entry,exit}()
- audit_log_uring() uses GFP_ATOMIC
- don't record the arch in __audit_uring_entry()
v1:
- initial draft
---
fs/io-wq.c |4 +
fs/io_uring.c | 55 --
include/linux/audit.h | 26 +++
include/uapi/linux/audit.h |1
kernel/audit.h |2 +
kernel/auditsc.c | 174
6 files changed, 256 insertions(+), 6 deletions(-)
diff --git a/fs/io-wq.c b/fs/io-wq.c
index 6c55362c1f99..dac5c5961c9d 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include "io-wq.h"
@@ -562,6 +563,8 @@ static int io_wqe_worker(void *data)
snprintf(buf, sizeof(buf), "iou-wrk-%d", wq->task->pid);
set_task_comm(current, buf);
+ audit_alloc_kernel(current);
+
while (!test_bit(IO_WQ_BIT_EXIT, >state)) {
long ret;
@@ -601,6 +604,7 @@ static int io_wqe_worker(void *data)
io_worker_handle_work(worker);
}
+ audit_free(current);
io_worker_exit(worker);
return 0;
}
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 16fb7436043c..388754b24785 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -79,6 +79,7 @@
#include
#include
#include
+#include
#define CREATE_TRACE_POINTS
#include
@@ -917,6 +918,8 @@ struct io_op_def {
unsignedneeds_async_setup : 1;
/* should block plug */
unsignedplug : 1;
+ /* skip auditing */
+ unsignedaudit_skip : 1;
/* size of async data needed, if any */
unsigned short async_size;
};
@@ -930,6 +933,7 @@ static const struct io_op_def io_op_defs[] = {
.buffer_select = 1,
.needs_async_setup = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_WRITEV] = {
@@ -939,16 +943,19 @@ static const struct io_op_def io_op_defs[] = {
.pollout= 1,
.needs_async_setup = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_FSYNC] = {
.needs_file = 1,
+ .audit_skip = 1,
},
[IORING_OP_READ_FIXED] = {
.needs_file = 1,
.unbound_nonreg_file= 1,
.pollin = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_WRITE_FIXED] = {
@@ -957,15 +964,20 @@ static const struct io_op_def io_op_defs[] = {
.unbound_nonreg_file= 1,
.pollout= 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_POLL_ADD] = {
.needs_file = 1,
.unbound_nonreg_file= 1,
+ .audit_skip = 1,
+ },
+ [IORING_OP_POLL_REMOVE] = {
+ .audit_skip
[PATCH v3 3/8] audit: add filtering for io_uring records
This patch adds basic audit io_uring filtering, using as much of the
existing audit filtering infrastructure as possible. In order to do
this we reuse the audit filter rule's syscall mask for the io_uring
operation and we create a new filter for io_uring operations as
AUDIT_FILTER_URING_EXIT/audit_filter_list[7].
Thanks to Richard Guy Briggs for his review, feedback, and work on
the corresponding audit userspace changes.
Signed-off-by: Paul Moore
---
v3:
- removed work-in-progress warning from the description
v2:
- incorporate feedback from Richard
v1:
- initial draft
---
include/uapi/linux/audit.h |3 +-
kernel/audit_tree.c|3 +-
kernel/audit_watch.c |3 +-
kernel/auditfilter.c | 15 +--
kernel/auditsc.c | 61 ++--
5 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index a1997697c8b1..ecf1edd2affa 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -167,8 +167,9 @@
#define AUDIT_FILTER_EXCLUDE 0x05/* Apply rule before record creation */
#define AUDIT_FILTER_TYPE AUDIT_FILTER_EXCLUDE /* obsolete misleading
naming */
#define AUDIT_FILTER_FS0x06/* Apply rule at
__audit_inode_child */
+#define AUDIT_FILTER_URING_EXIT0x07/* Apply rule at io_uring op
exit */
-#define AUDIT_NR_FILTERS 7
+#define AUDIT_NR_FILTERS 8
#define AUDIT_FILTER_PREPEND 0x10/* Prepend to front of list */
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 2cd7b5694422..338c53a961c5 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -726,7 +726,8 @@ int audit_make_tree(struct audit_krule *rule, char
*pathname, u32 op)
{
if (pathname[0] != '/' ||
- rule->listnr != AUDIT_FILTER_EXIT ||
+ (rule->listnr != AUDIT_FILTER_EXIT &&
+rule->listnr != AUDIT_FILTER_URING_EXIT) ||
op != Audit_equal ||
rule->inode_f || rule->watch || rule->tree)
return -EINVAL;
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 2acf7ca49154..698b62b4a2ec 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -183,7 +183,8 @@ int audit_to_watch(struct audit_krule *krule, char *path,
int len, u32 op)
return -EOPNOTSUPP;
if (path[0] != '/' || path[len-1] == '/' ||
- krule->listnr != AUDIT_FILTER_EXIT ||
+ (krule->listnr != AUDIT_FILTER_EXIT &&
+krule->listnr != AUDIT_FILTER_URING_EXIT) ||
op != Audit_equal ||
krule->inode_f || krule->watch || krule->tree)
return -EINVAL;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index db2c6b59dfc3..d75acb014ccd 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -44,7 +44,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
LIST_HEAD_INIT(audit_filter_list[6]),
-#if AUDIT_NR_FILTERS != 7
+ LIST_HEAD_INIT(audit_filter_list[7]),
+#if AUDIT_NR_FILTERS != 8
#error Fix audit_filter_list initialiser
#endif
};
@@ -56,6 +57,7 @@ static struct list_head audit_rules_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
LIST_HEAD_INIT(audit_rules_list[6]),
+ LIST_HEAD_INIT(audit_rules_list[7]),
};
DEFINE_MUTEX(audit_filter_mutex);
@@ -151,7 +153,8 @@ char *audit_unpack_string(void **bufp, size_t *remain,
size_t len)
static inline int audit_to_inode(struct audit_krule *krule,
struct audit_field *f)
{
- if (krule->listnr != AUDIT_FILTER_EXIT ||
+ if ((krule->listnr != AUDIT_FILTER_EXIT &&
+krule->listnr != AUDIT_FILTER_URING_EXIT) ||
krule->inode_f || krule->watch || krule->tree ||
(f->op != Audit_equal && f->op != Audit_not_equal))
return -EINVAL;
@@ -248,6 +251,7 @@ static inline struct audit_entry
*audit_to_entry_common(struct audit_rule_data *
pr_err("AUDIT_FILTER_ENTRY is deprecated\n");
goto exit_err;
case AUDIT_FILTER_EXIT:
+ case AUDIT_FILTER_URING_EXIT:
case AUDIT_FILTER_TASK:
#endif
case AUDIT_FILTER_USER:
@@ -332,6 +336,10 @@ static int audit_field_valid(struct audit_entry *entry,
struct audit_field *f)
if (entry->rule.listnr != AUDIT_FILTER_FS)
return -EINVAL;
break;
+ case AUDIT_PERM:
+ if (entry->rule.listnr == AUDIT_FILTER_URING_EXIT)
+ return -EINVAL;
+ break;
}
switch (entry->rule.listnr) {
@@ -980,7 +988,8 @@ static inlin
[PATCH v3 6/8] lsm,io_uring: add LSM hooks to io_uring
A full expalantion of io_uring is beyond the scope of this commit
description, but in summary it is an asynchronous I/O mechanism
which allows for I/O requests and the resulting data to be queued
in memory mapped "rings" which are shared between the kernel and
userspace. Optionally, io_uring offers the ability for applications
to spawn kernel threads to dequeue I/O requests from the ring and
submit the requests in the kernel, helping to minimize the syscall
overhead. Rings are accessed in userspace by memory mapping a file
descriptor provided by the io_uring_setup(2), and can be shared
between applications as one might do with any open file descriptor.
Finally, process credentials can be registered with a given ring
and any process with access to that ring can submit I/O requests
using any of the registered credentials.
While the io_uring functionality is widely recognized as offering a
vastly improved, and high performing asynchronous I/O mechanism, its
ability to allow processes to submit I/O requests with credentials
other than its own presents a challenge to LSMs. When a process
creates a new io_uring ring the ring's credentials are inhertied
from the calling process; if this ring is shared with another
process operating with different credentials there is the potential
to bypass the LSMs security policy. Similarly, registering
credentials with a given ring allows any process with access to that
ring to submit I/O requests with those credentials.
In an effort to allow LSMs to apply security policy to io_uring I/O
operations, this patch adds two new LSM hooks. These hooks, in
conjunction with the LSM anonymous inode support previously
submitted, allow an LSM to apply access control policy to the
sharing of io_uring rings as well as any io_uring credential changes
requested by a process.
The new LSM hooks are described below:
* int security_uring_override_creds(cred)
Controls if the current task, executing an io_uring operation,
is allowed to override it's credentials with @cred. In cases
where the current task is a user application, the current
credentials will be those of the user application. In cases
where the current task is a kernel thread servicing io_uring
requests the current credentials will be those of the io_uring
ring (inherited from the process that created the ring).
* int security_uring_sqpoll(void)
Controls if the current task is allowed to create an io_uring
polling thread (IORING_SETUP_SQPOLL). Without a SQPOLL thread
in the kernel processes must submit I/O requests via
io_uring_enter(2) which allows us to compare any requested
credential changes against the application making the request.
With a SQPOLL thread, we can no longer compare requested
credential changes against the application making the request,
the comparison is made against the ring's credentials.
Signed-off-by: Paul Moore
---
v3:
- removed work-in-progress warning from the description
v2:
- no change
v1:
- initial draft
---
fs/io_uring.c | 10 ++
include/linux/lsm_hook_defs.h |5 +
include/linux/lsm_hooks.h | 13 +
include/linux/security.h | 16
security/security.c | 12
5 files changed, 56 insertions(+)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 56cc9aba0d01..f89d00af3a67 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -80,6 +80,7 @@
#include
#include
#include
+#include
#define CREATE_TRACE_POINTS
#include
@@ -7070,6 +7071,11 @@ static int io_init_req(struct io_ring_ctx *ctx, struct
io_kiocb *req,
if (!req->creds)
return -EINVAL;
get_cred(req->creds);
+ ret = security_uring_override_creds(req->creds);
+ if (ret) {
+ put_cred(req->creds);
+ return ret;
+ }
req->flags |= REQ_F_CREDS;
}
state = >submit_state;
@@ -8566,6 +8572,10 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
struct io_sq_data *sqd;
bool attached;
+ ret = security_uring_sqpoll();
+ if (ret)
+ return ret;
+
sqd = io_get_sq_data(p, );
if (IS_ERR(sqd)) {
ret = PTR_ERR(sqd);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2adeea44c0d5..b3c525353769 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -402,3 +402,8 @@ LSM_HOOK(void, LSM_RET_VOID, perf_event_free, struct
perf_event *event)
LSM_HOOK(int, 0, perf_event_read, struct perf_event *event)
LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
#endif /* CONFIG_PERF_EVENTS */
+
+#ifdef CONFIG_IO_URING
+LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
+LSM_HOOK(int, 0, uring_sqpoll, void
[PATCH v3 8/8] Smack: Brutalist io_uring support with debug
From: Casey Schaufler
Add Smack privilege checks for io_uring. Use CAP_MAC_OVERRIDE
for the override_creds case and CAP_MAC_ADMIN for creating a
polling thread. These choices are based on conjecture regarding
the intent of the surrounding code.
Signed-off-by: Casey Schaufler
[PM: make the smack_uring_* funcs static, remove debug code]
Signed-off-by: Paul Moore
---
v3:
- removed debug code
v2:
- made the smack_uring_* funcs static
v1:
- initial draft
---
security/smack/smack_lsm.c | 46
1 file changed, 46 insertions(+)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index cacbe7518519..f90ab1efeb6d 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4691,6 +4691,48 @@ static int smack_dentry_create_files_as(struct dentry
*dentry, int mode,
return 0;
}
+#ifdef CONFIG_IO_URING
+/**
+ * smack_uring_override_creds - Is io_uring cred override allowed?
+ * @new: the target creds
+ *
+ * Check to see if the current task is allowed to override it's credentials
+ * to service an io_uring operation.
+ */
+static int smack_uring_override_creds(const struct cred *new)
+{
+ struct task_smack *tsp = smack_cred(current_cred());
+ struct task_smack *nsp = smack_cred(new);
+
+ /*
+* Allow the degenerate case where the new Smack value is
+* the same as the current Smack value.
+*/
+ if (tsp->smk_task == nsp->smk_task)
+ return 0;
+
+ if (smack_privileged_cred(CAP_MAC_OVERRIDE, current_cred()))
+ return 0;
+
+ return -EPERM;
+}
+
+/**
+ * smack_uring_sqpoll - check if a io_uring polling thread can be created
+ *
+ * Check to see if the current task is allowed to create a new io_uring
+ * kernel polling thread.
+ */
+static int smack_uring_sqpoll(void)
+{
+ if (smack_privileged_cred(CAP_MAC_ADMIN, current_cred()))
+ return 0;
+
+ return -EPERM;
+}
+
+#endif /* CONFIG_IO_URING */
+
struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
.lbs_cred = sizeof(struct task_smack),
.lbs_file = sizeof(struct smack_known *),
@@ -4843,6 +4885,10 @@ static struct security_hook_list smack_hooks[]
__lsm_ro_after_init = {
LSM_HOOK_INIT(inode_copy_up, smack_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, smack_inode_copy_up_xattr),
LSM_HOOK_INIT(dentry_create_files_as, smack_dentry_create_files_as),
+#ifdef CONFIG_IO_URING
+ LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
+ LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
+#endif
};
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 1/8] audit: prepare audit_context for use in calling contexts beyond syscalls
This patch cleans up some of our audit_context handling by
abstracting out the reset and return code fixup handling to dedicated
functions. Not only does this help make things easier to read and
inspect, it allows for easier reuse by future patches. We also
convert the simple audit_context->in_syscall flag into an enum which
can be used to by future patches to indicate a calling context other
than the syscall context.
Thanks to Richard Guy Briggs for review and feedback.
Acked-by: Richard Guy Briggs
Signed-off-by: Paul Moore
---
v3:
- removed work-in-progress warning from the description
v2:
- no change
v1:
- initial draft
---
kernel/audit.h |5 +
kernel/auditsc.c | 256 ++
2 files changed, 167 insertions(+), 94 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index d6a2c899a8db..13abc48de0bd 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -100,7 +100,10 @@ struct audit_proctitle {
/* The per-task audit context. */
struct audit_context {
int dummy; /* must be the first element */
- int in_syscall; /* 1 if task is in a syscall */
+ enum {
+ AUDIT_CTX_UNUSED, /* audit_context is currently unused */
+ AUDIT_CTX_SYSCALL, /* in use by syscall */
+ } context;
enum audit_statestate, current_state;
unsigned intserial; /* serial number for record */
int major; /* syscall number */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8dd73a64f921..c0383d554e61 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -915,10 +915,80 @@ static inline void audit_free_aux(struct audit_context
*context)
context->aux = aux->next;
kfree(aux);
}
+ context->aux = NULL;
while ((aux = context->aux_pids)) {
context->aux_pids = aux->next;
kfree(aux);
}
+ context->aux_pids = NULL;
+}
+
+/**
+ * audit_reset_context - reset a audit_context structure
+ * @ctx: the audit_context to reset
+ *
+ * All fields in the audit_context will be reset to an initial state, all
+ * references held by fields will be dropped, and private memory will be
+ * released. When this function returns the audit_context will be suitable
+ * for reuse, so long as the passed context is not NULL or a dummy context.
+ */
+static void audit_reset_context(struct audit_context *ctx)
+{
+ if (!ctx)
+ return;
+
+ /* if ctx is non-null, reset the "ctx->state" regardless */
+ ctx->context = AUDIT_CTX_UNUSED;
+ if (ctx->dummy)
+ return;
+
+ /*
+* NOTE: It shouldn't matter in what order we release the fields, so
+* release them in the order in which they appear in the struct;
+* this gives us some hope of quickly making sure we are
+* resetting the audit_context properly.
+*
+* Other things worth mentioning:
+* - we don't reset "dummy"
+* - we don't reset "state", we do reset "current_state"
+* - we preserver "filterkey" if "state" is AUDIT_STATE_RECORD
+* - much of this is likely overkill, but play it safe for now
+* - we really need to work on improving the audit_context struct
+*/
+
+ ctx->current_state = ctx->state;
+ ctx->serial = 0;
+ ctx->major = 0;
+ ctx->ctime = (struct timespec64){ .tv_sec = 0, .tv_nsec = 0 };
+ memset(ctx->argv, 0, sizeof(ctx->argv));
+ ctx->return_code = 0;
+ ctx->prio = (ctx->state == AUDIT_STATE_RECORD ? ~0ULL : 0);
+ ctx->return_valid = AUDITSC_INVALID;
+ audit_free_names(ctx);
+ if (ctx->state != AUDIT_STATE_RECORD) {
+ kfree(ctx->filterkey);
+ ctx->filterkey = NULL;
+ }
+ audit_free_aux(ctx);
+ kfree(ctx->sockaddr);
+ ctx->sockaddr = NULL;
+ ctx->sockaddr_len = 0;
+ ctx->pid = ctx->ppid = 0;
+ ctx->uid = ctx->euid = ctx->suid = ctx->fsuid = KUIDT_INIT(0);
+ ctx->gid = ctx->egid = ctx->sgid = ctx->fsgid = KGIDT_INIT(0);
+ ctx->personality = 0;
+ ctx->arch = 0;
+ ctx->target_pid = 0;
+ ctx->target_auid = ctx->target_uid = KUIDT_INIT(0);
+ ctx->target_sessionid = 0;
+ ctx->target_sid = 0;
+ ctx->target_comm[0] = '\0';
+ unroll_tree_refs(ctx, NULL, 0);
+ WARN_ON(!list_empty(>killed_trees));
+ ctx->type = 0;
+ audit_free_module(ctx);
+ ctx->fds[0] = -1;
+ audit_proctitle_free(ctx);
}
static inline struct audit_context *audit_alloc_context(enum audit_state state)
[PATCH v3 0/8] Add LSM access controls and auditing to io_uring
As promised, here is revision #3 of the io_uring/LSM/audit patchset. The changes from revision #2 are minimal and noted in the individual patches; they are mostly focused on removing debug/dev code and scary "BEWARE, DEVELOPMENT PATCH!" language from the commit descriptions. With plenty of good discussion happening on the initial RFC posting, and the second revision incorporating all the feedback garnering no objections, I plan to merge this patchset into the selinux/next tree later this week. Jens, Pavel, it would nice if I could get your ACK on the io_uring patches before I merge them. For those of you who may be seeing this for the first time, the second RFC revision of the patchset can be found in the archives at the link below: https://lore.kernel.org/linux-security-module/162871480969.63873.9434591871437326374.stgit@olly/ ... and the initial draft RFC can be found here: https://lore.kernel.org/linux-security-module/162163367115.8379.8459012634106035341.stgit@sifl/ Those who would prefer to fetch these patches directly from git can do so using the tree/branch below: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git (checkout branch "working-io_uring") -Paul --- Casey Schaufler (1): Smack: Brutalist io_uring support with debug Paul Moore (7): audit: prepare audit_context for use in calling contexts beyond syscalls audit,io_uring,io-wq: add some basic audit support to io_uring audit: add filtering for io_uring records fs: add anon_inode_getfile_secure() similar to anon_inode_getfd_secure() io_uring: convert io_uring to the secure anon inode interface lsm,io_uring: add LSM hooks to io_uring selinux: add support for the io_uring access controls fs/anon_inodes.c| 29 ++ fs/io-wq.c | 4 + fs/io_uring.c | 69 +++- include/linux/anon_inodes.h | 4 + include/linux/audit.h | 26 ++ include/linux/lsm_hook_defs.h | 5 + include/linux/lsm_hooks.h | 13 + include/linux/security.h| 16 + include/uapi/linux/audit.h | 4 +- kernel/audit.h | 7 +- kernel/audit_tree.c | 3 +- kernel/audit_watch.c| 3 +- kernel/auditfilter.c| 15 +- kernel/auditsc.c| 477 ++-- security/security.c | 12 + security/selinux/hooks.c| 34 ++ security/selinux/include/classmap.h | 2 + security/smack/smack_lsm.c | 46 +++ 18 files changed, 654 insertions(+), 115 deletions(-) -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 4/8] fs: add anon_inode_getfile_secure() similar to anon_inode_getfd_secure()
Extending the secure anonymous inode support to other subsystems
requires that we have a secure anon_inode_getfile() variant in
addition to the existing secure anon_inode_getfd() variant.
Thankfully we can reuse the existing __anon_inode_getfile() function
and just wrap it with the proper arguments.
Acked-by: Mickaël Salaün
Signed-off-by: Paul Moore
---
v3:
- no change
v2:
- no change
v1:
- initial draft
---
fs/anon_inodes.c| 29 +
include/linux/anon_inodes.h |4
2 files changed, 33 insertions(+)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index a280156138ed..e0c3e33c4177 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -148,6 +148,35 @@ struct file *anon_inode_getfile(const char *name,
}
EXPORT_SYMBOL_GPL(anon_inode_getfile);
+/**
+ * anon_inode_getfile_secure - Like anon_inode_getfile(), but creates a new
+ * !S_PRIVATE anon inode rather than reuse the
+ * singleton anon inode and calls the
+ * inode_init_security_anon() LSM hook. This
+ * allows for both the inode to have its own
+ * security context and for the LSM to enforce
+ * policy on the inode's creation.
+ *
+ * @name:[in]name of the "class" of the new file
+ * @fops:[in]file operations for the new file
+ * @priv:[in]private data for the new file (will be file's
private_data)
+ * @flags: [in]flags
+ * @context_inode:
+ * [in]the logical relationship with the new inode (optional)
+ *
+ * The LSM may use @context_inode in inode_init_security_anon(), but a
+ * reference to it is not held. Returns the newly created file* or an error
+ * pointer. See the anon_inode_getfile() documentation for more information.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return __anon_inode_getfile(name, fops, priv, flags,
+ context_inode, true);
+}
+
static int __anon_inode_getfd(const char *name,
const struct file_operations *fops,
void *priv, int flags,
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index 71881a2b6f78..5deaddbd7927 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -15,6 +15,10 @@ struct inode;
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
int anon_inode_getfd_secure(const char *name,
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 5/8] io_uring: convert io_uring to the secure anon inode interface
Converting io_uring's anonymous inode to the secure anon inode API
enables LSMs to enforce policy on the io_uring anonymous inodes if
they chose to do so. This is an important first step towards
providing the necessary mechanisms so that LSMs can apply security
policy to io_uring operations.
Signed-off-by: Paul Moore
---
v3:
- no change
v2:
- no change
v1:
- initial draft
---
fs/io_uring.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 388754b24785..56cc9aba0d01 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -10155,8 +10155,8 @@ static struct file *io_uring_get_file(struct
io_ring_ctx *ctx)
return ERR_PTR(ret);
#endif
- file = anon_inode_getfile("[io_uring]", _uring_fops, ctx,
- O_RDWR | O_CLOEXEC);
+ file = anon_inode_getfile_secure("[io_uring]", _uring_fops, ctx,
+O_RDWR | O_CLOEXEC, NULL);
#if defined(CONFIG_UNIX)
if (IS_ERR(file)) {
sock_release(ctx->ring_sock);
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v4 3/8] audit: add filtering for io_uring records
This patch adds basic audit io_uring filtering, using as much of the
existing audit filtering infrastructure as possible. In order to do
this we reuse the audit filter rule's syscall mask for the io_uring
operation and we create a new filter for io_uring operations as
AUDIT_FILTER_URING_EXIT/audit_filter_list[7].
Thanks to Richard Guy Briggs for his review, feedback, and work on
the corresponding audit userspace changes.
Signed-off-by: Paul Moore
---
v4:
- no change
v3:
- removed work-in-progress warning from the description
v2:
- incorporate feedback from Richard
v1:
- initial draft
---
include/uapi/linux/audit.h |3 +-
kernel/audit_tree.c|3 +-
kernel/audit_watch.c |3 +-
kernel/auditfilter.c | 15 +--
kernel/auditsc.c | 61 ++--
5 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index a1997697c8b1..ecf1edd2affa 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -167,8 +167,9 @@
#define AUDIT_FILTER_EXCLUDE 0x05/* Apply rule before record creation */
#define AUDIT_FILTER_TYPE AUDIT_FILTER_EXCLUDE /* obsolete misleading
naming */
#define AUDIT_FILTER_FS0x06/* Apply rule at
__audit_inode_child */
+#define AUDIT_FILTER_URING_EXIT0x07/* Apply rule at io_uring op
exit */
-#define AUDIT_NR_FILTERS 7
+#define AUDIT_NR_FILTERS 8
#define AUDIT_FILTER_PREPEND 0x10/* Prepend to front of list */
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 2cd7b5694422..338c53a961c5 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -726,7 +726,8 @@ int audit_make_tree(struct audit_krule *rule, char
*pathname, u32 op)
{
if (pathname[0] != '/' ||
- rule->listnr != AUDIT_FILTER_EXIT ||
+ (rule->listnr != AUDIT_FILTER_EXIT &&
+rule->listnr != AUDIT_FILTER_URING_EXIT) ||
op != Audit_equal ||
rule->inode_f || rule->watch || rule->tree)
return -EINVAL;
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 2acf7ca49154..698b62b4a2ec 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -183,7 +183,8 @@ int audit_to_watch(struct audit_krule *krule, char *path,
int len, u32 op)
return -EOPNOTSUPP;
if (path[0] != '/' || path[len-1] == '/' ||
- krule->listnr != AUDIT_FILTER_EXIT ||
+ (krule->listnr != AUDIT_FILTER_EXIT &&
+krule->listnr != AUDIT_FILTER_URING_EXIT) ||
op != Audit_equal ||
krule->inode_f || krule->watch || krule->tree)
return -EINVAL;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index db2c6b59dfc3..d75acb014ccd 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -44,7 +44,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
LIST_HEAD_INIT(audit_filter_list[6]),
-#if AUDIT_NR_FILTERS != 7
+ LIST_HEAD_INIT(audit_filter_list[7]),
+#if AUDIT_NR_FILTERS != 8
#error Fix audit_filter_list initialiser
#endif
};
@@ -56,6 +57,7 @@ static struct list_head audit_rules_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
LIST_HEAD_INIT(audit_rules_list[6]),
+ LIST_HEAD_INIT(audit_rules_list[7]),
};
DEFINE_MUTEX(audit_filter_mutex);
@@ -151,7 +153,8 @@ char *audit_unpack_string(void **bufp, size_t *remain,
size_t len)
static inline int audit_to_inode(struct audit_krule *krule,
struct audit_field *f)
{
- if (krule->listnr != AUDIT_FILTER_EXIT ||
+ if ((krule->listnr != AUDIT_FILTER_EXIT &&
+krule->listnr != AUDIT_FILTER_URING_EXIT) ||
krule->inode_f || krule->watch || krule->tree ||
(f->op != Audit_equal && f->op != Audit_not_equal))
return -EINVAL;
@@ -248,6 +251,7 @@ static inline struct audit_entry
*audit_to_entry_common(struct audit_rule_data *
pr_err("AUDIT_FILTER_ENTRY is deprecated\n");
goto exit_err;
case AUDIT_FILTER_EXIT:
+ case AUDIT_FILTER_URING_EXIT:
case AUDIT_FILTER_TASK:
#endif
case AUDIT_FILTER_USER:
@@ -332,6 +336,10 @@ static int audit_field_valid(struct audit_entry *entry,
struct audit_field *f)
if (entry->rule.listnr != AUDIT_FILTER_FS)
return -EINVAL;
break;
+ case AUDIT_PERM:
+ if (entry->rule.listnr == AUDIT_FILTER_URING_EXIT)
+ return -EINVAL;
+ break;
}
switch (entry->rule.listnr) {
@@ -980,7 +98
[PATCH v4 2/8] audit, io_uring, io-wq: add some basic audit support to io_uring
This patch adds basic auditing to io_uring operations, regardless of
their context. This is accomplished by allocating audit_context
structures for the io-wq worker and io_uring SQPOLL kernel threads
as well as explicitly auditing the io_uring operations in
io_issue_sqe(). Individual io_uring operations can bypass auditing
through the "audit_skip" field in the struct io_op_def definition for
the operation; although great care must be taken so that security
relevant io_uring operations do not bypass auditing; please contact
the audit mailing list (see the MAINTAINERS file) with any questions.
The io_uring operations are audited using a new AUDIT_URINGOP record,
an example is shown below:
type=UNKNOWN[1336] msg=audit(1630523381.288:260):
uring_op=19 success=yes exit=0 items=0 ppid=853 pid=1204
uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
key=(null)
AUID="root" UID="root" GID="root" EUID="root" SUID="root"
FSUID="root" EGID="root" SGID="root" FSGID="root"
Thanks to Richard Guy Briggs for review and feedback.
Signed-off-by: Paul Moore
---
v4:
- removed some work-in-progress comments
- removed the auid logging in audit_log_uring()
v3:
- removed work-in-progress warning from the description
v2:
- added dummy funcs for audit_uring_{entry,exit}()
- replaced opcode checks in io_issue_sqe() with audit_skip checks
- moved fastpath checks into audit_uring_{entry,exit}()
- audit_log_uring() uses GFP_ATOMIC
- don't record the arch in __audit_uring_entry()
v1:
- initial draft
---
fs/io-wq.c |4 +
fs/io_uring.c | 55 +--
include/linux/audit.h | 26 +++
include/uapi/linux/audit.h |1
kernel/audit.h |2 +
kernel/auditsc.c | 166
6 files changed, 248 insertions(+), 6 deletions(-)
diff --git a/fs/io-wq.c b/fs/io-wq.c
index 6c55362c1f99..dac5c5961c9d 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include "io-wq.h"
@@ -562,6 +563,8 @@ static int io_wqe_worker(void *data)
snprintf(buf, sizeof(buf), "iou-wrk-%d", wq->task->pid);
set_task_comm(current, buf);
+ audit_alloc_kernel(current);
+
while (!test_bit(IO_WQ_BIT_EXIT, >state)) {
long ret;
@@ -601,6 +604,7 @@ static int io_wqe_worker(void *data)
io_worker_handle_work(worker);
}
+ audit_free(current);
io_worker_exit(worker);
return 0;
}
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 16fb7436043c..388754b24785 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -79,6 +79,7 @@
#include
#include
#include
+#include
#define CREATE_TRACE_POINTS
#include
@@ -917,6 +918,8 @@ struct io_op_def {
unsignedneeds_async_setup : 1;
/* should block plug */
unsignedplug : 1;
+ /* skip auditing */
+ unsignedaudit_skip : 1;
/* size of async data needed, if any */
unsigned short async_size;
};
@@ -930,6 +933,7 @@ static const struct io_op_def io_op_defs[] = {
.buffer_select = 1,
.needs_async_setup = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_WRITEV] = {
@@ -939,16 +943,19 @@ static const struct io_op_def io_op_defs[] = {
.pollout= 1,
.needs_async_setup = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_FSYNC] = {
.needs_file = 1,
+ .audit_skip = 1,
},
[IORING_OP_READ_FIXED] = {
.needs_file = 1,
.unbound_nonreg_file= 1,
.pollin = 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_WRITE_FIXED] = {
@@ -957,15 +964,20 @@ static const struct io_op_def io_op_defs[] = {
.unbound_nonreg_file= 1,
.pollout= 1,
.plug = 1,
+ .audit_skip = 1,
.async_size = sizeof(struct io_async_rw),
},
[IORING_OP_POLL_ADD] = {
.needs_file = 1,
.unbound_nonreg_file= 1,
+
[PATCH v4 1/8] audit: prepare audit_context for use in calling contexts beyond syscalls
This patch cleans up some of our audit_context handling by
abstracting out the reset and return code fixup handling to dedicated
functions. Not only does this help make things easier to read and
inspect, it allows for easier reuse by future patches. We also
convert the simple audit_context->in_syscall flag into an enum which
can be used to by future patches to indicate a calling context other
than the syscall context.
Thanks to Richard Guy Briggs for review and feedback.
Acked-by: Richard Guy Briggs
Signed-off-by: Paul Moore
---
v4:
- fix some spelling errors in the comments
v3:
- removed work-in-progress warning from the description
v2:
- no change
v1:
- initial draft
---
kernel/audit.h |5 +
kernel/auditsc.c | 256 ++
2 files changed, 167 insertions(+), 94 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index d6a2c899a8db..13abc48de0bd 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -100,7 +100,10 @@ struct audit_proctitle {
/* The per-task audit context. */
struct audit_context {
int dummy; /* must be the first element */
- int in_syscall; /* 1 if task is in a syscall */
+ enum {
+ AUDIT_CTX_UNUSED, /* audit_context is currently unused */
+ AUDIT_CTX_SYSCALL, /* in use by syscall */
+ } context;
enum audit_statestate, current_state;
unsigned intserial; /* serial number for record */
int major; /* syscall number */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8dd73a64f921..f3d309b05c2d 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -915,10 +915,80 @@ static inline void audit_free_aux(struct audit_context
*context)
context->aux = aux->next;
kfree(aux);
}
+ context->aux = NULL;
while ((aux = context->aux_pids)) {
context->aux_pids = aux->next;
kfree(aux);
}
+ context->aux_pids = NULL;
+}
+
+/**
+ * audit_reset_context - reset a audit_context structure
+ * @ctx: the audit_context to reset
+ *
+ * All fields in the audit_context will be reset to an initial state, all
+ * references held by fields will be dropped, and private memory will be
+ * released. When this function returns the audit_context will be suitable
+ * for reuse, so long as the passed context is not NULL or a dummy context.
+ */
+static void audit_reset_context(struct audit_context *ctx)
+{
+ if (!ctx)
+ return;
+
+ /* if ctx is non-null, reset the "ctx->state" regardless */
+ ctx->context = AUDIT_CTX_UNUSED;
+ if (ctx->dummy)
+ return;
+
+ /*
+* NOTE: It shouldn't matter in what order we release the fields, so
+* release them in the order in which they appear in the struct;
+* this gives us some hope of quickly making sure we are
+* resetting the audit_context properly.
+*
+* Other things worth mentioning:
+* - we don't reset "dummy"
+* - we don't reset "state", we do reset "current_state"
+* - we preserve "filterkey" if "state" is AUDIT_STATE_RECORD
+* - much of this is likely overkill, but play it safe for now
+* - we really need to work on improving the audit_context struct
+*/
+
+ ctx->current_state = ctx->state;
+ ctx->serial = 0;
+ ctx->major = 0;
+ ctx->ctime = (struct timespec64){ .tv_sec = 0, .tv_nsec = 0 };
+ memset(ctx->argv, 0, sizeof(ctx->argv));
+ ctx->return_code = 0;
+ ctx->prio = (ctx->state == AUDIT_STATE_RECORD ? ~0ULL : 0);
+ ctx->return_valid = AUDITSC_INVALID;
+ audit_free_names(ctx);
+ if (ctx->state != AUDIT_STATE_RECORD) {
+ kfree(ctx->filterkey);
+ ctx->filterkey = NULL;
+ }
+ audit_free_aux(ctx);
+ kfree(ctx->sockaddr);
+ ctx->sockaddr = NULL;
+ ctx->sockaddr_len = 0;
+ ctx->pid = ctx->ppid = 0;
+ ctx->uid = ctx->euid = ctx->suid = ctx->fsuid = KUIDT_INIT(0);
+ ctx->gid = ctx->egid = ctx->sgid = ctx->fsgid = KGIDT_INIT(0);
+ ctx->personality = 0;
+ ctx->arch = 0;
+ ctx->target_pid = 0;
+ ctx->target_auid = ctx->target_uid = KUIDT_INIT(0);
+ ctx->target_sessionid = 0;
+ ctx->target_sid = 0;
+ ctx->target_comm[0] = '\0';
+ unroll_tree_refs(ctx, NULL, 0);
+ WARN_ON(!list_empty(>killed_trees));
+ ctx->type = 0;
+ audit_free_module(ctx);
+ ctx->fds[0] = -1;
+ audit_proctitle_free(ctx);
}
static inline struct audit_c
[PATCH v4 0/8] Add LSM access controls and auditing to io_uring
A quick update to the v3 patchset with a small change to the audit record format (remove the audit login ID on io_uring records) and a subject line fix on the Smack patch. I also caught a few minor things in the code comments and fixed those up. All told, nothing significant but I really dislike merging patches that haven't hit the list so here ya go ... As a reminder, I'm planning to merge these in the selinux/next tree later this week and it would be *really* nice to get some ACKs from the io_uring folks; this patchset is implementing the ideas we all agreed to back in the v1 patchset so there shouldn't be anything surprising in here. For reference the v3 patchset can be found here: https://lore.kernel.org/linux-security-module/163159032713.470089.11728103630366176255.stgit@olly/T/#t Those who would prefer to fetch these patches directly from git can do so using the tree/branch below: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git (checkout branch "working-io_uring") --- Casey Schaufler (1): Smack: Brutalist io_uring support Paul Moore (7): audit: prepare audit_context for use in calling contexts beyond syscalls audit,io_uring,io-wq: add some basic audit support to io_uring audit: add filtering for io_uring records fs: add anon_inode_getfile_secure() similar to anon_inode_getfd_secure() io_uring: convert io_uring to the secure anon inode interface lsm,io_uring: add LSM hooks to io_uring selinux: add support for the io_uring access controls fs/anon_inodes.c| 29 ++ fs/io-wq.c | 4 + fs/io_uring.c | 69 +++- include/linux/anon_inodes.h | 4 + include/linux/audit.h | 26 ++ include/linux/lsm_hook_defs.h | 5 + include/linux/lsm_hooks.h | 13 + include/linux/security.h| 16 + include/uapi/linux/audit.h | 4 +- kernel/audit.h | 7 +- kernel/audit_tree.c | 3 +- kernel/audit_watch.c| 3 +- kernel/auditfilter.c| 15 +- kernel/auditsc.c| 469 ++-- security/security.c | 12 + security/selinux/hooks.c| 34 ++ security/selinux/include/classmap.h | 2 + security/smack/smack_lsm.c | 46 +++ 18 files changed, 646 insertions(+), 115 deletions(-) -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v4 7/8] selinux: add support for the io_uring access controls
This patch implements two new io_uring access controls, specifically
support for controlling the io_uring "personalities" and
IORING_SETUP_SQPOLL. Controlling the sharing of io_urings themselves
is handled via the normal file/inode labeling and sharing mechanisms.
The io_uring { override_creds } permission restricts which domains
the subject domain can use to override it's own credentials.
Granting a domain the io_uring { override_creds } permission allows
it to impersonate another domain in io_uring operations.
The io_uring { sqpoll } permission restricts which domains can create
asynchronous io_uring polling threads. This is important from a
security perspective as operations queued by this asynchronous thread
inherit the credentials of the thread creator by default; if an
io_uring is shared across process/domain boundaries this could result
in one domain impersonating another. Controlling the creation of
sqpoll threads, and the sharing of io_urings across processes, allow
policy authors to restrict the ability of one domain to impersonate
another via io_uring.
As a quick summary, this patch adds a new object class with two
permissions:
io_uring { override_creds sqpoll }
These permissions can be seen in the two simple policy statements
below:
allow domA_t domB_t : io_uring { override_creds };
allow domA_t self : io_uring { sqpoll };
Signed-off-by: Paul Moore
---
v4:
- no change
v3:
- removed work-in-progress warning from the description
v2:
- made the selinux_uring_* funcs static
- removed the debugging code
v1:
- initial draft
---
security/selinux/hooks.c| 34 ++
security/selinux/include/classmap.h |2 ++
2 files changed, 36 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6517f221d52c..012e8504ed9e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7111,6 +7111,35 @@ static int selinux_perf_event_write(struct perf_event
*event)
}
#endif
+#ifdef CONFIG_IO_URING
+/**
+ * selinux_uring_override_creds - check the requested cred override
+ * @new: the target creds
+ *
+ * Check to see if the current task is allowed to override it's credentials
+ * to service an io_uring operation.
+ */
+static int selinux_uring_override_creds(const struct cred *new)
+{
+ return avc_has_perm(_state, current_sid(), cred_sid(new),
+ SECCLASS_IO_URING, IO_URING__OVERRIDE_CREDS, NULL);
+}
+
+/**
+ * selinux_uring_sqpoll - check if a io_uring polling thread can be created
+ *
+ * Check to see if the current task is allowed to create a new io_uring
+ * kernel polling thread.
+ */
+static int selinux_uring_sqpoll(void)
+{
+ int sid = current_sid();
+
+ return avc_has_perm(_state, sid, sid,
+ SECCLASS_IO_URING, IO_URING__SQPOLL, NULL);
+}
+#endif /* CONFIG_IO_URING */
+
/*
* IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
* 1. any hooks that don't belong to (2.) or (3.) below,
@@ -7349,6 +7378,11 @@ static struct security_hook_list selinux_hooks[]
__lsm_ro_after_init = {
LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write),
#endif
+#ifdef CONFIG_IO_URING
+ LSM_HOOK_INIT(uring_override_creds, selinux_uring_override_creds),
+ LSM_HOOK_INIT(uring_sqpoll, selinux_uring_sqpoll),
+#endif
+
LSM_HOOK_INIT(locked_down, selinux_lockdown),
/*
diff --git a/security/selinux/include/classmap.h
b/security/selinux/include/classmap.h
index 084757ff4390..698ccfdaf82d 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -254,6 +254,8 @@ struct security_class_mapping secclass_map[] = {
{ "integrity", "confidentiality", NULL } },
{ "anon_inode",
{ COMMON_FILE_PERMS, NULL } },
+ { "io_uring",
+ { "override_creds", "sqpoll", NULL } },
{ NULL }
};
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v4 6/8] lsm,io_uring: add LSM hooks to io_uring
A full expalantion of io_uring is beyond the scope of this commit
description, but in summary it is an asynchronous I/O mechanism
which allows for I/O requests and the resulting data to be queued
in memory mapped "rings" which are shared between the kernel and
userspace. Optionally, io_uring offers the ability for applications
to spawn kernel threads to dequeue I/O requests from the ring and
submit the requests in the kernel, helping to minimize the syscall
overhead. Rings are accessed in userspace by memory mapping a file
descriptor provided by the io_uring_setup(2), and can be shared
between applications as one might do with any open file descriptor.
Finally, process credentials can be registered with a given ring
and any process with access to that ring can submit I/O requests
using any of the registered credentials.
While the io_uring functionality is widely recognized as offering a
vastly improved, and high performing asynchronous I/O mechanism, its
ability to allow processes to submit I/O requests with credentials
other than its own presents a challenge to LSMs. When a process
creates a new io_uring ring the ring's credentials are inhertied
from the calling process; if this ring is shared with another
process operating with different credentials there is the potential
to bypass the LSMs security policy. Similarly, registering
credentials with a given ring allows any process with access to that
ring to submit I/O requests with those credentials.
In an effort to allow LSMs to apply security policy to io_uring I/O
operations, this patch adds two new LSM hooks. These hooks, in
conjunction with the LSM anonymous inode support previously
submitted, allow an LSM to apply access control policy to the
sharing of io_uring rings as well as any io_uring credential changes
requested by a process.
The new LSM hooks are described below:
* int security_uring_override_creds(cred)
Controls if the current task, executing an io_uring operation,
is allowed to override it's credentials with @cred. In cases
where the current task is a user application, the current
credentials will be those of the user application. In cases
where the current task is a kernel thread servicing io_uring
requests the current credentials will be those of the io_uring
ring (inherited from the process that created the ring).
* int security_uring_sqpoll(void)
Controls if the current task is allowed to create an io_uring
polling thread (IORING_SETUP_SQPOLL). Without a SQPOLL thread
in the kernel processes must submit I/O requests via
io_uring_enter(2) which allows us to compare any requested
credential changes against the application making the request.
With a SQPOLL thread, we can no longer compare requested
credential changes against the application making the request,
the comparison is made against the ring's credentials.
Signed-off-by: Paul Moore
---
v4:
- no change
v3:
- removed work-in-progress warning from the description
v2:
- no change
v1:
- initial draft
---
fs/io_uring.c | 10 ++
include/linux/lsm_hook_defs.h |5 +
include/linux/lsm_hooks.h | 13 +
include/linux/security.h | 16
security/security.c | 12
5 files changed, 56 insertions(+)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 56cc9aba0d01..f89d00af3a67 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -80,6 +80,7 @@
#include
#include
#include
+#include
#define CREATE_TRACE_POINTS
#include
@@ -7070,6 +7071,11 @@ static int io_init_req(struct io_ring_ctx *ctx, struct
io_kiocb *req,
if (!req->creds)
return -EINVAL;
get_cred(req->creds);
+ ret = security_uring_override_creds(req->creds);
+ if (ret) {
+ put_cred(req->creds);
+ return ret;
+ }
req->flags |= REQ_F_CREDS;
}
state = >submit_state;
@@ -8566,6 +8572,10 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
struct io_sq_data *sqd;
bool attached;
+ ret = security_uring_sqpoll();
+ if (ret)
+ return ret;
+
sqd = io_get_sq_data(p, );
if (IS_ERR(sqd)) {
ret = PTR_ERR(sqd);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2adeea44c0d5..b3c525353769 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -402,3 +402,8 @@ LSM_HOOK(void, LSM_RET_VOID, perf_event_free, struct
perf_event *event)
LSM_HOOK(int, 0, perf_event_read, struct perf_event *event)
LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
#endif /* CONFIG_PERF_EVENTS */
+
+#ifdef CONFIG_IO_URING
+LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
+LSM_HOOK(int, 0,
[PATCH v4 5/8] io_uring: convert io_uring to the secure anon inode interface
Converting io_uring's anonymous inode to the secure anon inode API
enables LSMs to enforce policy on the io_uring anonymous inodes if
they chose to do so. This is an important first step towards
providing the necessary mechanisms so that LSMs can apply security
policy to io_uring operations.
Signed-off-by: Paul Moore
---
v4:
- no change
v3:
- no change
v2:
- no change
v1:
- initial draft
---
fs/io_uring.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 388754b24785..56cc9aba0d01 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -10155,8 +10155,8 @@ static struct file *io_uring_get_file(struct
io_ring_ctx *ctx)
return ERR_PTR(ret);
#endif
- file = anon_inode_getfile("[io_uring]", _uring_fops, ctx,
- O_RDWR | O_CLOEXEC);
+ file = anon_inode_getfile_secure("[io_uring]", _uring_fops, ctx,
+O_RDWR | O_CLOEXEC, NULL);
#if defined(CONFIG_UNIX)
if (IS_ERR(file)) {
sock_release(ctx->ring_sock);
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v4 8/8] Smack: Brutalist io_uring support
From: Casey Schaufler
Add Smack privilege checks for io_uring. Use CAP_MAC_OVERRIDE
for the override_creds case and CAP_MAC_ADMIN for creating a
polling thread. These choices are based on conjecture regarding
the intent of the surrounding code.
Signed-off-by: Casey Schaufler
[PM: make the smack_uring_* funcs static, remove debug code]
Signed-off-by: Paul Moore
---
v4:
- updated subject line
v3:
- removed debug code
v2:
- made the smack_uring_* funcs static
v1:
- initial draft
---
security/smack/smack_lsm.c | 46
1 file changed, 46 insertions(+)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index cacbe7518519..f90ab1efeb6d 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4691,6 +4691,48 @@ static int smack_dentry_create_files_as(struct dentry
*dentry, int mode,
return 0;
}
+#ifdef CONFIG_IO_URING
+/**
+ * smack_uring_override_creds - Is io_uring cred override allowed?
+ * @new: the target creds
+ *
+ * Check to see if the current task is allowed to override it's credentials
+ * to service an io_uring operation.
+ */
+static int smack_uring_override_creds(const struct cred *new)
+{
+ struct task_smack *tsp = smack_cred(current_cred());
+ struct task_smack *nsp = smack_cred(new);
+
+ /*
+* Allow the degenerate case where the new Smack value is
+* the same as the current Smack value.
+*/
+ if (tsp->smk_task == nsp->smk_task)
+ return 0;
+
+ if (smack_privileged_cred(CAP_MAC_OVERRIDE, current_cred()))
+ return 0;
+
+ return -EPERM;
+}
+
+/**
+ * smack_uring_sqpoll - check if a io_uring polling thread can be created
+ *
+ * Check to see if the current task is allowed to create a new io_uring
+ * kernel polling thread.
+ */
+static int smack_uring_sqpoll(void)
+{
+ if (smack_privileged_cred(CAP_MAC_ADMIN, current_cred()))
+ return 0;
+
+ return -EPERM;
+}
+
+#endif /* CONFIG_IO_URING */
+
struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
.lbs_cred = sizeof(struct task_smack),
.lbs_file = sizeof(struct smack_known *),
@@ -4843,6 +4885,10 @@ static struct security_hook_list smack_hooks[]
__lsm_ro_after_init = {
LSM_HOOK_INIT(inode_copy_up, smack_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, smack_inode_copy_up_xattr),
LSM_HOOK_INIT(dentry_create_files_as, smack_dentry_create_files_as),
+#ifdef CONFIG_IO_URING
+ LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds),
+ LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
+#endif
};
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v4 4/8] fs: add anon_inode_getfile_secure() similar to anon_inode_getfd_secure()
Extending the secure anonymous inode support to other subsystems
requires that we have a secure anon_inode_getfile() variant in
addition to the existing secure anon_inode_getfd() variant.
Thankfully we can reuse the existing __anon_inode_getfile() function
and just wrap it with the proper arguments.
Acked-by: Mickaël Salaün
Signed-off-by: Paul Moore
---
v4:
- no change
v3:
- no change
v2:
- no change
v1:
- initial draft
---
fs/anon_inodes.c| 29 +
include/linux/anon_inodes.h |4
2 files changed, 33 insertions(+)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index a280156138ed..e0c3e33c4177 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -148,6 +148,35 @@ struct file *anon_inode_getfile(const char *name,
}
EXPORT_SYMBOL_GPL(anon_inode_getfile);
+/**
+ * anon_inode_getfile_secure - Like anon_inode_getfile(), but creates a new
+ * !S_PRIVATE anon inode rather than reuse the
+ * singleton anon inode and calls the
+ * inode_init_security_anon() LSM hook. This
+ * allows for both the inode to have its own
+ * security context and for the LSM to enforce
+ * policy on the inode's creation.
+ *
+ * @name:[in]name of the "class" of the new file
+ * @fops:[in]file operations for the new file
+ * @priv:[in]private data for the new file (will be file's
private_data)
+ * @flags: [in]flags
+ * @context_inode:
+ * [in]the logical relationship with the new inode (optional)
+ *
+ * The LSM may use @context_inode in inode_init_security_anon(), but a
+ * reference to it is not held. Returns the newly created file* or an error
+ * pointer. See the anon_inode_getfile() documentation for more information.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return __anon_inode_getfile(name, fops, priv, flags,
+ context_inode, true);
+}
+
static int __anon_inode_getfd(const char *name,
const struct file_operations *fops,
void *priv, int flags,
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index 71881a2b6f78..5deaddbd7927 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -15,6 +15,10 @@ struct inode;
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
int anon_inode_getfd_secure(const char *name,
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] audit: Convert to SPDX identifier
On Mon, Sep 13, 2021 at 11:33 PM Cai Huoqing wrote: > > Use SPDX-License-Identifier instead of a verbose license text. > > Signed-off-by: Cai Huoqing > --- > v1->v2: Change recommended token from "GPL-2.0+" to "GPL-2.0-or-later" > > kernel/auditsc.c | 15 +-- > 1 file changed, 1 insertion(+), 14 deletions(-) Merged into audit/next, thanks! -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] lsm_audit: avoid overloading the "key" audit field
On Tue, Sep 14, 2021 at 9:15 AM Ondrej Mosnacek wrote: > > The "key" field is used to associate records with the rule that > triggered them, os it's not a good idea to overload it with an > additional IPC key semantic. Moreover, as the classic "key" field is a > text field, while the IPC key is numeric, AVC records containing the IPC > key info actually confuse audit userspace, which tries to interpret the > number as a hex-encoded string, thus showing garbage for example in the > ausearch "interpret" output mode. > > Hence, change it to "ipc_key" to fix both issues and also make the > meaning of this field more clear. > > Signed-off-by: Ondrej Mosnacek > --- > security/lsm_audit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Seems reasonable to me, I can merge it via the audit/next tree unless James would prefer to take it via the LSM tree. > diff --git a/security/lsm_audit.c b/security/lsm_audit.c > index 5a5016ef43b0..1897cbf6fc69 100644 > --- a/security/lsm_audit.c > +++ b/security/lsm_audit.c > @@ -224,7 +224,7 @@ static void dump_common_audit_data(struct audit_buffer > *ab, > case LSM_AUDIT_DATA_NONE: > return; > case LSM_AUDIT_DATA_IPC: > - audit_log_format(ab, " key=%d ", a->u.ipc_id); > + audit_log_format(ab, " ipc_key=%d ", a->u.ipc_id); > break; > case LSM_AUDIT_DATA_CAP: > audit_log_format(ab, " capability=%d ", a->u.cap); > -- > 2.31.1 -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 8/8] Smack: Brutalist io_uring support with debug
On Tue, Sep 14, 2021 at 10:26 AM Casey Schaufler wrote: > > On 9/13/2021 8:33 PM, Paul Moore wrote: > > From: Casey Schaufler > > > > Add Smack privilege checks for io_uring. Use CAP_MAC_OVERRIDE > > for the override_creds case and CAP_MAC_ADMIN for creating a > > polling thread. These choices are based on conjecture regarding > > the intent of the surrounding code. > > > > Signed-off-by: Casey Schaufler > > [PM: make the smack_uring_* funcs static, remove debug code] > > Signed-off-by: Paul Moore > > You want to change the subject: > > [PATCH v3 8/8] Smack: Brutalist io_uring support with debug > > s/ with debug// Thanks Casey, good catch. I updated my local copy and the selinux/working-io_uring branch but I'll refrain from pushing a new patchset just for this. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Mon, Sep 13, 2021 at 3:23 PM Paul Moore wrote: > On Thu, Sep 9, 2021 at 8:59 PM Richard Guy Briggs wrote: > > On 2021-09-01 15:21, Paul Moore wrote: > > > On Sun, Aug 29, 2021 at 11:18 AM Paul Moore wrote: > > > > On Sat, Aug 28, 2021 at 11:04 AM Richard Guy Briggs > > > > wrote: > > > > > I did set a syscall filter for > > > > > -a exit,always -F arch=b64 -S > > > > > io_uring_enter,io_uring_setup,io_uring_register -F key=iouringsyscall > > > > > and that yielded some records with a couple of orphans that surprised > > > > > me > > > > > a bit. > > > > > > > > Without looking too closely at the log you sent, you can expect URING > > > > records without an associated SYSCALL record when the uring op is > > > > being processed in the io-wq or sqpoll context. In the io-wq case the > > > > processing is happening after the thread finished the syscall but > > > > before the execution context returns to userspace and in the case of > > > > sqpoll the processing is handled by a separate kernel thread with no > > > > association to a process thread. > > > > > > I spent some time this morning/afternoon playing with the io_uring > > > audit filtering capability and with your audit userspace > > > ghau-iouring-filtering.v1.0 branch it appears to work correctly. Yes, > > > the userspace tooling isn't quite 100% yet (e.g. `auditctl -l` doesn't > > > map the io_uring ops correctly), but I know you mentioned you have a > > > number of fixes/improvements still as a work-in-progress there so I'm > > > not too concerned. The important part is that the kernel pieces look > > > to be working correctly. > > > > Ok, I have squashed and pushed the audit userspace support for iouring: > > > > https://github.com/rgbriggs/audit-userspace/commit/e8bd8d2ea8adcaa758024cb9b8fa93895ae35eea > > > > https://github.com/linux-audit/audit-userspace/compare/master...rgbriggs:ghak-iouring-filtering.v2.1 > > There are test rpms for f35 here: > > http://people.redhat.com/~rbriggs/ghak-iouring/git-e8bd8d2-fc35/ > > > > userspace v2 changelog: > > - check for watch before adding perm > > - update manpage to include filesystem filter > > - update support for the uring filter list: doc, -U op, op names > > - add support for the AUDIT_URINGOP record type > > - add uringop support to ausearch > > - add uringop support to aureport > > - lots of bug fixes > > > > "auditctl -a uring,always -S ..." will now throw an error and require > > "-U" instead. > > Thanks Richard. > > FYI, I rebased the io_uring/LSM/audit patchset on top of v5.15-rc1 > today and tested both with your v1.0 and with your v2.1 branch and the > various combinations seemed to work just fine (of course the v2.1 > userspace branch was more polished, less warts, etc.). I'm going to > go over the patch set one more time to make sure everything is still > looking good, write up an updated cover letter, and post a v3 revision > later tonight with the hope of merging it into -next later this week. Best laid plans of mice and men ... It turns out the LSM hook macros are full of warnings-now-errors that should likely be resolved before sending anything LSM related to Linus. I'll post v3 once I fix this, which may not be until tomorrow. (To be clear, the warnings/errors aren't new to this patchset, I'm likely just the first person to notice them.) -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 2/8] audit, io_uring, io-wq: add some basic audit support to io_uring
On Thu, Sep 16, 2021 at 9:33 AM Richard Guy Briggs wrote: > On 2021-09-15 12:49, Paul Moore wrote: > > This patch adds basic auditing to io_uring operations, regardless of > > their context. This is accomplished by allocating audit_context > > structures for the io-wq worker and io_uring SQPOLL kernel threads > > as well as explicitly auditing the io_uring operations in > > io_issue_sqe(). Individual io_uring operations can bypass auditing > > through the "audit_skip" field in the struct io_op_def definition for > > the operation; although great care must be taken so that security > > relevant io_uring operations do not bypass auditing; please contact > > the audit mailing list (see the MAINTAINERS file) with any questions. > > > > The io_uring operations are audited using a new AUDIT_URINGOP record, > > an example is shown below: > > > > type=UNKNOWN[1336] msg=audit(1630523381.288:260): > > uring_op=19 success=yes exit=0 items=0 ppid=853 pid=1204 > > uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > key=(null) > > AUID="root" UID="root" GID="root" EUID="root" SUID="root" > > FSUID="root" EGID="root" SGID="root" FSGID="root" > > > > Thanks to Richard Guy Briggs for review and feedback. > > I share Steve's concerns about the missing auid and ses. The userspace > log interpreter conjured up AUID="root" from the absent auid=. > > Some of the creds are here including ppid, pid, a herd of *id and subj. > *Something* initiated this action and then delegated it to iouring to > carry out. That should be in there somewhere. You had a concern about > shared queues and mis-attribution. All of these creds including auid > and ses should be kept together to get this right. Look, there are a lot of things about io_uring that frustrate me from a security perspective - this is one of them - but I've run out of ways to say it's not possible to reliably capture the audit ID or session ID here. With io_uring it is possible to submit an io_uring operation, and capture the results, by simply reading and writing to a mmap'd buffer. Yes, it would be nice to have that information, but I don't believe there is a practical way to capture it. If you have any suggestions on how to do so, please share, but please make it concrete; hand wavy solutions aren't useful at this stage. As for the userspace mysteriously creating an AUID out of thin air, that was my mistake: I simply removed the "auid=" field from the example and didn't remove the additional fields, e.g. AUID, that auditd appends to the end of the record. I've updated the commit description with a freshly generated record and removed the auditd bonus bits as those probably shouldn't be shown in an example of a kernel generated audit record. I'm not going to repost the patchset just for this small edit to the description, but I have force-pushed the update to the selinux/working-io_uring branch. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 2/8] audit, io_uring, io-wq: add some basic audit support to io_uring
On Thu, Sep 16, 2021 at 10:19 AM Richard Guy Briggs wrote: > On 2021-09-16 10:02, Paul Moore wrote: > > On Thu, Sep 16, 2021 at 9:33 AM Richard Guy Briggs wrote: > > > On 2021-09-15 12:49, Paul Moore wrote: > > > > This patch adds basic auditing to io_uring operations, regardless of > > > > their context. This is accomplished by allocating audit_context > > > > structures for the io-wq worker and io_uring SQPOLL kernel threads > > > > as well as explicitly auditing the io_uring operations in > > > > io_issue_sqe(). Individual io_uring operations can bypass auditing > > > > through the "audit_skip" field in the struct io_op_def definition for > > > > the operation; although great care must be taken so that security > > > > relevant io_uring operations do not bypass auditing; please contact > > > > the audit mailing list (see the MAINTAINERS file) with any questions. > > > > > > > > The io_uring operations are audited using a new AUDIT_URINGOP record, > > > > an example is shown below: > > > > > > > > type=UNKNOWN[1336] msg=audit(1630523381.288:260): > > > > uring_op=19 success=yes exit=0 items=0 ppid=853 pid=1204 > > > > uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 > > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > > key=(null) > > > > AUID="root" UID="root" GID="root" EUID="root" SUID="root" > > > > FSUID="root" EGID="root" SGID="root" FSGID="root" > > > > > > > > Thanks to Richard Guy Briggs for review and feedback. > > > > > > I share Steve's concerns about the missing auid and ses. The userspace > > > log interpreter conjured up AUID="root" from the absent auid=. > > > > > > Some of the creds are here including ppid, pid, a herd of *id and subj. > > > *Something* initiated this action and then delegated it to iouring to > > > carry out. That should be in there somewhere. You had a concern about > > > shared queues and mis-attribution. All of these creds including auid > > > and ses should be kept together to get this right. > > > > Look, there are a lot of things about io_uring that frustrate me from > > a security perspective - this is one of them - but I've run out of > > ways to say it's not possible to reliably capture the audit ID or > > session ID here. With io_uring it is possible to submit an io_uring > > operation, and capture the results, by simply reading and writing to a > > mmap'd buffer. Yes, it would be nice to have that information, but I > > don't believe there is a practical way to capture it. If you have any > > suggestions on how to do so, please share, but please make it > > concrete; hand wavy solutions aren't useful at this stage. > > I was hoping to give a more concrete solution but have other > distractions at the moment. My concern is adding it later once the > message format is committed. We have too many field orderings already. > Recognizing this adds useless characters to the record type at this > time, I'm even thinking auid=? ses=? until a solution can be found. You know my feelings on audit record field orderings, so let's not follow that distraction right now. Regarding proactively inserting a placeholder for auid= and ses=, I'm reasonably convinced it is not practical, and likely not possible, to capture that information for the audit record. As a result, I see no reason to waste the space in the record. However, if you (or anyone else for that matter) can show that we can reliably capture that information then I'm in complete agreement that it should be added. What I'm not going to do is hold this patchset any longer on a vague feeling that it should be possible. On the plus side I'm only merging this into selinux/next, not selinux/stable-5.15, so worst case you have a couple more weeks before things are "set". I realize we all have competing priorities, but as the saying goes, "it's time to put up or shut up" ;) > So you are sure the rest of the creds are correct? Yes, the credentials that are logged in audit_log_uring() are all taken from the currently executing task via a call to current_cred(). Regardless of the io_uring calling context (synchronous, io-wq, sqpoll) the current_cred() call should always return the correct credentials; look at how io_uring manages credentials in io_issue_sqe(). While the audit log treats the audit ID just as if it were another credential on the system, it is definitely not like a normal credential and requires special handling; this is why there was the mixup where I mistakenly left it in the audit record, and one of the reasons why we will not always have a valid audit ID for io_uring operations. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] lsm_audit: avoid overloading the "key" audit field
On Tue, Sep 14, 2021 at 10:49 AM Paul Moore wrote: > > On Tue, Sep 14, 2021 at 9:15 AM Ondrej Mosnacek wrote: > > > > The "key" field is used to associate records with the rule that > > triggered them, os it's not a good idea to overload it with an > > additional IPC key semantic. Moreover, as the classic "key" field is a > > text field, while the IPC key is numeric, AVC records containing the IPC > > key info actually confuse audit userspace, which tries to interpret the > > number as a hex-encoded string, thus showing garbage for example in the > > ausearch "interpret" output mode. > > > > Hence, change it to "ipc_key" to fix both issues and also make the > > meaning of this field more clear. > > > > Signed-off-by: Ondrej Mosnacek > > --- > > security/lsm_audit.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Seems reasonable to me, I can merge it via the audit/next tree unless > James would prefer to take it via the LSM tree. As this is pretty minor and unlikely to conflict with any LSMs, I've gone ahead and merged this into the audit/next tree. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 0/8] Add LSM access controls and auditing to io_uring
On Wed, Sep 15, 2021 at 12:49 PM Paul Moore wrote: > > A quick update to the v3 patchset with a small change to the audit > record format (remove the audit login ID on io_uring records) and > a subject line fix on the Smack patch. I also caught a few minor > things in the code comments and fixed those up. All told, nothing > significant but I really dislike merging patches that haven't hit > the list so here ya go ... > > As a reminder, I'm planning to merge these in the selinux/next tree > later this week and it would be *really* nice to get some ACKs from > the io_uring folks; this patchset is implementing the ideas we all > agreed to back in the v1 patchset so there shouldn't be anything > surprising in here. > > For reference the v3 patchset can be found here: > https://lore.kernel.org/linux-security-module/163159032713.470089.11728103630366176255.stgit@olly/T/#t > > Those who would prefer to fetch these patches directly from git can > do so using the tree/branch below: > git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git > (checkout branch "working-io_uring") > > --- > > Casey Schaufler (1): > Smack: Brutalist io_uring support > > Paul Moore (7): > audit: prepare audit_context for use in calling contexts beyond syscalls > audit,io_uring,io-wq: add some basic audit support to io_uring > audit: add filtering for io_uring records > fs: add anon_inode_getfile_secure() similar to anon_inode_getfd_secure() > io_uring: convert io_uring to the secure anon inode interface > lsm,io_uring: add LSM hooks to io_uring > selinux: add support for the io_uring access controls > > > fs/anon_inodes.c| 29 ++ > fs/io-wq.c | 4 + > fs/io_uring.c | 69 +++- > include/linux/anon_inodes.h | 4 + > include/linux/audit.h | 26 ++ > include/linux/lsm_hook_defs.h | 5 + > include/linux/lsm_hooks.h | 13 + > include/linux/security.h| 16 + > include/uapi/linux/audit.h | 4 +- > kernel/audit.h | 7 +- > kernel/audit_tree.c | 3 +- > kernel/audit_watch.c| 3 +- > kernel/auditfilter.c| 15 +- > kernel/auditsc.c| 469 ++-- > security/security.c | 12 + > security/selinux/hooks.c| 34 ++ > security/selinux/include/classmap.h | 2 + > security/smack/smack_lsm.c | 46 +++ > 18 files changed, 646 insertions(+), 115 deletions(-) With no serious objections or outstanding comments, I just merged these patches into selinux/next. If anyone has any follow-on patches please base them against selinux/next, thanks. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: Fix build failure by renaming struct node to struct audit_node
On Tue, Sep 7, 2021 at 11:45 AM LEROY Christophe
wrote:
> > -Message d'origine-
> > De : Paul Moore
> > On Mon, Sep 6, 2021 at 2:41 AM LEROY Christophe
> > wrote:
> > > Le 03/09/2021 à 19:06, Paul Moore a écrit :
> > > > On Fri, Sep 3, 2021 at 11:48 AM Christophe Leroy
> > > > wrote:
> > > >>
> > > >> struct node defined in kernel/audit_tree.c conflicts with struct
> > > >> node defined in include/linux/node.h
> > > >>
> > > >>CC kernel/audit_tree.o
> > > >> kernel/audit_tree.c:33:9: error: redefinition of 'struct node'
> > > >> 33 | struct node {
> > > >>| ^~~~
> > > >> In file included from ./include/linux/cpu.h:17,
> > > >> from ./include/linux/static_call.h:102,
> > > >> from ./arch/powerpc/include/asm/machdep.h:10,
> > > >> from
> > > >> ./arch/powerpc/include/asm/archrandom.h:7,
> > > >> from ./include/linux/random.h:121,
> > > >> from ./include/linux/net.h:18,
> > > >> from ./include/linux/skbuff.h:26,
> > > >> from kernel/audit.h:11,
> > > >> from kernel/audit_tree.c:2:
> > > >> ./include/linux/node.h:84:8: note: originally defined here
> > > >> 84 | struct node {
> > > >>|^~~~
> > > >> make[2]: *** [kernel/audit_tree.o] Error 1
> > > >>
> > > >> Rename it audit_node.
> > > >>
> > > >> Signed-off-by: Christophe Leroy
> > > >> ---
> > > >> kernel/audit_tree.c | 20 ++--
> > > >> 1 file changed, 10 insertions(+), 10 deletions(-)
> > > >
> > > > That's interesting, I wonder why we didn't see this prior? Also as
> > > > an aside, there are evidently a good handful of symbols named
> > > > "node". In fact I don't see this now in the audit/stable-5.15 or
> > > > Linus' tree as of a right now, both using an allyesconfig:
> > > >
> > > > % git show-ref HEAD
> > > > a9c9a6f741cdaa2fa9ba24a790db8d07295761e3 refs/remotes/linus/HEAD %
> > > > touch kernel/audit_tree.c % make C=1 kernel/
> > > > CALLscripts/checksyscalls.sh
> > > > CALLscripts/atomic/check-atomics.sh
> > > > DESCEND objtool
> > > > CHK kernel/kheaders_data.tar.xz
> > > > CC kernel/audit_tree.o
> > > > CHECK kernel/audit_tree.c
> > > > AR kernel/built-in.a
> > > >
> > > > What tree and config are you using where you see this error?
> > > > Looking at your error, I'm guessing this is limited to ppc builds,
> > > > and if I look at the arch/powerpc/include/asm/machdep.h file in
> > > > Linus tree I don't see a static_call.h include so I'm guessing this
> > > > is a -next tree for ppc? Something else?
> > > >
> > > > Without knowing the context, is adding the static_call.h include in
> > > > arch/powerpc/include/asm/machdep.h intentional or simply a bit of
> > > > include file creep?
> > >
> > > struct machdep_calls in asm/machdep.h is full of function pointers and
> > > I'm working on converting that to static_calls
> > > (https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=260878
> > > =*)
> > >
> > > So yes, adding static_call.h in asm/machdep.h is intentional and the
> > > issue was detected by CI build test
> > > (http://kisskb.ellerman.id.au/kisskb/buildresult/14628100/)
> > >
> > > I submitted this change to you because for me it make sense to not
> > > re-use globably defined struct names in local C files, and anybody may
> > > encounter the problem as soon as linux/node.h gets included directly
> > > or indirectly. But if you prefer I guess the fix may be merged through
> > > powerpc tree as part of this series.
> >
> > Yes, this patch should go in via the audit tree, and while I don't have an
> > objection to the patch, whenever I see a patch to fix an issue that is not
> > visible in
> > Linus' tree or the audit tree it raises some questions. I usually hope to
> > see those
> > questions answered proactively in the cover letter and/or patch description
> > but
> > that wasn't the case here so you get to play a game of 20 questions.
> >
> > Speaking of which, I don't recall seeing an answer to the "where do these
> > include file changes live?" question, is is the ppc -next tree, or are they
> > still
> > unmerged and just on the ppc list?
>
> It is still an RFC in the ppc list.
I just merged this into audit/next but I rewrote chunks of the subject
line and commit description as the build failure isn't yet "real" as
the offending patch is still just a RFC. Hopefully be merging this
patch into audit/next now we'll prevent future problems if/when the
other patch is merged.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: Convert to SPDX identifier
On Sat, Aug 21, 2021 at 10:14 PM Cai Huoqing wrote: > > use SPDX-License-Identifier instead of a verbose license text > > Signed-off-by: Cai Huoqing > --- > kernel/auditsc.c | 15 +-- > 1 file changed, 1 insertion(+), 14 deletions(-) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 8dd73a64f921..969c1613fed9 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -1,3 +1,4 @@ > +// SPDX-License-Identifier: GPL-2.0+ It appears the current recommended token is "GPL-2.0-or-later", please update this patch to use the preferred license identifier. * https://spdx.org/licenses > /* auditsc.c -- System-call auditing support > * Handles all system-call specific auditing features. > * > @@ -6,20 +7,6 @@ > * Copyright (C) 2005, 2006 IBM Corporation > * All Rights Reserved. > * > - * This program is free software; you can redistribute it and/or modify > - * it under the terms of the GNU General Public License as published by > - * the Free Software Foundation; either version 2 of the License, or > - * (at your option) any later version. > - * > - * This program is distributed in the hope that it will be useful, > - * but WITHOUT ANY WARRANTY; without even the implied warranty of > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > - * GNU General Public License for more details. > - * > - * You should have received a copy of the GNU General Public License > - * along with this program; if not, write to the Free Software > - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > - * > * Written by Rickard E. (Rik) Faith > * > * Many of the ideas implemented here are from Stephen C. Tweedie, > -- > 2.25.1 -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Thu, Sep 9, 2021 at 8:59 PM Richard Guy Briggs wrote: > On 2021-09-01 15:21, Paul Moore wrote: > > On Sun, Aug 29, 2021 at 11:18 AM Paul Moore wrote: > > > On Sat, Aug 28, 2021 at 11:04 AM Richard Guy Briggs > > > wrote: > > > > I did set a syscall filter for > > > > -a exit,always -F arch=b64 -S > > > > io_uring_enter,io_uring_setup,io_uring_register -F key=iouringsyscall > > > > and that yielded some records with a couple of orphans that surprised me > > > > a bit. > > > > > > Without looking too closely at the log you sent, you can expect URING > > > records without an associated SYSCALL record when the uring op is > > > being processed in the io-wq or sqpoll context. In the io-wq case the > > > processing is happening after the thread finished the syscall but > > > before the execution context returns to userspace and in the case of > > > sqpoll the processing is handled by a separate kernel thread with no > > > association to a process thread. > > > > I spent some time this morning/afternoon playing with the io_uring > > audit filtering capability and with your audit userspace > > ghau-iouring-filtering.v1.0 branch it appears to work correctly. Yes, > > the userspace tooling isn't quite 100% yet (e.g. `auditctl -l` doesn't > > map the io_uring ops correctly), but I know you mentioned you have a > > number of fixes/improvements still as a work-in-progress there so I'm > > not too concerned. The important part is that the kernel pieces look > > to be working correctly. > > Ok, I have squashed and pushed the audit userspace support for iouring: > > https://github.com/rgbriggs/audit-userspace/commit/e8bd8d2ea8adcaa758024cb9b8fa93895ae35eea > > https://github.com/linux-audit/audit-userspace/compare/master...rgbriggs:ghak-iouring-filtering.v2.1 > There are test rpms for f35 here: > http://people.redhat.com/~rbriggs/ghak-iouring/git-e8bd8d2-fc35/ > > userspace v2 changelog: > - check for watch before adding perm > - update manpage to include filesystem filter > - update support for the uring filter list: doc, -U op, op names > - add support for the AUDIT_URINGOP record type > - add uringop support to ausearch > - add uringop support to aureport > - lots of bug fixes > > "auditctl -a uring,always -S ..." will now throw an error and require > "-U" instead. Thanks Richard. FYI, I rebased the io_uring/LSM/audit patchset on top of v5.15-rc1 today and tested both with your v1.0 and with your v2.1 branch and the various combinations seemed to work just fine (of course the v2.1 userspace branch was more polished, less warts, etc.). I'm going to go over the patch set one more time to make sure everything is still looking good, write up an updated cover letter, and post a v3 revision later tonight with the hope of merging it into -next later this week. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] kernel/auditsc: remove unused header file
On Fri, Jul 30, 2021 at 3:52 AM Hui Su wrote:
>
> Since commit c72051d5778a ("audit: use ktime_get_coarse_ts64()
> for time access"), the linux/time.h is unused.
>
> Signed-off-by: Hui Su
> ---
> kernel/auditsc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8dd73a64f921..33bb6a83baf1 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -57,7 +57,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
At the very least the kernel/auditsc.c file still makes use of the
timespec64 struct which is defined in include/linux/time64.h which is
brought in by include/linux/time.h and *not* by
include/linux/timekeeping.h. As long as we make use of the timespec64
struct and the definition remains in time64.h let's keep the time.h
include.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 1/3] audit: replace magic audit syscall class numbers with macros
On Wed, May 19, 2021 at 4:01 PM Richard Guy Briggs wrote:
>
> Replace audit syscall class magic numbers with macros.
>
> This required putting the macros into new header file
> include/linux/auditsc_classmacros.h since the syscall macros were
> included for both 64 bit and 32 bit in any compat code, causing
> redefinition warnings.
>
> Signed-off-by: Richard Guy Briggs
> Link:
> https://lore.kernel.org/r/2300b1083a32aade7ae7efb95826e8f3f260b1df.1621363275.git@redhat.com
> ---
> MAINTAINERS | 1 +
> arch/alpha/kernel/audit.c | 8
> arch/ia64/kernel/audit.c| 8
> arch/parisc/kernel/audit.c | 8
> arch/parisc/kernel/compat_audit.c | 9 +
> arch/powerpc/kernel/audit.c | 10 +-
> arch/powerpc/kernel/compat_audit.c | 11 ++-
> arch/s390/kernel/audit.c| 10 +-
> arch/s390/kernel/compat_audit.c | 11 ++-
> arch/sparc/kernel/audit.c | 10 +-
> arch/sparc/kernel/compat_audit.c| 11 ++-
> arch/x86/ia32/audit.c | 11 ++-
> arch/x86/kernel/audit_64.c | 8
> include/linux/audit.h | 1 +
> include/linux/auditsc_classmacros.h | 23 +++
> kernel/auditsc.c| 12 ++--
> lib/audit.c | 10 +-
> lib/compat_audit.c | 11 ++-
> 18 files changed, 102 insertions(+), 71 deletions(-)
> create mode 100644 include/linux/auditsc_classmacros.h
...
> diff --git a/include/linux/auditsc_classmacros.h
> b/include/linux/auditsc_classmacros.h
> new file mode 100644
> index ..18757d270961
> --- /dev/null
> +++ b/include/linux/auditsc_classmacros.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* auditsc_classmacros.h -- Auditing support syscall macros
> + *
> + * Copyright 2021 Red Hat Inc., Durham, North Carolina.
> + * All Rights Reserved.
> + *
> + * Author: Richard Guy Briggs
> + */
> +#ifndef _LINUX_AUDITSCM_H_
> +#define _LINUX_AUDITSCM_H_
> +
> +enum auditsc_class_t {
> + AUDITSC_NATIVE = 0,
> + AUDITSC_COMPAT,
> + AUDITSC_OPEN,
> + AUDITSC_OPENAT,
> + AUDITSC_SOCKETCALL,
> + AUDITSC_EXECVE,
> +
> + AUDITSC_NVALS /* count */
> +};
> +
> +#endif
My apologies Richard, for some reason I had it in my mind that this
series was waiting on you to answer a question and/or respin; however,
now that I'm clearing my patch queues looking for any stragglers I see
that isn't the case. Looking over the patchset I think it looks okay
to me, my only concern is that "auditsc_classmacros.h" is an awfully
specific header file name and could prove to be annoying if we want to
add to it in the future. What do you think about something like
"audit_arch.h" instead?
If that change is okay with you I can go ahead and do the rename while
I'm merging the patches, I'll consider it penance for letting this
patchset sit for so long :/
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
