Add audit container identifier auxiliary record to user event standalone
records.
Signed-off-by: Richard Guy Briggs <[email protected]>
Acked-by: Neil Horman <[email protected]>
Reviewed-by: Ondrej Mosnacek <[email protected]>
---
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
@@ -1212,12 +1212,6 @@ static void audit_log_common_recv_msg(struct audit_context *context,
audit_log_task_context(*ab);
}
-static inline void audit_log_user_recv_msg(struct audit_buffer **ab,
- u16 msg_type)
-{
- audit_log_common_recv_msg(NULL, ab, msg_type);
-}
-
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1486,6 +1480,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
err = audit_filter(msg_type, AUDIT_FILTER_USER);
if (err == 1) { /* match or error */
char *str = data;
+ struct audit_context *context;
+ struct audit_contobj *cont;
err = 0;
if (msg_type == AUDIT_USER_TTY) {
@@ -1493,7 +1489,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (err)
break;
}
- audit_log_user_recv_msg(&ab, msg_type);
+ context = audit_alloc_local(GFP_KERNEL);
+ audit_log_common_recv_msg(context, &ab, msg_type);
if (msg_type != AUDIT_USER_TTY) {
/* ensure NULL termination */
str[data_len - 1] = '\0';
@@ -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);
}
break;
case AUDIT_ADD_RULE:
--
1.8.3.1
On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs <[email protected]> wrote:
>
> Add audit container identifier auxiliary record to user event standalone
> records.
>
> Signed-off-by: Richard Guy Briggs <[email protected]>
> Acked-by: Neil Horman <[email protected]>
> Reviewed-by: Ondrej Mosnacek <[email protected]>
> ---
> 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
http://www.paul-moore.com
On 2020-07-05 11:11, Paul Moore wrote:
> On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs <[email protected]> wrote:
> >
> > Add audit container identifier auxiliary record to user event standalone
> > records.
> >
> > Signed-off-by: Richard Guy Briggs <[email protected]>
> > Acked-by: Neil Horman <[email protected]>
> > Reviewed-by: Ondrej Mosnacek <[email protected]>
> > ---
> > 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?
I've redone the locking with an rcu lock around the get and a spinlock
around the put. It occurs to me that putting an rcu lock around the
whole thing and doing a get without the refcount increment would save
us the spinlock and put and be fine since we'd be fine with stale but
consistent information traversing the contobj list from this point to
report it. Problem with that is needing to use GFP_ATOMIC due to the
rcu lock. If I stick with the spinlock around the put then I can use
GFP_KERNEL and just grab the spinlock while traversing the contobj list.
> 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.
There is only one other place this is used, in audit_log_exit in
auditsc.c. I had noted the pattern but wasn't sure it was worth it.
Inline or not? Should we just let the compiler decide?
> paul moore
- RGB
--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
On Fri, Jul 17, 2020 at 8:44 PM Richard Guy Briggs <[email protected]> wrote:
> On 2020-07-05 11:11, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs <[email protected]> wrote:
> > >
> > > Add audit container identifier auxiliary record to user event standalone
> > > records.
> > >
> > > Signed-off-by: Richard Guy Briggs <[email protected]>
> > > Acked-by: Neil Horman <[email protected]>
> > > Reviewed-by: Ondrej Mosnacek <[email protected]>
> > > ---
> > > 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?
>
> I've redone the locking with an rcu lock around the get and a spinlock
> around the put. It occurs to me that putting an rcu lock around the
> whole thing and doing a get without the refcount increment would save
> us the spinlock and put and be fine since we'd be fine with stale but
> consistent information traversing the contobj list from this point to
> report it. Problem with that is needing to use GFP_ATOMIC due to the
> rcu lock. If I stick with the spinlock around the put then I can use
> GFP_KERNEL and just grab the spinlock while traversing the contobj list.
>
> > 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.
>
> There is only one other place this is used, in audit_log_exit in
> auditsc.c. I had noted the pattern but wasn't sure it was worth it.
> Inline or not? Should we just let the compiler decide?
I'm generally not a fan of explicit inlines unless it has been shown
to be a real problem.
--
paul moore
http://www.paul-moore.com