Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records

2018-04-19 Thread Paul Moore 
On Thu, Apr 19, 2018 at 8:31 AM, Richard Guy Briggs  wrote:
> On 2018-04-18 21:27, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
>> > Add container ID auxiliary records to configuration change, feature set 
>> > change
>> > and user generated standalone records.
>> >
>> > Signed-off-by: Richard Guy Briggs 
>> > ---
>> >  kernel/audit.c   | 50 
>> > --
>> >  kernel/auditfilter.c |  5 -
>> >  2 files changed, 44 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index b238be5..08662b4 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -400,8 +400,9 @@ static int audit_log_config_change(char 
>> > *function_name, u32 new, u32 old,
>> >  {
>> > struct audit_buffer *ab;
>> > int rc = 0;
>> > +   struct audit_context *context = audit_alloc_local();
>>
>> We should be able to use current->audit_context here right?  If we
>> can't for every caller, perhaps we pass an audit_context as an
>> argument and only allocate a local context when the passed
>> audit_context is NULL.
>>
>> Also, if you're not comfortable always using current, just pass the
>> audit_context as you do with audit_log_common_recv_msg().
>
> As mentioned in the tree/watch/mark patch, this is all obsoleted by
> making the AUDIT_CONFIG_CHANGE record a SYSCALL auxiliary record.

You've known about my desire to connect records for quite some time.

> This review would have been more helpful a month and a half ago.

If you really want to sink to that level of discussion, better quality
patches from you would have been helpful too, that is the one of the
main reasons why it takes so long to review your code.  Let's keep the
commentary focused on the code, discussions like this aren't likely to
be helpful to anyone.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records

2018-04-19 Thread Richard Guy Briggs 
On 2018-04-18 21:27, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> > Add container ID auxiliary records to configuration change, feature set 
> > change
> > and user generated standalone records.
> >
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  kernel/audit.c   | 50 
> > --
> >  kernel/auditfilter.c |  5 -
> >  2 files changed, 44 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index b238be5..08662b4 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -400,8 +400,9 @@ static int audit_log_config_change(char *function_name, 
> > u32 new, u32 old,
> >  {
> > struct audit_buffer *ab;
> > int rc = 0;
> > +   struct audit_context *context = audit_alloc_local();
> 
> We should be able to use current->audit_context here right?  If we
> can't for every caller, perhaps we pass an audit_context as an
> argument and only allocate a local context when the passed
> audit_context is NULL.
> 
> Also, if you're not comfortable always using current, just pass the
> audit_context as you do with audit_log_common_recv_msg().

As mentioned in the tree/watch/mark patch, this is all obsoleted by
making the AUDIT_CONFIG_CHANGE record a SYSCALL auxiliary record.
This review would have been more helpful a month and a half ago.

> > -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > if (unlikely(!ab))
> > return rc;
> > audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> > @@ -411,6 +412,8 @@ static int audit_log_config_change(char *function_name, 
> > u32 new, u32 old,
> > allow_changes = 0; /* Something weird, deny request */
> > audit_log_format(ab, " res=%d", allow_changes);
> > audit_log_end(ab);
> > +   audit_log_container_info(context, "config", 
> > audit_get_containerid(current));
> > +   audit_free_context(context);
> > return rc;
> >  }
> >
> > @@ -1058,7 +1061,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 
> > msg_type)
> > return err;
> >  }
> >
> > -static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 
> > msg_type)
> > +static void audit_log_common_recv_msg(struct audit_context *context,
> > + struct audit_buffer **ab, u16 
> > msg_type)
> >  {
> > uid_t uid = from_kuid(_user_ns, current_uid());
> > pid_t pid = task_tgid_nr(current);
> > @@ -1068,7 +1072,7 @@ static void audit_log_common_recv_msg(struct 
> > audit_buffer **ab, u16 msg_type)
> > return;
> > }
> >
> > -   *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
> > +   *ab = audit_log_start(context, GFP_KERNEL, msg_type);
> > if (unlikely(!*ab))
> > return;
> > audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> > @@ -1097,11 +1101,12 @@ static void audit_log_feature_change(int which, u32 
> > old_feature, u32 new_feature
> >  u32 old_lock, u32 new_lock, int res)
> >  {
> > struct audit_buffer *ab;
> > +   struct audit_context *context = audit_alloc_local();
> 
> So I know based on the other patch we are currently discussing that we
> can use current here ...
> 
> > if (audit_enabled == AUDIT_OFF)
> > return;
> >
> > -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> > +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> > if (!ab)
> > return;
> > audit_log_task_info(ab, current);
> > @@ -1109,6 +1114,8 @@ static void audit_log_feature_change(int which, u32 
> > old_feature, u32 new_feature
> >  audit_feature_names[which], !!old_feature, 
> > !!new_feature,
> >  !!old_lock, !!new_lock, res);
> > audit_log_end(ab);
> > +   audit_log_container_info(context, "feature", 
> > audit_get_containerid(current));
> > +   audit_free_context(context);
> >  }
> >
> >  static int audit_set_feature(struct sk_buff *skb)
> > @@ -1337,13 +1344,15 @@ 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 */
> > +   struct audit_context *context = audit_alloc_local();
> 
> I'm pretty sure we can use current here.
> 
> > err = 0;
> > if (msg_type == AUDIT_USER_TTY) {
> > err = tty_audit_push();
> > if (err)
> > break;
> > }
> > -   audit_log_common_recv_msg(, msg_type);
> > +

Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records

2018-04-18 Thread Paul Moore 
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Add container ID auxiliary records to configuration change, feature set change
> and user generated standalone records.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/audit.c   | 50 --
>  kernel/auditfilter.c |  5 -
>  2 files changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index b238be5..08662b4 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -400,8 +400,9 @@ static int audit_log_config_change(char *function_name, 
> u32 new, u32 old,
>  {
> struct audit_buffer *ab;
> int rc = 0;
> +   struct audit_context *context = audit_alloc_local();

We should be able to use current->audit_context here right?  If we
can't for every caller, perhaps we pass an audit_context as an
argument and only allocate a local context when the passed
audit_context is NULL.

Also, if you're not comfortable always using current, just pass the
audit_context as you do with audit_log_common_recv_msg().

> -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return rc;
> audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> @@ -411,6 +412,8 @@ static int audit_log_config_change(char *function_name, 
> u32 new, u32 old,
> allow_changes = 0; /* Something weird, deny request */
> audit_log_format(ab, " res=%d", allow_changes);
> audit_log_end(ab);
> +   audit_log_container_info(context, "config", 
> audit_get_containerid(current));
> +   audit_free_context(context);
> return rc;
>  }
>
> @@ -1058,7 +1061,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 
> msg_type)
> return err;
>  }
>
> -static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> +static void audit_log_common_recv_msg(struct audit_context *context,
> + struct audit_buffer **ab, u16 msg_type)
>  {
> uid_t uid = from_kuid(_user_ns, current_uid());
> pid_t pid = task_tgid_nr(current);
> @@ -1068,7 +1072,7 @@ static void audit_log_common_recv_msg(struct 
> audit_buffer **ab, u16 msg_type)
> return;
> }
>
> -   *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
> +   *ab = audit_log_start(context, GFP_KERNEL, msg_type);
> if (unlikely(!*ab))
> return;
> audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> @@ -1097,11 +1101,12 @@ static void audit_log_feature_change(int which, u32 
> old_feature, u32 new_feature
>  u32 old_lock, u32 new_lock, int res)
>  {
> struct audit_buffer *ab;
> +   struct audit_context *context = audit_alloc_local();

So I know based on the other patch we are currently discussing that we
can use current here ...

> if (audit_enabled == AUDIT_OFF)
> return;
>
> -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> if (!ab)
> return;
> audit_log_task_info(ab, current);
> @@ -1109,6 +1114,8 @@ static void audit_log_feature_change(int which, u32 
> old_feature, u32 new_feature
>  audit_feature_names[which], !!old_feature, 
> !!new_feature,
>  !!old_lock, !!new_lock, res);
> audit_log_end(ab);
> +   audit_log_container_info(context, "feature", 
> audit_get_containerid(current));
> +   audit_free_context(context);
>  }
>
>  static int audit_set_feature(struct sk_buff *skb)
> @@ -1337,13 +1344,15 @@ 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 */
> +   struct audit_context *context = audit_alloc_local();

I'm pretty sure we can use current here.

> err = 0;
> if (msg_type == AUDIT_USER_TTY) {
> err = tty_audit_push();
> if (err)
> break;
> }
> -   audit_log_common_recv_msg(, msg_type);
> +   audit_log_common_recv_msg(context, , msg_type);
> if (msg_type != AUDIT_USER_TTY)
> audit_log_format(ab, " msg='%.*s'",
>  AUDIT_MESSAGE_TEXT_MAX,
> @@ -1359,6 +1368,9 @@ static int audit_receive_msg(struct sk_buff *skb, 
> struct nlmsghdr *nlh)
> audit_log_n_untrustedstring(ab, data, size);
> }
>

[RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records

2018-03-16 Thread Richard Guy Briggs 
Add container ID auxiliary records to configuration change, feature set change
and user generated standalone records.

Signed-off-by: Richard Guy Briggs 
---
 kernel/audit.c   | 50 --
 kernel/auditfilter.c |  5 -
 2 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index b238be5..08662b4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -400,8 +400,9 @@ static int audit_log_config_change(char *function_name, u32 
new, u32 old,
 {
struct audit_buffer *ab;
int rc = 0;
+   struct audit_context *context = audit_alloc_local();
 
-   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+   ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return rc;
audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
@@ -411,6 +412,8 @@ static int audit_log_config_change(char *function_name, u32 
new, u32 old,
allow_changes = 0; /* Something weird, deny request */
audit_log_format(ab, " res=%d", allow_changes);
audit_log_end(ab);
+   audit_log_container_info(context, "config", 
audit_get_containerid(current));
+   audit_free_context(context);
return rc;
 }
 
@@ -1058,7 +1061,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 
msg_type)
return err;
 }
 
-static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+static void audit_log_common_recv_msg(struct audit_context *context,
+ struct audit_buffer **ab, u16 msg_type)
 {
uid_t uid = from_kuid(_user_ns, current_uid());
pid_t pid = task_tgid_nr(current);
@@ -1068,7 +1072,7 @@ static void audit_log_common_recv_msg(struct audit_buffer 
**ab, u16 msg_type)
return;
}
 
-   *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
+   *ab = audit_log_start(context, GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
@@ -1097,11 +1101,12 @@ static void audit_log_feature_change(int which, u32 
old_feature, u32 new_feature
 u32 old_lock, u32 new_lock, int res)
 {
struct audit_buffer *ab;
+   struct audit_context *context = audit_alloc_local();
 
if (audit_enabled == AUDIT_OFF)
return;
 
-   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+   ab = audit_log_start(context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
if (!ab)
return;
audit_log_task_info(ab, current);
@@ -1109,6 +1114,8 @@ static void audit_log_feature_change(int which, u32 
old_feature, u32 new_feature
 audit_feature_names[which], !!old_feature, 
!!new_feature,
 !!old_lock, !!new_lock, res);
audit_log_end(ab);
+   audit_log_container_info(context, "feature", 
audit_get_containerid(current));
+   audit_free_context(context);
 }
 
 static int audit_set_feature(struct sk_buff *skb)
@@ -1337,13 +1344,15 @@ 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 */
+   struct audit_context *context = audit_alloc_local();
+
err = 0;
if (msg_type == AUDIT_USER_TTY) {
err = tty_audit_push();
if (err)
break;
}
-   audit_log_common_recv_msg(, msg_type);
+   audit_log_common_recv_msg(context, , msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
 AUDIT_MESSAGE_TEXT_MAX,
@@ -1359,6 +1368,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
audit_log_n_untrustedstring(ab, data, size);
}
audit_log_end(ab);
+   audit_log_container_info(context, "user",
+
audit_get_containerid(current));
+   audit_free_context(context);
}
break;
case AUDIT_ADD_RULE:
@@ -1366,9 +1378,14 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
if (nlmsg_len(nlh) < sizeof(struct audit_rule_data))
return -EINVAL;
if (audit_enabled == AUDIT_LOCKED) {
-   audit_log_common_recv_msg(, AUDIT_CONFIG_CHANGE);
+   struct audit_context *context = audit_alloc_local();
+
+