* Re: [PATCH 24/24] debugfs: Restrict debugfs when the kernel is locked down
From: David Howells @ 2018-04-19 14:35 UTC (permalink / raw)
To: Pavel Machek
Cc: dhowells, torvalds, linux-man, linux-api, jmorris, linux-kernel,
linux-security-module
In-Reply-To: <20180413202241.GB4484@amd>
Pavel Machek <pavel@ucw.cz> wrote:
> > (1) chmod and chown are disallowed on debugfs objects (though the root dir
> > can be modified by mount and remount, but I'm not worried about that).
>
> This has nothing to do with the lockdown goals, right? I find chown of
> such files quite nice, to allow debugging without doing sudo all the time.
It allows someone to give everyone access to files that should perhaps only be
accessible by root. Besides, if you disable lockdown then you can do this if
you want.
> > (2) When the kernel is locked down, only files with the following criteria
> > are permitted to be opened:
> >
> > - The file must have mode 00444
> > - The file must not have ioctl methods
> > - The file must not have mmap
>
> Dunno. Would not it be nicer to go through the debugfs files and split
> them into safe/unsafe varieties?
Whilst that is a laudable idea, there are at least a couple of thousand files
to analyse, some of them doing weird stuff in drivers that aren't easy to
understand, and some of them with lists of files, some of which might be safe
and some of which aren't safe. Even some reads that look like they ought to
be safe have side effects (such as read-and-reset counters).
Besides, define 'safe'. Is reading a particular reg on a device and returning
the value through a debugfs read safe, for example? What about files where
reading is harmless, but writing needs to be disallowed?
I did try initially passing in a flag to say whether something was safe or
not, abusing an inode flag to store it since debugfs uses a plain inode
struct, but then I realised just how many ways there are to create debugfs
files and it started to get out of hand. My initial idea also included
locking everything down by default and marking things unlocked on an as-needed
basis.
If you have the time to audit all these files, then that would be great.
I lean more towards the lock debugfs down entirely side.
David
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Paul Moore @ 2018-04-19 13:13 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: simo-H+wXaHxf7aLQT0dZR+AlfA, jlayton-H+wXaHxf7aLQT0dZR+AlfA,
carlos-H+wXaHxf7aLQT0dZR+AlfA, linux-api-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
Eric Paris, dhowells-H+wXaHxf7aLQT0dZR+AlfA,
Linux-Audit Mailing List, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
luto-DgEjT+Ai2ygdnm+yROfE0A, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
In-Reply-To: <20180419124550.7uknp4oebvwoo67s-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On Thu, Apr 19, 2018 at 8:45 AM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 2018-04-18 22:10, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > Add container ID auxiliary record(s) to NETFILTER_PKT event standalone
>> > records. Iterate through all potential container IDs associated with a
>> > network namespace.
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> > kernel/audit.c | 1 +
>> > kernel/auditsc.c | 2 ++
>> > net/netfilter/xt_AUDIT.c | 15 ++++++++++++++-
>> > 3 files changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index 08662b4..3c77e47 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -2102,6 +2102,7 @@ int audit_log_container_info(struct audit_context *context,
>> > audit_log_end(ab);
>> > return 0;
>> > }
>> > +EXPORT_SYMBOL(audit_log_container_info);
>> >
>> > void audit_log_key(struct audit_buffer *ab, char *key)
>> > {
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 208da962..af68d01 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -975,6 +975,7 @@ struct audit_context *audit_alloc_local(void)
>> > context->in_syscall = 1;
>> > return context;
>> > }
>> > +EXPORT_SYMBOL(audit_alloc_local);
>> >
>> > inline void audit_free_context(struct audit_context *context)
>> > {
>> > @@ -989,6 +990,7 @@ inline void audit_free_context(struct audit_context *context)
>> > audit_proctitle_free(context);
>> > kfree(context);
>> > }
>> > +EXPORT_SYMBOL(audit_free_context);
>> >
>> > static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>> > kuid_t auid, kuid_t uid, unsigned int sessionid,
>> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
>> > index c502419..edaa456 100644
>> > --- a/net/netfilter/xt_AUDIT.c
>> > +++ b/net/netfilter/xt_AUDIT.c
>> > @@ -71,10 +71,14 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>> > {
>> > struct audit_buffer *ab;
>> > int fam = -1;
>> > + struct audit_context *context = audit_alloc_local();
>> > + struct audit_containerid *cont;
>> > + int i = 0;
>> > + struct net *net;
>> >
>> > if (audit_enabled == 0)
>> > goto errout;
>>
>> Do I need to say it? I probably should ... the allocation should
>> happen after the audit_enabled check.
>
> Already fixed in V3 in my tree a couple of weeks ago...
... which you never posted, at least not anywhere I've seen. Which
effectively means I wasted a good chunk of time reviewing this code
late last night. Awesome.
> More timely review please?
More patience on your part?
>> > - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
>> > + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
>> > if (ab == NULL)
>> > goto errout;
>> >
>> > @@ -104,7 +108,16 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>> >
>> > audit_log_end(ab);
>> >
>> > + net = sock_net(NETLINK_CB(skb).sk);
>> > + list_for_each_entry(cont, &net->audit_containerid, list) {
>> > + char buf[14];
>> > +
>> > + sprintf(buf, "net%u", i++);
>> > + audit_log_container_info(context, buf, cont->id);
>> > + }
>>
>> It seems like this could (should?) be hidden inside an audit function,
>> e.g. audit_log_net_containers() or something like that.
>
> Perhaps... It was open-coded since at this point there are no other
> users. That'll make this tidier though.
If the code was all contained within a single subsystem them I would
generally agree that open coding is preferable, but since we are
crossing a subsystem boundary I think it would be preferable to
abstract away the details into a separate function.
This will probably also be necessary once you change to using the
audit_net/net_generic mechanism.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records
From: Paul Moore @ 2018-04-19 12:59 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <20180419123109.ab7gsnwrbtog4tbf@madcap2.tricolour.ca>
On Thu, Apr 19, 2018 at 8:31 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-04-18 21:27, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Add container ID auxiliary records to configuration change, feature set change
>> > and user generated standalone records.
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> > 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
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Richard Guy Briggs @ 2018-04-19 12:45 UTC (permalink / raw)
To: Paul Moore
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhR3BNRr24BPxud0X_eyFmSxUOh9bwjWNU4Z=rnDR0fENA@mail.gmail.com>
On 2018-04-18 22:10, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add container ID auxiliary record(s) to NETFILTER_PKT event standalone
> > records. Iterate through all potential container IDs associated with a
> > network namespace.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > kernel/audit.c | 1 +
> > kernel/auditsc.c | 2 ++
> > net/netfilter/xt_AUDIT.c | 15 ++++++++++++++-
> > 3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 08662b4..3c77e47 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2102,6 +2102,7 @@ int audit_log_container_info(struct audit_context *context,
> > audit_log_end(ab);
> > return 0;
> > }
> > +EXPORT_SYMBOL(audit_log_container_info);
> >
> > void audit_log_key(struct audit_buffer *ab, char *key)
> > {
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 208da962..af68d01 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -975,6 +975,7 @@ struct audit_context *audit_alloc_local(void)
> > context->in_syscall = 1;
> > return context;
> > }
> > +EXPORT_SYMBOL(audit_alloc_local);
> >
> > inline void audit_free_context(struct audit_context *context)
> > {
> > @@ -989,6 +990,7 @@ inline void audit_free_context(struct audit_context *context)
> > audit_proctitle_free(context);
> > kfree(context);
> > }
> > +EXPORT_SYMBOL(audit_free_context);
> >
> > static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> > kuid_t auid, kuid_t uid, unsigned int sessionid,
> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> > index c502419..edaa456 100644
> > --- a/net/netfilter/xt_AUDIT.c
> > +++ b/net/netfilter/xt_AUDIT.c
> > @@ -71,10 +71,14 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> > {
> > struct audit_buffer *ab;
> > int fam = -1;
> > + struct audit_context *context = audit_alloc_local();
> > + struct audit_containerid *cont;
> > + int i = 0;
> > + struct net *net;
> >
> > if (audit_enabled == 0)
> > goto errout;
>
> Do I need to say it? I probably should ... the allocation should
> happen after the audit_enabled check.
Already fixed in V3 in my tree a couple of weeks ago...
More timely review please?
> > - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> > + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> > if (ab == NULL)
> > goto errout;
> >
> > @@ -104,7 +108,16 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> >
> > audit_log_end(ab);
> >
> > + net = sock_net(NETLINK_CB(skb).sk);
> > + list_for_each_entry(cont, &net->audit_containerid, list) {
> > + char buf[14];
> > +
> > + sprintf(buf, "net%u", i++);
> > + audit_log_container_info(context, buf, cont->id);
> > + }
>
> It seems like this could (should?) be hidden inside an audit function,
> e.g. audit_log_net_containers() or something like that.
Perhaps... It was open-coded since at this point there are no other
users. That'll make this tidier though.
> > errout:
> > + audit_free_context(context);
> > return XT_CONTINUE;
> > }
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
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
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records
From: Richard Guy Briggs @ 2018-04-19 12:31 UTC (permalink / raw)
To: Paul Moore
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhQ-i5oA48sXXnN2fP06t5=9-NMoY0bKcGXorQw2k=CK0Q@mail.gmail.com>
On 2018-04-18 21:27, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add container ID auxiliary records to configuration change, feature set change
> > and user generated standalone records.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > 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(&init_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(&ab, msg_type);
> > + audit_log_common_recv_msg(context, &ab, 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(&ab, AUDIT_CONFIG_CHANGE);
> > + struct audit_context *context = audit_alloc_local();
>
> Pretty sure current can be used here too. In fact I think everywhere
> where we are processing commands from netlink we can use current as I
> believe the entire netlink stack is processed in the context of the
> caller.
>
> > + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE);
> > audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
> > audit_log_end(ab);
> > + audit_log_container_info(context, "config",
> > + audit_get_containerid(current));
> > + audit_free_context(context);
> > return -EPERM;
> > }
> > err = audit_rule_change(msg_type, seq, data, nlmsg_len(nlh));
> > @@ -1376,17 +1393,23 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > case AUDIT_LIST_RULES:
> > err = audit_list_rules_send(skb, seq);
> > break;
> > - case AUDIT_TRIM:
> > + case AUDIT_TRIM: {
> > + struct audit_context *context = audit_alloc_local();
>
> Same.
>
> > audit_trim_trees();
> > - audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> > + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE);
> > audit_log_format(ab, " op=trim res=1");
> > audit_log_end(ab);
> > + audit_log_container_info(context, "config",
> > + audit_get_containerid(current));
> > + audit_free_context(context);
> > break;
> > + }
> > case AUDIT_MAKE_EQUIV: {
> > void *bufp = data;
> > u32 sizes[2];
> > size_t msglen = nlmsg_len(nlh);
> > char *old, *new;
> > + struct audit_context *context = audit_alloc_local();
>
> Same.
>
> > err = -EINVAL;
> > if (msglen < 2 * sizeof(u32))
> > @@ -1408,7 +1431,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > /* OK, here comes... */
> > err = audit_tag_tree(old, new);
> >
> > - audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> > + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE);
> >
> > audit_log_format(ab, " op=make_equiv old=");
> > audit_log_untrustedstring(ab, old);
> > @@ -1418,6 +1441,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > audit_log_end(ab);
> > kfree(old);
> > kfree(new);
> > + audit_log_container_info(context, "config",
> > + audit_get_containerid(current));
> > + audit_free_context(context);
> > break;
> > }
> > case AUDIT_SIGNAL_INFO:
> > @@ -1459,6 +1485,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > struct audit_tty_status s, old;
> > struct audit_buffer *ab;
> > unsigned int t;
> > + struct audit_context *context = audit_alloc_local();
>
> Same.
>
> > memset(&s, 0, sizeof(s));
> > /* guard against past and future API changes */
> > @@ -1477,12 +1504,15 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > old.enabled = t & AUDIT_TTY_ENABLE;
> > old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD);
> >
> > - audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> > + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE);
> > audit_log_format(ab, " op=tty_set old-enabled=%d new-enabled=%d"
> > " old-log_passwd=%d new-log_passwd=%d res=%d",
> > old.enabled, s.enabled, old.log_passwd,
> > s.log_passwd, !err);
> > audit_log_end(ab);
> > + audit_log_container_info(context, "config",
> > + audit_get_containerid(current));
> > + audit_free_context(context);
> > break;
> > }
> > default:
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index c4c8746..5f7f4d6 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -1109,11 +1109,12 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
> > struct audit_buffer *ab;
> > uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(current));
> > unsigned int sessionid = audit_get_sessionid(current);
> > + struct audit_context *context = audit_alloc_local();
> >
> > if (!audit_enabled)
> > return;
>
> Well, first I think we should be able to get rid of the local context,
> but if for some reason we can't use current->audit_context then do the
> allocation after the audit_enabled check.
>
> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > if (!ab)
> > return;
> > audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
> > @@ -1122,6 +1123,8 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
> > audit_log_key(ab, rule->filterkey);
> > audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
> > audit_log_end(ab);
> > + audit_log_container_info(context, "config", audit_get_containerid(current));
> > + audit_free_context(context);
> > }
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
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
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 07/13] audit: add container aux record to watch/tree/mark
From: Richard Guy Briggs @ 2018-04-19 12:24 UTC (permalink / raw)
To: Paul Moore
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhTzp-r2TFytt1zTEpeGK=O5dEnLPFw-CdsM1ttpY0a30g@mail.gmail.com>
On 2018-04-18 20:42, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add container ID auxiliary record to mark, watch and tree rule
> > configuration standalone records.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > kernel/audit_fsnotify.c | 5 ++++-
> > kernel/audit_tree.c | 5 ++++-
> > kernel/audit_watch.c | 33 +++++++++++++++++++--------------
> > 3 files changed, 27 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> > index 52f368b..18c110d 100644
> > --- a/kernel/audit_fsnotify.c
> > +++ b/kernel/audit_fsnotify.c
> > @@ -124,10 +124,11 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
> > {
> > struct audit_buffer *ab;
> > struct audit_krule *rule = audit_mark->rule;
> > + struct audit_context *context = audit_alloc_local();
> >
> > if (!audit_enabled)
> > return;
>
> Move the audit_alloc_local() after the audit_enabled check.
Already fixed in V3 as previously warned, by making all
AUDIT_CONFIG_CHANGE records SYSCALL auxiliary records.
> > - ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> > + ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> > if (unlikely(!ab))
> > return;
> > audit_log_format(ab, "auid=%u ses=%u op=%s",
> > @@ -138,6 +139,8 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
> > audit_log_key(ab, rule->filterkey);
> > audit_log_format(ab, " list=%d res=1", rule->listnr);
> > audit_log_end(ab);
> > + audit_log_container_info(context, "config", audit_get_containerid(current));
> > + audit_free_context(context);
> > }
> >
> > void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index 67e6956..7c085be 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -496,8 +496,9 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
> > static void audit_tree_log_remove_rule(struct audit_krule *rule)
> > {
> > struct audit_buffer *ab;
> > + struct audit_context *context = audit_alloc_local();
>
> Sort of independent of the audit container ID work, but shouldn't we
> have an audit_enabled check here?
Same.
> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > if (unlikely(!ab))
> > return;
> > audit_log_format(ab, "op=remove_rule");
> > @@ -506,6 +507,8 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
> > audit_log_key(ab, rule->filterkey);
> > audit_log_format(ab, " list=%d res=1", rule->listnr);
> > audit_log_end(ab);
> > + audit_log_container_info(context, "config", audit_get_containerid(current));
> > + audit_free_context(context);
> > }
> >
> > static void kill_rules(struct audit_tree *tree)
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index 9eb8b35..60d75a2 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -238,20 +238,25 @@ static struct audit_watch *audit_dupe_watch(struct audit_watch *old)
> >
> > static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watch *w, char *op)
> > {
> > - if (audit_enabled) {
> > - struct audit_buffer *ab;
> > - ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> > - if (unlikely(!ab))
> > - return;
> > - audit_log_format(ab, "auid=%u ses=%u op=%s",
> > - from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > - audit_get_sessionid(current), op);
> > - audit_log_format(ab, " path=");
> > - audit_log_untrustedstring(ab, w->path);
> > - audit_log_key(ab, r->filterkey);
> > - audit_log_format(ab, " list=%d res=1", r->listnr);
> > - audit_log_end(ab);
> > - }
> > + struct audit_buffer *ab;
> > + struct audit_context *context = audit_alloc_local();
> > +
> > + if (!audit_enabled)
> > + return;
>
> Same as above, do the allocation after the audit_enabled check.
Same.
> > + ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> > + if (unlikely(!ab))
> > + return;
> > + audit_log_format(ab, "auid=%u ses=%u op=%s",
> > + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > + audit_get_sessionid(current), op);
> > + audit_log_format(ab, " path=");
> > + audit_log_untrustedstring(ab, w->path);
> > + audit_log_key(ab, r->filterkey);
> > + audit_log_format(ab, " list=%d res=1", r->listnr);
> > + audit_log_end(ab);
> > + audit_log_container_info(context, "config", audit_get_containerid(current));
> > + audit_free_context(context);
> > }
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
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
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering
From: Richard Guy Briggs @ 2018-04-19 12:17 UTC (permalink / raw)
To: Paul Moore
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhRVGTCVJxG3Etcs-aOpr71A7xGsn5VPhskUG35rmQ7WUw@mail.gmail.com>
On 2018-04-18 20:24, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Implement container ID filtering using the AUDIT_CONTAINERID field name
> > to send an 8-character string representing a u64 since the value field
> > is only u32.
> >
> > Sending it as two u32 was considered, but gathering and comparing two
> > fields was more complex.
>
> My only worry here is that you aren't really sending a string in the
> ASCII sense, you are sending an 8 byte buffer (that better be NUL
> terminated) that happens to be an unsigned 64-bit integer. To be
> clear, I'm okay with that (it's protected by AUDIT_CONTAINERID), and
> the code is okay with that, I just want us to pause for a minute and
> make sure that is an okay thing to do long term.
I already went through that process and warned of it 7 weeks ago. As
already noted, That was preferable to two seperate u32 fields that
depend on each other making comparisons more complicated. Using two
seperate fields to configure the rule could be gated for validity, then
the result stored in a special rule field, but I wasn't keen about that
approach.
> > The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER.
> >
> > This requires support from userspace to be useful.
> > See: https://github.com/linux-audit/audit-userspace/issues/40
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 1 +
> > include/uapi/linux/audit.h | 5 ++++-
> > kernel/audit.h | 1 +
> > kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> > kernel/auditsc.c | 3 +++
> > 5 files changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 3acbe9d..f10ca1b 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -76,6 +76,7 @@ struct audit_field {
> > u32 type;
> > union {
> > u32 val;
> > + u64 val64;
> > kuid_t uid;
> > kgid_t gid;
> > struct {
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index e83ccbd..8443a8f 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -262,6 +262,7 @@
> > #define AUDIT_LOGINUID_SET 24
> > #define AUDIT_SESSIONID 25 /* Session ID */
> > #define AUDIT_FSTYPE 26 /* FileSystem Type */
> > +#define AUDIT_CONTAINERID 27 /* Container ID */
> >
> > /* These are ONLY useful when checking
> > * at syscall exit time (AUDIT_AT_EXIT). */
> > @@ -342,6 +343,7 @@ enum {
> > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> > #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> > #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> > +#define AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER 0x00000080
> >
> > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> > @@ -349,7 +351,8 @@ enum {
> > AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> > AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> > AUDIT_FEATURE_BITMAP_LOST_RESET | \
> > - AUDIT_FEATURE_BITMAP_FILTER_FS)
> > + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> > + AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER)
> >
> > /* deprecated: AUDIT_VERSION_* */
> > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 214e149..aaa651a 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -234,6 +234,7 @@ static inline int audit_hash_ino(u32 ino)
> >
> > extern int audit_match_class(int class, unsigned syscall);
> > extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> > +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> > extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> > extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> > extern int parent_len(const char *path);
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index d7a807e..c4c8746 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> > /* FALL THROUGH */
> > case AUDIT_ARCH:
> > case AUDIT_FSTYPE:
> > + case AUDIT_CONTAINERID:
> > if (f->op != Audit_not_equal && f->op != Audit_equal)
> > return -EINVAL;
> > break;
> > @@ -584,6 +585,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> > }
> > entry->rule.exe = audit_mark;
> > break;
> > + case AUDIT_CONTAINERID:
> > + if (f->val != sizeof(u64))
> > + goto exit_free;
> > + str = audit_unpack_string(&bufp, &remain, f->val);
> > + if (IS_ERR(str))
> > + goto exit_free;
> > + f->val64 = ((u64 *)str)[0];
> > + break;
> > }
> > }
> >
> > @@ -666,6 +675,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> > data->buflen += data->values[i] =
> > audit_pack_string(&bufp, audit_mark_path(krule->exe));
> > break;
> > + case AUDIT_CONTAINERID:
> > + data->buflen += data->values[i] = sizeof(u64);
> > + for (i = 0; i < sizeof(u64); i++)
> > + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> > + break;
> > case AUDIT_LOGINUID_SET:
> > if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> > data->fields[i] = AUDIT_LOGINUID;
> > @@ -752,6 +766,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> > if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> > return 1;
> > break;
> > + case AUDIT_CONTAINERID:
> > + if (a->fields[i].val64 != b->fields[i].val64)
> > + return 1;
> > + break;
> > default:
> > if (a->fields[i].val != b->fields[i].val)
> > return 1;
> > @@ -1210,6 +1228,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> > }
> > }
> >
> > +int audit_comparator64(u64 left, u32 op, u64 right)
> > +{
> > + switch (op) {
> > + case Audit_equal:
> > + return (left == right);
> > + case Audit_not_equal:
> > + return (left != right);
> > + case Audit_lt:
> > + return (left < right);
> > + case Audit_le:
> > + return (left <= right);
> > + case Audit_gt:
> > + return (left > right);
> > + case Audit_ge:
> > + return (left >= right);
> > + case Audit_bitmask:
> > + return (left & right);
> > + case Audit_bittest:
> > + return ((left & right) == right);
> > + default:
> > + BUG();
> > + return 0;
> > + }
> > +}
> > +
> > int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> > {
> > switch (op) {
> > @@ -1348,6 +1391,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> > result = audit_comparator(audit_loginuid_set(current),
> > f->op, f->val);
> > break;
> > + case AUDIT_CONTAINERID:
> > + result = audit_comparator64(audit_get_containerid(current),
> > + f->op, f->val64);
> > + break;
> > case AUDIT_MSGTYPE:
> > result = audit_comparator(msgtype, f->op, f->val);
> > break;
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 65be110..2bba324 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -614,6 +614,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > case AUDIT_LOGINUID_SET:
> > result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> > break;
> > + case AUDIT_CONTAINERID:
> > + result = audit_comparator64(audit_get_containerid(tsk), f->op, f->val64);
> > + break;
> > case AUDIT_SUBJ_USER:
> > case AUDIT_SUBJ_ROLE:
> > case AUDIT_SUBJ_TYPE:
> > --
> > 1.8.3.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
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
^ permalink raw reply
* Re: [tip GIT PULL] y2038: timekeeping syscall changes
From: Arnd Bergmann @ 2018-04-19 12:04 UTC (permalink / raw)
To: Thomas Gleixner
Cc: y2038 Mailman List, Deepa Dinamani, Linux Kernel Mailing List,
linux-arch, Linux API
In-Reply-To: <alpine.DEB.2.21.1804191240510.1563@nanos.tec.linutronix.de>
On Thu, Apr 19, 2018 at 12:48 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Can you please fix that and send me an updated pull request?
Done. I sent a v2 pull request now as a separate thread. I still messed
up the URL but replied with the correct one and figured out how to
get that all automatically now.
Arnd
^ permalink raw reply
* Re: [tip GIT PULL v2] y2038: timekeeping syscall changes
From: Arnd Bergmann @ 2018-04-19 12:03 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Thomas Gleixner, y2038 Mailman List, Deepa Dinamani,
Linux Kernel Mailing List, linux-arch, Linux API
In-Reply-To: <CAMuHMdXNHaBVMqrQOfBuN+Ro4SC40B9OxCs9HsNM_c4fxcJT0g@mail.gmail.com>
On Thu, Apr 19, 2018 at 1:59 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Arnd,
>
> On Thu, Apr 19, 2018 at 1:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thu, Apr 19, 2018 at 1:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:
>>>
>>> Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)
>>>
>>> are available in the git repository at:
>>>
>>> git+ssh://gitolite@ra.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
>>> tags/y2038-timekeeping
>>
>> That one is still messed up. I changed my .gitconfig to have a
>> different "insteadof"
>> section, but for some reason that failed. Obviously the URL I meant is
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
>> tags/y2038-timekeeping
>>
>> I've never figured out how to configure this correctly and try to
>> manually edit the
>> pull request, but forget to do that half of the time.
>
> I use:
>
> [remote "foo"]
> url = git://git.kernel.org/pub/scm/linux/kernel/git/user/foo.git
> pushurl = gitolite.kernel.org:/pub/scm/linux/kernel/git/user/foo.git
Thanks! I've figured something out now using 'insteadOf' and
'pushInsteadOf' config options to the same effect. I had a
separate pushurl in the past but that broke when I added
an insteadOf substitution without adding pushInsteadOf.
Arnd
^ permalink raw reply
* Re: [tip GIT PULL v2] y2038: timekeeping syscall changes
From: Geert Uytterhoeven @ 2018-04-19 11:59 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Thomas Gleixner, y2038 Mailman List, Deepa Dinamani,
Linux Kernel Mailing List, linux-arch, Linux API
In-Reply-To: <CAK8P3a3vS1AWHGMmGchQOrkV40GESNuJHQfZF-YDtOwZ1Z18Dw@mail.gmail.com>
Hi Arnd,
On Thu, Apr 19, 2018 at 1:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Apr 19, 2018 at 1:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:
>>
>> Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)
>>
>> are available in the git repository at:
>>
>> git+ssh://gitolite@ra.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
>> tags/y2038-timekeeping
>
> That one is still messed up. I changed my .gitconfig to have a
> different "insteadof"
> section, but for some reason that failed. Obviously the URL I meant is
>
> git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
> tags/y2038-timekeeping
>
> I've never figured out how to configure this correctly and try to
> manually edit the
> pull request, but forget to do that half of the time.
I use:
[remote "foo"]
url = git://git.kernel.org/pub/scm/linux/kernel/git/user/foo.git
pushurl = gitolite.kernel.org:/pub/scm/linux/kernel/git/user/foo.git
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [tip GIT PULL v2] y2038: timekeeping syscall changes
From: Arnd Bergmann @ 2018-04-19 11:47 UTC (permalink / raw)
To: Thomas Gleixner
Cc: y2038 Mailman List, Deepa Dinamani, Linux Kernel Mailing List,
linux-arch, Linux API
In-Reply-To: <CAK8P3a0-n-Qabdr+5hwjq8wa9nZHtFwcE32WsW8ZTbKjtpiBUQ@mail.gmail.com>
On Thu, Apr 19, 2018 at 1:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:
>
> Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)
>
> are available in the git repository at:
>
> git+ssh://gitolite@ra.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
> tags/y2038-timekeeping
That one is still messed up. I changed my .gitconfig to have a
different "insteadof"
section, but for some reason that failed. Obviously the URL I meant is
git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
tags/y2038-timekeeping
I've never figured out how to configure this correctly and try to
manually edit the
pull request, but forget to do that half of the time.
Arnd
^ permalink raw reply
* [tip GIT PULL v2] y2038: timekeeping syscall changes
From: Arnd Bergmann @ 2018-04-19 11:45 UTC (permalink / raw)
To: Thomas Gleixner
Cc: y2038 Mailman List, Deepa Dinamani, Linux Kernel Mailing List,
linux-arch, Linux API
The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:
Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)
are available in the git repository at:
git+ssh://gitolite@ra.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
tags/y2038-timekeeping
for you to fetch changes up to 01909974b41036a6a8d3907c66cc7b41c9a73da9:
time: Change nanosleep to safe __kernel_* types (2018-04-19 13:32:03 +0200)
----------------------------------------------------------------
y2038: timekeeping syscall changes
This is the first set of system call entry point changes to enable 32-bit
architectures to have variants on both 32-bit and 64-bit time_t. Typically
these system calls take a 'struct timespec' argument, but that structure
is defined in user space by the C library and its layout will change.
The kernel already supports handling the 32-bit time_t on 64-bit
architectures through the CONFIG_COMPAT mechanism. As there are a total
of 51 system calls suffering from this problem, reusing that mechanism
on 32-bit architectures.
We already have patches for most of the remaining system calls, but this
set contains most of the complexity and is best tested. There was one
last-minute regression that prevented it from going into 4.17, but that
is fixed now.
More details from Deepa's patch series description:
Big picture is as per the lwn article:
https://lwn.net/Articles/643234/ [2]
The series is directed at converting posix clock syscalls:
clock_gettime, clock_settime, clock_getres and clock_nanosleep
to use a new data structure __kernel_timespec at syscall boundaries.
__kernel_timespec maintains 64 bit time_t across all execution modes.
vdso will be handled as part of each architecture when they enable
support for 64 bit time_t.
The compat syscalls are repurposed to provide backward compatibility
by using them as native syscalls as well for 32 bit architectures.
They will continue to use timespec at syscall boundaries.
CONFIG_64_BIT_TIME controls whether the syscalls use __kernel_timespec
or timespec at syscall boundaries.
The series does the following:
1. Enable compat syscalls on 32 bit architectures.
2. Add a new __kernel_timespec type to be used as the data structure
for all the new syscalls.
3. Add new config CONFIG_64BIT_TIME(intead of the CONFIG_COMPAT_TIME in
[1] and [2] to switch to new definition of __kernel_timespec. It is
the same as struct timespec otherwise.
4. Add new CONFIG_32BIT_TIME to conditionally compile compat syscalls.
----------------------------------------------------------------
v2: rewrote subject lines as suggested by Thomas, the contents
are unchanged. I Hope I understood all the requests right.
Arnd Bergmann (2):
time: Add an asm-generic/compat.h file
sparc: compat: Allow including asm/compat.h for 32-bit
Deepa Dinamani (10):
compat: Make compat helpers independent of CONFIG_COMPAT
compat: Move compat_timespec/ timeval to compat_time.h
compat: Enable compat_get/put_timespec64 always
time: Introduce CONFIG_64BIT_TIME in architectures
time: Introduce CONFIG_COMPAT_32BIT_TIME
posix-timers: Make compat syscalls depend on CONFIG_COMPAT_32BIT_TIME
time: Add new y2038 safe __kernel_timespec
time: Fix get_timespec64() for y2038 safe compat interfaces
time: Change types to new y2038 safe __kernel_* types
time: Change nanosleep to safe __kernel_* types
arch/Kconfig | 15 +++++++++
arch/alpha/include/asm/Kbuild | 1 +
arch/arc/include/asm/Kbuild | 1 +
arch/arm/include/asm/Kbuild | 1 +
arch/arm64/include/asm/compat.h | 11 -------
arch/arm64/include/asm/stat.h | 1 +
arch/arm64/kernel/hw_breakpoint.c | 1 -
arch/arm64/kernel/perf_regs.c | 2 +-
arch/c6x/include/asm/Kbuild | 1 +
arch/h8300/include/asm/Kbuild | 1 +
arch/hexagon/include/asm/Kbuild | 1 +
arch/ia64/include/asm/Kbuild | 1 +
arch/m68k/include/asm/Kbuild | 1 +
arch/microblaze/include/asm/Kbuild | 1 +
arch/mips/include/asm/compat.h | 11 -------
arch/mips/kernel/signal32.c | 2 +-
arch/nds32/include/asm/Kbuild | 1 +
arch/nios2/include/asm/Kbuild | 1 +
arch/openrisc/include/asm/Kbuild | 1 +
arch/parisc/include/asm/compat.h | 11 -------
arch/powerpc/include/asm/compat.h | 11 -------
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/oprofile/backtrace.c | 1 +
arch/s390/hypfs/hypfs_sprp.c | 1 -
arch/s390/include/asm/compat.h | 11 -------
arch/s390/include/asm/elf.h | 4 +--
arch/s390/kvm/priv.c | 1 -
arch/s390/pci/pci_clp.c | 1 -
arch/sh/include/asm/Kbuild | 1 +
arch/sparc/include/asm/compat.h | 15 +++------
arch/um/include/asm/Kbuild | 1 +
arch/unicore32/include/asm/Kbuild | 1 +
arch/x86/events/core.c | 2 +-
arch/x86/include/asm/compat.h | 11 -------
arch/x86/include/asm/ftrace.h | 2 +-
arch/x86/kernel/sys_x86_64.c | 2 +-
arch/xtensa/include/asm/Kbuild | 1 +
drivers/s390/block/dasd_ioctl.c | 1 -
drivers/s390/char/fs3270.c | 1 -
drivers/s390/char/sclp_ctl.c | 1 -
drivers/s390/char/vmcp.c | 1 -
drivers/s390/cio/chsc_sch.c | 1 -
drivers/s390/net/qeth_core_main.c | 2 +-
include/asm-generic/compat.h | 3 ++
include/linux/compat.h | 12 ++++---
include/linux/compat_time.h | 23 ++++++++++++++
include/linux/restart_block.h | 7 ++--
include/linux/syscalls.h | 13 ++++----
include/linux/time.h | 4 +--
include/linux/time64.h | 10 +++++-
include/uapi/asm-generic/posix_types.h | 1 +
include/uapi/linux/time.h | 7 ++++
kernel/compat.c | 52 +++++-------------------------
kernel/time/hrtimer.c | 10 ++++--
kernel/time/posix-stubs.c | 12 ++++---
kernel/time/posix-timers.c | 24 ++++++++++----
kernel/time/time.c | 58 +++++++++++++++++++++++++++++++---
57 files changed, 201 insertions(+), 176 deletions(-)
create mode 100644 include/asm-generic/compat.h
create mode 100644 include/linux/compat_time.h
^ permalink raw reply
* Re: [tip GIT PULL] y2038: timekeeping syscall changes
From: Thomas Gleixner @ 2018-04-19 10:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: y2038 Mailman List, Deepa Dinamani, Linux Kernel Mailing List,
linux-arch, Linux API
In-Reply-To: <CAK8P3a2CEK=4Os4gbnXbR7VJJgaXVQ7NLjihr7rvMSK6g3_tNA@mail.gmail.com>
On Thu, 19 Apr 2018, Arnd Bergmann wrote:
> The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:
>
> Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)
>
> are available in the git repository at:
>
> git+ssh://gitolite@ra.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
> tags/y2038-timekeeping
Please don't use the +ssh address as that's only accessible for those who
have a k.org account.
git://git.kernel.org/
is accessible for everyone.
> ----------------------------------------------------------------
> Arnd Bergmann (2):
> compat: add an asm-generic/compat.h file
Please start the subject line sentence with an uppercase letter.
> sparc: compat: allow including asm/compat.h for 32-bit
>
> Deepa Dinamani (10):
> compat: Make compat helpers independent of CONFIG_COMPAT
> include: Move compat_timespec/ timeval to compat_time.h
The proper subsystem is 'time:' not 'include:'
> compat: enable compat_get/put_timespec64 always
> arch: introduce CONFIG_64BIT_TIME
> arch: Introduce CONFIG_COMPAT_32BIT_TIME
At least it should be consistent..
> posix-clocks: Make compat syscalls depend on CONFIG_COMPAT_32BIT_TIME
'posix-timers:' please
> include: Add new y2038 safe __kernel_timespec
'time:'
> fix get_timespec64() for y2038 safe compat interfaces
Bah. This lacks a subsystem, i.e. 'time:'
> change time types to new y2038 safe __kernel_* types
Ditto
> nanosleep: change time types to safe __kernel_* types
Can you please fix that and send me an updated pull request?
Thanks,
tglx
^ permalink raw reply
* [tip GIT PULL] y2038: timekeeping syscall changes
From: Arnd Bergmann @ 2018-04-19 9:35 UTC (permalink / raw)
To: Thomas Gleixner
Cc: y2038 Mailman List, Deepa Dinamani, Linux Kernel Mailing List,
linux-arch, Linux API
The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:
Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)
are available in the git repository at:
git+ssh://gitolite@ra.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
tags/y2038-timekeeping
for you to fetch changes up to 2e294d2c97d6683fcdf1134d87be1c461dff8ffd:
nanosleep: change time types to safe __kernel_* types (2018-04-19
10:07:58 +0200)
----------------------------------------------------------------
y2038: timekeeping syscall changes
This is the first set of system call entry point changes to enable 32-bit
architectures to have variants on both 32-bit and 64-bit time_t. Typically
these system calls take a 'struct timespec' argument, but that structure
is defined in user space by the C library and its layout will change.
The kernel already supports handling the 32-bit time_t on 64-bit
architectures through the CONFIG_COMPAT mechanism. As there are a total
of 51 system calls suffering from this problem, reusing that mechanism
on 32-bit architectures.
We already have patches for most of the remaining system calls, but this
set contains most of the complexity and is best tested. There was one
last-minute regression that prevented it from going into 4.17, but that
is fixed now.
More details from Deepa's patch series description:
Big picture is as per the lwn article:
https://lwn.net/Articles/643234/ [2]
The series is directed at converting posix clock syscalls:
clock_gettime, clock_settime, clock_getres and clock_nanosleep
to use a new data structure __kernel_timespec at syscall boundaries.
__kernel_timespec maintains 64 bit time_t across all execution modes.
vdso will be handled as part of each architecture when they enable
support for 64 bit time_t.
The compat syscalls are repurposed to provide backward compatibility
by using them as native syscalls as well for 32 bit architectures.
They will continue to use timespec at syscall boundaries.
CONFIG_64_BIT_TIME controls whether the syscalls use __kernel_timespec
or timespec at syscall boundaries.
The series does the following:
1. Enable compat syscalls on 32 bit architectures.
2. Add a new __kernel_timespec type to be used as the data structure
for all the new syscalls.
3. Add new config CONFIG_64BIT_TIME(intead of the CONFIG_COMPAT_TIME in
[1] and [2] to switch to new definition of __kernel_timespec. It is
the same as struct timespec otherwise.
4. Add new CONFIG_32BIT_TIME to conditionally compile compat syscalls.
----------------------------------------------------------------
Arnd Bergmann (2):
compat: add an asm-generic/compat.h file
sparc: compat: allow including asm/compat.h for 32-bit
Deepa Dinamani (10):
compat: Make compat helpers independent of CONFIG_COMPAT
include: Move compat_timespec/ timeval to compat_time.h
compat: enable compat_get/put_timespec64 always
arch: introduce CONFIG_64BIT_TIME
arch: Introduce CONFIG_COMPAT_32BIT_TIME
posix-clocks: Make compat syscalls depend on CONFIG_COMPAT_32BIT_TIME
include: Add new y2038 safe __kernel_timespec
fix get_timespec64() for y2038 safe compat interfaces
change time types to new y2038 safe __kernel_* types
nanosleep: change time types to safe __kernel_* types
arch/Kconfig | 15 +++++++++
arch/alpha/include/asm/Kbuild | 1 +
arch/arc/include/asm/Kbuild | 1 +
arch/arm/include/asm/Kbuild | 1 +
arch/arm64/include/asm/compat.h | 11 -------
arch/arm64/include/asm/stat.h | 1 +
arch/arm64/kernel/hw_breakpoint.c | 1 -
arch/arm64/kernel/perf_regs.c | 2 +-
arch/c6x/include/asm/Kbuild | 1 +
arch/h8300/include/asm/Kbuild | 1 +
arch/hexagon/include/asm/Kbuild | 1 +
arch/ia64/include/asm/Kbuild | 1 +
arch/m68k/include/asm/Kbuild | 1 +
arch/microblaze/include/asm/Kbuild | 1 +
arch/mips/include/asm/compat.h | 11 -------
arch/mips/kernel/signal32.c | 2 +-
arch/nds32/include/asm/Kbuild | 1 +
arch/nios2/include/asm/Kbuild | 1 +
arch/openrisc/include/asm/Kbuild | 1 +
arch/parisc/include/asm/compat.h | 11 -------
arch/powerpc/include/asm/compat.h | 11 -------
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/oprofile/backtrace.c | 1 +
arch/s390/hypfs/hypfs_sprp.c | 1 -
arch/s390/include/asm/compat.h | 11 -------
arch/s390/include/asm/elf.h | 4 +--
arch/s390/kvm/priv.c | 1 -
arch/s390/pci/pci_clp.c | 1 -
arch/sh/include/asm/Kbuild | 1 +
arch/sparc/include/asm/compat.h | 15 +++------
arch/um/include/asm/Kbuild | 1 +
arch/unicore32/include/asm/Kbuild | 1 +
arch/x86/events/core.c | 2 +-
arch/x86/include/asm/compat.h | 11 -------
arch/x86/include/asm/ftrace.h | 2 +-
arch/x86/kernel/sys_x86_64.c | 2 +-
arch/xtensa/include/asm/Kbuild | 1 +
drivers/s390/block/dasd_ioctl.c | 1 -
drivers/s390/char/fs3270.c | 1 -
drivers/s390/char/sclp_ctl.c | 1 -
drivers/s390/char/vmcp.c | 1 -
drivers/s390/cio/chsc_sch.c | 1 -
drivers/s390/net/qeth_core_main.c | 2 +-
include/asm-generic/compat.h | 3 ++
include/linux/compat.h | 12 ++++---
include/linux/compat_time.h | 23 ++++++++++++++
include/linux/restart_block.h | 7 ++--
include/linux/syscalls.h | 13 ++++----
include/linux/time.h | 4 +--
include/linux/time64.h | 10 +++++-
include/uapi/asm-generic/posix_types.h | 1 +
include/uapi/linux/time.h | 7 ++++
kernel/compat.c | 52 +++++-------------------------
kernel/time/hrtimer.c | 10 ++++--
kernel/time/posix-stubs.c | 12 ++++---
kernel/time/posix-timers.c | 24 ++++++++++----
kernel/time/time.c | 58 +++++++++++++++++++++++++++++++---
57 files changed, 201 insertions(+), 176 deletions(-)
create mode 100644 include/asm-generic/compat.h
create mode 100644 include/linux/compat_time.h
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Paul Moore @ 2018-04-19 2:10 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <66adde01c1dda792aff99a457eea576a0b08ca98.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add container ID auxiliary record(s) to NETFILTER_PKT event standalone
> records. Iterate through all potential container IDs associated with a
> network namespace.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit.c | 1 +
> kernel/auditsc.c | 2 ++
> net/netfilter/xt_AUDIT.c | 15 ++++++++++++++-
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 08662b4..3c77e47 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2102,6 +2102,7 @@ int audit_log_container_info(struct audit_context *context,
> audit_log_end(ab);
> return 0;
> }
> +EXPORT_SYMBOL(audit_log_container_info);
>
> void audit_log_key(struct audit_buffer *ab, char *key)
> {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 208da962..af68d01 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -975,6 +975,7 @@ struct audit_context *audit_alloc_local(void)
> context->in_syscall = 1;
> return context;
> }
> +EXPORT_SYMBOL(audit_alloc_local);
>
> inline void audit_free_context(struct audit_context *context)
> {
> @@ -989,6 +990,7 @@ inline void audit_free_context(struct audit_context *context)
> audit_proctitle_free(context);
> kfree(context);
> }
> +EXPORT_SYMBOL(audit_free_context);
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> kuid_t auid, kuid_t uid, unsigned int sessionid,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index c502419..edaa456 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,14 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context = audit_alloc_local();
> + struct audit_containerid *cont;
> + int i = 0;
> + struct net *net;
>
> if (audit_enabled == 0)
> goto errout;
Do I need to say it? I probably should ... the allocation should
happen after the audit_enabled check.
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +108,16 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = sock_net(NETLINK_CB(skb).sk);
> + list_for_each_entry(cont, &net->audit_containerid, list) {
> + char buf[14];
> +
> + sprintf(buf, "net%u", i++);
> + audit_log_container_info(context, buf, cont->id);
> + }
It seems like this could (should?) be hidden inside an audit function,
e.g. audit_log_net_containers() or something like that.
> errout:
> + audit_free_context(context);
> return XT_CONTINUE;
> }
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
From: Paul Moore @ 2018-04-19 1:46 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <11b43a498e768a14764594c808a96b34d52be0af.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Audit events could happen in a network namespace outside of a task
> context due to packets received from the net that trigger an auditing
> rule prior to being associated with a running task. The network
> namespace could in use by multiple containers by association to the
> tasks in that network namespace. We still want a way to attribute
> these events to any potential containers. Keep a list per network
> namespace to track these container identifiiers.
>
> Add/increment the container identifier on:
> - initial setting of the container id via /proc
> - clone/fork call that inherits a container identifier
> - unshare call that inherits a container identifier
> - setns call that inherits a container identifier
> Delete/decrement the container identifier on:
> - an inherited container id dropped when child set
> - process exit
> - unshare call that drops a net namespace
> - setns call that drops a net namespace
>
> See: https://github.com/linux-audit/audit-kernel/issues/32
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 7 +++++++
> include/net/net_namespace.h | 12 ++++++++++++
> kernel/auditsc.c | 9 ++++++---
> kernel/nsproxy.c | 6 ++++++
> net/core/net_namespace.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 76 insertions(+), 3 deletions(-)
...
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index 0490084..343a428 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -33,6 +33,7 @@
> #include <linux/ns_common.h>
> #include <linux/idr.h>
> #include <linux/skbuff.h>
> +#include <linux/audit.h>
>
> struct user_namespace;
> struct proc_dir_entry;
> @@ -150,6 +151,7 @@ struct net {
> #endif
> struct sock *diag_nlsk;
> atomic_t fnhe_genid;
> + struct list_head audit_containerid;
> } __randomize_layout;
We talked about this briefly off-list, you should be using audit_net
and the net_generic mechanism instead of this.
> #include <linux/seq_file_net.h>
> @@ -301,6 +303,16 @@ static inline struct net *read_pnet(const possible_net_t *pnet)
> #define __net_initconst __initconst
> #endif
>
> +#ifdef CONFIG_NET_NS
> +void net_add_audit_containerid(struct net *net, u64 containerid);
> +void net_del_audit_containerid(struct net *net, u64 containerid);
> +#else
> +static inline void net_add_audit_containerid(struct net *, u64)
> +{ }
> +static inline void net_del_audit_containerid(struct net *, u64)
> +{ }
> +#endif
> +
> int peernet2id_alloc(struct net *net, struct net *peer);
> int peernet2id(struct net *net, struct net *peer);
> bool peernet_has_id(struct net *net, struct net *peer);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2f02ed9..208da962 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -75,6 +75,7 @@
> #include <linux/uaccess.h>
> #include <linux/fsnotify_backend.h>
> #include <uapi/linux/limits.h>
> +#include <net/net_namespace.h>
>
> #include "audit.h"
>
> @@ -2175,16 +2176,18 @@ static void audit_log_set_containerid(struct task_struct *task, u64 oldcontainer
> */
> int audit_set_containerid(struct task_struct *task, u64 containerid)
> {
> - u64 oldcontainerid;
> + u64 oldcontainerid = audit_get_containerid(task);
> int rc;
> -
> - oldcontainerid = audit_get_containerid(task);
> + struct net *net = task->nsproxy->net_ns;
>
> rc = audit_set_containerid_perm(task, containerid);
> if (!rc) {
> + if (cid_valid(oldcontainerid))
> + net_del_audit_containerid(net, oldcontainerid);
Using audit_net we can handle this internal to audit, which is a Good Thing.
> task_lock(task);
> task->containerid = containerid;
> task_unlock(task);
> + net_add_audit_containerid(net, containerid);
Same.
> }
>
> audit_log_set_containerid(task, oldcontainerid, containerid, rc);
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d33..d9f1090 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -140,6 +140,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> struct nsproxy *old_ns = tsk->nsproxy;
> struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> struct nsproxy *new_ns;
> + u64 containerid = audit_get_containerid(tsk);
>
> if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> CLONE_NEWPID | CLONE_NEWNET |
> @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + net_add_audit_containerid(new_ns->net_ns, containerid);
> return 0;
> }
Hopefully we can handle this in audit_net_init(), we just need to
figure out where we can get the correct task_struct for the audit
container ID (some backpointer in the net struct?).
> @@ -217,6 +219,7 @@ int unshare_nsproxy_namespaces(unsigned long unshare_flags,
> void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> {
> struct nsproxy *ns;
> + u64 containerid = audit_get_containerid(p);
>
> might_sleep();
>
> @@ -224,6 +227,9 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + net_del_audit_containerid(ns->net_ns, containerid);
> + if (new)
> + net_add_audit_containerid(new->net_ns, containerid);
Okay, we might need a hook here for switching namespaces, but I would
much rather it be a generic audit hook that calls directly into audit.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 10/13] audit: add containerid support for seccomp and anom_abend records
From: Paul Moore @ 2018-04-19 1:31 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <11174597083f89352f1d6491ec94e27f882625d9.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add container ID auxiliary records to secure computing and abnormal end
> standalone records.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/auditsc.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 7103d23..2f02ed9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2571,6 +2571,7 @@ static void audit_log_task(struct audit_buffer *ab)
> void audit_core_dumps(long signr)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
Looking quickly at do_coredump() I *believe* we can use current here.
> if (!audit_enabled)
> return;
> @@ -2578,19 +2579,22 @@ void audit_core_dumps(long signr)
> if (signr == SIGQUIT) /* don't care for those */
> return;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_ANOM_ABEND);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> audit_log_format(ab, " sig=%ld res=1", signr);
> audit_log_end(ab);
> + audit_log_container_info(context, "abend", audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> void __audit_seccomp(unsigned long syscall, long signr, int code)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
We can definitely use current here.
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_SECCOMP);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_SECCOMP);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> @@ -2598,6 +2602,8 @@ void __audit_seccomp(unsigned long syscall, long signr, int code)
> signr, syscall_get_arch(), syscall,
> in_compat_syscall(), KSTK_EIP(current), code);
> audit_log_end(ab);
> + audit_log_container_info(context, "seccomp", audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> struct list_head *audit_killed_trees(void)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records
From: Paul Moore @ 2018-04-19 1:27 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <c34a7a95eb045a62e2443457979db9d7afbd9aee.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add container ID auxiliary records to configuration change, feature set change
> and user generated standalone records.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> 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(&init_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(&ab, msg_type);
> + audit_log_common_recv_msg(context, &ab, 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(&ab, AUDIT_CONFIG_CHANGE);
> + struct audit_context *context = audit_alloc_local();
Pretty sure current can be used here too. In fact I think everywhere
where we are processing commands from netlink we can use current as I
believe the entire netlink stack is processed in the context of the
caller.
> + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE);
> audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
> audit_log_end(ab);
> + audit_log_container_info(context, "config",
> + audit_get_containerid(current));
> + audit_free_context(context);
> return -EPERM;
> }
> err = audit_rule_change(msg_type, seq, data, nlmsg_len(nlh));
> @@ -1376,17 +1393,23 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> case AUDIT_LIST_RULES:
> err = audit_list_rules_send(skb, seq);
> break;
> - case AUDIT_TRIM:
> + case AUDIT_TRIM: {
> + struct audit_context *context = audit_alloc_local();
Same.
> audit_trim_trees();
> - audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE);
> audit_log_format(ab, " op=trim res=1");
> audit_log_end(ab);
> + audit_log_container_info(context, "config",
> + audit_get_containerid(current));
> + audit_free_context(context);
> break;
> + }
> case AUDIT_MAKE_EQUIV: {
> void *bufp = data;
> u32 sizes[2];
> size_t msglen = nlmsg_len(nlh);
> char *old, *new;
> + struct audit_context *context = audit_alloc_local();
Same.
> err = -EINVAL;
> if (msglen < 2 * sizeof(u32))
> @@ -1408,7 +1431,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> /* OK, here comes... */
> err = audit_tag_tree(old, new);
>
> - audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE);
>
> audit_log_format(ab, " op=make_equiv old=");
> audit_log_untrustedstring(ab, old);
> @@ -1418,6 +1441,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> audit_log_end(ab);
> kfree(old);
> kfree(new);
> + audit_log_container_info(context, "config",
> + audit_get_containerid(current));
> + audit_free_context(context);
> break;
> }
> case AUDIT_SIGNAL_INFO:
> @@ -1459,6 +1485,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> struct audit_tty_status s, old;
> struct audit_buffer *ab;
> unsigned int t;
> + struct audit_context *context = audit_alloc_local();
Same.
> memset(&s, 0, sizeof(s));
> /* guard against past and future API changes */
> @@ -1477,12 +1504,15 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> old.enabled = t & AUDIT_TTY_ENABLE;
> old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD);
>
> - audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE);
> audit_log_format(ab, " op=tty_set old-enabled=%d new-enabled=%d"
> " old-log_passwd=%d new-log_passwd=%d res=%d",
> old.enabled, s.enabled, old.log_passwd,
> s.log_passwd, !err);
> audit_log_end(ab);
> + audit_log_container_info(context, "config",
> + audit_get_containerid(current));
> + audit_free_context(context);
> break;
> }
> default:
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index c4c8746..5f7f4d6 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1109,11 +1109,12 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
> struct audit_buffer *ab;
> uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(current));
> unsigned int sessionid = audit_get_sessionid(current);
> + struct audit_context *context = audit_alloc_local();
>
> if (!audit_enabled)
> return;
Well, first I think we should be able to get rid of the local context,
but if for some reason we can't use current->audit_context then do the
allocation after the audit_enabled check.
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (!ab)
> return;
> audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
> @@ -1122,6 +1123,8 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
> audit_log_end(ab);
> + audit_log_container_info(context, "config", audit_get_containerid(current));
> + audit_free_context(context);
> }
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Casey Schaufler @ 2018-04-19 1:15 UTC (permalink / raw)
To: Paul Moore
Cc: Richard Guy Briggs, cgroups, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev, ebiederm,
luto, jlayton, carlos, dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhTz-pr-iUVv-+R3ShwEKSHDsweDGuN7255HV7Cu3ZYPEw@mail.gmail.com>
On 4/18/2018 5:46 PM, Paul Moore wrote:
> On Wed, Apr 18, 2018 at 8:41 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/18/2018 4:47 PM, Paul Moore wrote:
>>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>>>> Implement the proc fs write to set the audit container ID of a process,
>>>> emitting an AUDIT_CONTAINER record to document the event.
>>>> ...
>>>>
>>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>>> index d258826..1b82191 100644
>>>> --- a/include/linux/sched.h
>>>> +++ b/include/linux/sched.h
>>>> @@ -796,6 +796,7 @@ struct task_struct {
>>>> #ifdef CONFIG_AUDITSYSCALL
>>>> kuid_t loginuid;
>>>> unsigned int sessionid;
>>>> + u64 containerid;
>>> This one line addition to the task_struct scares me the most of
>>> anything in this patchset. Why? It's a field named "containerid" in
>>> a perhaps one of the most widely used core kernel structures; the
>>> possibilities for abuse are endless, and it's foolish to think we
>>> would ever be able to adequately police this.
>> If we can get the LSM infrastructure managed task blobs from
>> module stacking in ahead of this we could create a trivial security
>> module to manage this. It's not as if there aren't all sorts of
>> interactions between security modules and the audit system already.
> While yes, there are plenty of interactions between the two, it is
> possible to use audit without the LSMs and I would like to preserve
> that.
Fair enough.
> Further, I don't want to entangle two very complicated code
> changes or make the audit container ID effort dependent on LSM
> stacking.
Also fair, although the use case for container audit IDs is
already pulling in audit, namespaces (yeah, I know it's not
necessary for a container to use namespaces) security modules
(stacked and/or namespaced), cgroups and who knows what else.
> You're a good salesman Casey, but you're not that good ;)
I have to keep the skills sharpened somehow!
OK, I'll grant that this isn't a great fit.
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Paul Moore @ 2018-04-19 0:46 UTC (permalink / raw)
To: Casey Schaufler
Cc: simo-H+wXaHxf7aLQT0dZR+AlfA, jlayton-H+wXaHxf7aLQT0dZR+AlfA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
Eric Paris, dhowells-H+wXaHxf7aLQT0dZR+AlfA,
carlos-H+wXaHxf7aLQT0dZR+AlfA, Linux-Audit Mailing List,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, luto-DgEjT+Ai2ygdnm+yROfE0A,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
In-Reply-To: <32d3e7a6-36f0-571a-bb91-67f746c7eafa-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org>
On Wed, Apr 18, 2018 at 8:41 PM, Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote:
> On 4/18/2018 4:47 PM, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> Implement the proc fs write to set the audit container ID of a process,
>>> emitting an AUDIT_CONTAINER record to document the event.
>>> ...
>>>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index d258826..1b82191 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -796,6 +796,7 @@ struct task_struct {
>>> #ifdef CONFIG_AUDITSYSCALL
>>> kuid_t loginuid;
>>> unsigned int sessionid;
>>> + u64 containerid;
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset. Why? It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> If we can get the LSM infrastructure managed task blobs from
> module stacking in ahead of this we could create a trivial security
> module to manage this. It's not as if there aren't all sorts of
> interactions between security modules and the audit system already.
While yes, there are plenty of interactions between the two, it is
possible to use audit without the LSMs and I would like to preserve
that. Further, I don't want to entangle two very complicated code
changes or make the audit container ID effort dependent on LSM
stacking.
You're a good salesman Casey, but you're not that good ;)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 07/13] audit: add container aux record to watch/tree/mark
From: Paul Moore @ 2018-04-19 0:42 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <737f914a88d048b9985984c0ce1f946c30ca374c.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add container ID auxiliary record to mark, watch and tree rule
> configuration standalone records.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit_fsnotify.c | 5 ++++-
> kernel/audit_tree.c | 5 ++++-
> kernel/audit_watch.c | 33 +++++++++++++++++++--------------
> 3 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 52f368b..18c110d 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -124,10 +124,11 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
> {
> struct audit_buffer *ab;
> struct audit_krule *rule = audit_mark->rule;
> + struct audit_context *context = audit_alloc_local();
>
> if (!audit_enabled)
> return;
Move the audit_alloc_local() after the audit_enabled check.
> - ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> + ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> audit_log_format(ab, "auid=%u ses=%u op=%s",
> @@ -138,6 +139,8 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> audit_log_end(ab);
> + audit_log_container_info(context, "config", audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 67e6956..7c085be 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -496,8 +496,9 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
> static void audit_tree_log_remove_rule(struct audit_krule *rule)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
Sort of independent of the audit container ID work, but shouldn't we
have an audit_enabled check here?
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> audit_log_format(ab, "op=remove_rule");
> @@ -506,6 +507,8 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> audit_log_end(ab);
> + audit_log_container_info(context, "config", audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> static void kill_rules(struct audit_tree *tree)
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 9eb8b35..60d75a2 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -238,20 +238,25 @@ static struct audit_watch *audit_dupe_watch(struct audit_watch *old)
>
> static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watch *w, char *op)
> {
> - if (audit_enabled) {
> - struct audit_buffer *ab;
> - ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> - if (unlikely(!ab))
> - return;
> - audit_log_format(ab, "auid=%u ses=%u op=%s",
> - from_kuid(&init_user_ns, audit_get_loginuid(current)),
> - audit_get_sessionid(current), op);
> - audit_log_format(ab, " path=");
> - audit_log_untrustedstring(ab, w->path);
> - audit_log_key(ab, r->filterkey);
> - audit_log_format(ab, " list=%d res=1", r->listnr);
> - audit_log_end(ab);
> - }
> + struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
> +
> + if (!audit_enabled)
> + return;
Same as above, do the allocation after the audit_enabled check.
> + ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> + if (unlikely(!ab))
> + return;
> + audit_log_format(ab, "auid=%u ses=%u op=%s",
> + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> + audit_get_sessionid(current), op);
> + audit_log_format(ab, " path=");
> + audit_log_untrustedstring(ab, w->path);
> + audit_log_key(ab, r->filterkey);
> + audit_log_format(ab, " list=%d res=1", r->listnr);
> + audit_log_end(ab);
> + audit_log_container_info(context, "config", audit_get_containerid(current));
> + audit_free_context(context);
> }
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Casey Schaufler @ 2018-04-19 0:41 UTC (permalink / raw)
To: Paul Moore, Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhTyvxxj2e2Gn+iyW6iLLeYB7hp8a+JvfeMmJ2nUPqtEaw@mail.gmail.com>
On 4/18/2018 4:47 PM, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> Implement the proc fs write to set the audit container ID of a process,
>> emitting an AUDIT_CONTAINER record to document the event.
>> ...
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index d258826..1b82191 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -796,6 +796,7 @@ struct task_struct {
>> #ifdef CONFIG_AUDITSYSCALL
>> kuid_t loginuid;
>> unsigned int sessionid;
>> + u64 containerid;
> This one line addition to the task_struct scares me the most of
> anything in this patchset. Why? It's a field named "containerid" in
> a perhaps one of the most widely used core kernel structures; the
> possibilities for abuse are endless, and it's foolish to think we
> would ever be able to adequately police this.
If we can get the LSM infrastructure managed task blobs from
module stacking in ahead of this we could create a trivial security
module to manage this. It's not as if there aren't all sorts of
interactions between security modules and the audit system already.
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 06/13] audit: add support for non-syscall auxiliary records
From: Paul Moore @ 2018-04-19 0:39 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <ee2a945fb09a939b3c214f45e49dab6a770d83e6.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> 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.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 8 ++++++++
> kernel/auditsc.c | 20 +++++++++++++++++++-
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index ed16bb6..c0b83cb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -227,7 +227,9 @@ static inline int audit_log_container_info(struct audit_context *context,
> /* These are defined in auditsc.c */
> /* Public API */
> extern int audit_alloc(struct task_struct *task);
> +extern struct audit_context *audit_alloc_local(void);
> extern void __audit_free(struct task_struct *task);
> +extern void audit_free_context(struct audit_context *context);
> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> unsigned long a2, unsigned long a3);
> extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -472,6 +474,12 @@ static inline int audit_alloc(struct task_struct *task)
> {
> return 0;
> }
> +static inline struct audit_context *audit_alloc_local(void)
> +{
> + return NULL;
> +}
> +static inline void audit_free_context(struct audit_context *context)
> +{ }
> static inline void audit_free(struct task_struct *task)
> { }
> static inline void audit_syscall_entry(int major, unsigned long a0,
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2932ef1..7103d23 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -959,8 +959,26 @@ int audit_alloc(struct task_struct *tsk)
> return 0;
> }
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(void)
> {
> + struct audit_context *context;
> +
> + if (!audit_ever_enabled)
> + return NULL; /* Return if not auditing. */
> +
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> + if (!context)
> + return NULL;
> + context->serial = audit_serial();
> + context->ctime = current_kernel_time64();
> + context->in_syscall = 1;
> + return context;
> +}
> +
> +inline void audit_free_context(struct audit_context *context)
> +{
> + if (!context)
> + return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
I'm reserving the option to comment on this idea further as I make my
way through the patchset, but audit_free_context() definitely
shouldn't be declared as an inline function.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals
From: Paul Moore @ 2018-04-19 0:32 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <8c7ff567377f4a83edac48e962c1b5b824b523c8.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add container ID support to ptrace and signals. In particular, the "op"
> field provides a way to label the auxiliary record to which it is
> associated.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 16 +++++++++++-----
> kernel/audit.c | 12 ++++++++----
> kernel/audit.h | 2 ++
> kernel/auditsc.c | 19 +++++++++++++++----
> 4 files changed, 36 insertions(+), 13 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a12f21f..b238be5 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -142,6 +142,7 @@ struct audit_net {
> kuid_t audit_sig_uid = INVALID_UID;
> pid_t audit_sig_pid = -1;
> u32 audit_sig_sid = 0;
> +u64 audit_sig_cid = INVALID_CID;
>
> /* Records can be lost in several ways:
> 0) [suppressed in audit_alloc]
> @@ -1438,6 +1439,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> memcpy(sig_data->ctx, ctx, len);
> security_release_secctx(ctx, len);
> }
> + sig_data->cid = audit_sig_cid;
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> kfree(sig_data);
> @@ -2051,20 +2053,22 @@ void audit_log_session_info(struct audit_buffer *ab)
>
> /*
> * audit_log_container_info - report container info
> - * @tsk: task to be recorded
> * @context: task or local context for record
> + * @op: containerid string description
> + * @containerid: container ID to report
> */
> -int audit_log_container_info(struct task_struct *tsk, struct audit_context *context)
> +int audit_log_container_info(struct audit_context *context,
> + char *op, u64 containerid)
> {
> struct audit_buffer *ab;
>
> - if (!audit_containerid_set(tsk))
> + if (!cid_valid(containerid))
> return 0;
> /* Generate AUDIT_CONTAINER_INFO with container ID */
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_INFO);
> if (!ab)
> return -ENOMEM;
> - audit_log_format(ab, "contid=%llu", audit_get_containerid(tsk));
> + audit_log_format(ab, "op=%s contid=%llu", op, containerid);
> audit_log_end(ab);
> return 0;
> }
Let's get these changes into the first patch where
audit_log_container_info() is defined. Why? This inserts a new field
into the record which is a no-no. Yes, it is one single patchset, but
they are still separate patches and who knows which patches a given
distribution and/or tree may decide to backport.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2bba324..2932ef1 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -113,6 +113,7 @@ struct audit_aux_data_pids {
> kuid_t target_uid[AUDIT_AUX_PIDS];
> unsigned int target_sessionid[AUDIT_AUX_PIDS];
> u32 target_sid[AUDIT_AUX_PIDS];
> + u64 target_cid[AUDIT_AUX_PIDS];
> char target_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN];
> int pid_count;
> };
> @@ -1422,21 +1423,27 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
> for (aux = context->aux_pids; aux; aux = aux->next) {
> struct audit_aux_data_pids *axs = (void *)aux;
>
> - for (i = 0; i < axs->pid_count; i++)
> + for (i = 0; i < axs->pid_count; i++) {
> + char axsn[sizeof("aux0xN ")];
> +
> + sprintf(axsn, "aux0x%x", i);
> if (audit_log_pid_context(context, axs->target_pid[i],
> axs->target_auid[i],
> axs->target_uid[i],
> axs->target_sessionid[i],
> axs->target_sid[i],
> - axs->target_comm[i]))
> + axs->target_comm[i])
> + && audit_log_container_info(context, axsn, axs->target_cid[i]))
Shouldn't this be an OR instead of an AND?
> call_panic = 1;
> + }
> }
>
> if (context->target_pid &&
> audit_log_pid_context(context, context->target_pid,
> context->target_auid, context->target_uid,
> context->target_sessionid,
> - context->target_sid, context->target_comm))
> + context->target_sid, context->target_comm)
> + && audit_log_container_info(context, "target", context->target_cid))
Same question.
> call_panic = 1;
>
> if (context->pwd.dentry && context->pwd.mnt) {
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering
From: Paul Moore @ 2018-04-19 0:24 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <b933f93762435990e9b1e6d5aebf15f186ac8951.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Implement container ID filtering using the AUDIT_CONTAINERID field name
> to send an 8-character string representing a u64 since the value field
> is only u32.
>
> Sending it as two u32 was considered, but gathering and comparing two
> fields was more complex.
My only worry here is that you aren't really sending a string in the
ASCII sense, you are sending an 8 byte buffer (that better be NUL
terminated) that happens to be an unsigned 64-bit integer. To be
clear, I'm okay with that (it's protected by AUDIT_CONTAINERID), and
the code is okay with that, I just want us to pause for a minute and
make sure that is an okay thing to do long term.
> The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER.
>
> This requires support from userspace to be useful.
> See: https://github.com/linux-audit/audit-userspace/issues/40
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 1 +
> include/uapi/linux/audit.h | 5 ++++-
> kernel/audit.h | 1 +
> kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> kernel/auditsc.c | 3 +++
> 5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3acbe9d..f10ca1b 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -76,6 +76,7 @@ struct audit_field {
> u32 type;
> union {
> u32 val;
> + u64 val64;
> kuid_t uid;
> kgid_t gid;
> struct {
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index e83ccbd..8443a8f 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -262,6 +262,7 @@
> #define AUDIT_LOGINUID_SET 24
> #define AUDIT_SESSIONID 25 /* Session ID */
> #define AUDIT_FSTYPE 26 /* FileSystem Type */
> +#define AUDIT_CONTAINERID 27 /* Container ID */
>
> /* These are ONLY useful when checking
> * at syscall exit time (AUDIT_AT_EXIT). */
> @@ -342,6 +343,7 @@ enum {
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> +#define AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER 0x00000080
>
> #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> @@ -349,7 +351,8 @@ enum {
> AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> AUDIT_FEATURE_BITMAP_LOST_RESET | \
> - AUDIT_FEATURE_BITMAP_FILTER_FS)
> + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> + AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 214e149..aaa651a 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -234,6 +234,7 @@ static inline int audit_hash_ino(u32 ino)
>
> extern int audit_match_class(int class, unsigned syscall);
> extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> extern int parent_len(const char *path);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index d7a807e..c4c8746 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> /* FALL THROUGH */
> case AUDIT_ARCH:
> case AUDIT_FSTYPE:
> + case AUDIT_CONTAINERID:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -584,6 +585,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> }
> entry->rule.exe = audit_mark;
> break;
> + case AUDIT_CONTAINERID:
> + if (f->val != sizeof(u64))
> + goto exit_free;
> + str = audit_unpack_string(&bufp, &remain, f->val);
> + if (IS_ERR(str))
> + goto exit_free;
> + f->val64 = ((u64 *)str)[0];
> + break;
> }
> }
>
> @@ -666,6 +675,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> data->buflen += data->values[i] =
> audit_pack_string(&bufp, audit_mark_path(krule->exe));
> break;
> + case AUDIT_CONTAINERID:
> + data->buflen += data->values[i] = sizeof(u64);
> + for (i = 0; i < sizeof(u64); i++)
> + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> + break;
> case AUDIT_LOGINUID_SET:
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> data->fields[i] = AUDIT_LOGINUID;
> @@ -752,6 +766,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> return 1;
> break;
> + case AUDIT_CONTAINERID:
> + if (a->fields[i].val64 != b->fields[i].val64)
> + return 1;
> + break;
> default:
> if (a->fields[i].val != b->fields[i].val)
> return 1;
> @@ -1210,6 +1228,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> }
> }
>
> +int audit_comparator64(u64 left, u32 op, u64 right)
> +{
> + switch (op) {
> + case Audit_equal:
> + return (left == right);
> + case Audit_not_equal:
> + return (left != right);
> + case Audit_lt:
> + return (left < right);
> + case Audit_le:
> + return (left <= right);
> + case Audit_gt:
> + return (left > right);
> + case Audit_ge:
> + return (left >= right);
> + case Audit_bitmask:
> + return (left & right);
> + case Audit_bittest:
> + return ((left & right) == right);
> + default:
> + BUG();
> + return 0;
> + }
> +}
> +
> int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> {
> switch (op) {
> @@ -1348,6 +1391,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> result = audit_comparator(audit_loginuid_set(current),
> f->op, f->val);
> break;
> + case AUDIT_CONTAINERID:
> + result = audit_comparator64(audit_get_containerid(current),
> + f->op, f->val64);
> + break;
> case AUDIT_MSGTYPE:
> result = audit_comparator(msgtype, f->op, f->val);
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 65be110..2bba324 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -614,6 +614,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> break;
> + case AUDIT_CONTAINERID:
> + result = audit_comparator64(audit_get_containerid(tsk), f->op, f->val64);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox