Re: [PATCH ghak84 v4] audit: purge audit_log_string from the intra-kernel audit API
On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs wrote:
> On 2020-07-14 16:29, Paul Moore wrote:
> > On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs wrote:
> > > On 2020-07-14 12:21, Paul Moore wrote:
> > > > On Mon, Jul 13, 2020 at 3:52 PM Richard Guy Briggs
> > > > wrote:
> > > > >
> > > > > audit_log_string() was inteded to be an internal audit function and
> > > > > since there are only two internal uses, remove them. Purge all
> > > > > external
> > > > > uses of it by restructuring code to use an existing audit_log_format()
> > > > > or using audit_log_format().
> > > > >
> > > > > Please see the upstream issue
> > > > > https://github.com/linux-audit/audit-kernel/issues/84
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs
> > > > > ---
> > > > > Passes audit-testsuite.
> > > > >
> > > > > Changelog:
> > > > > v4
> > > > > - use double quotes in all replaced audit_log_string() calls
> > > > >
> > > > > v3
> > > > > - fix two warning: non-void function does not return a value in all
> > > > > control paths
> > > > > Reported-by: kernel test robot
> > > > >
> > > > > v2
> > > > > - restructure to piggyback on existing audit_log_format() calls,
> > > > > checking quoting needs for each.
> > > > >
> > > > > v1 Vlad Dronov
> > > > > -
> > > > > https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf
> > > > >
> > > > > include/linux/audit.h | 5 -
> > > > > kernel/audit.c| 4 ++--
> > > > > security/apparmor/audit.c | 10 --
> > > > > security/apparmor/file.c | 25 +++--
> > > > > security/apparmor/ipc.c | 46
> > > > > +++---
> > > > > security/apparmor/net.c | 14 --
> > > > > security/lsm_audit.c | 4 ++--
> > > > > 7 files changed, 46 insertions(+), 62 deletions(-)
> > > >
> > > > Thanks for restoring the quotes, just one question below ...
> > > >
> > > > > diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
> > > > > index 4ecedffbdd33..fe36d112aad9 100644
> > > > > --- a/security/apparmor/ipc.c
> > > > > +++ b/security/apparmor/ipc.c
> > > > > @@ -20,25 +20,23 @@
> > > > >
> > > > > /**
> > > > > * audit_ptrace_mask - convert mask to permission string
> > > > > - * @buffer: buffer to write string to (NOT NULL)
> > > > > * @mask: permission mask to convert
> > > > > + *
> > > > > + * Returns: pointer to static string
> > > > > */
> > > > > -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask)
> > > > > +static const char *audit_ptrace_mask(u32 mask)
> > > > > {
> > > > > switch (mask) {
> > > > > case MAY_READ:
> > > > > - audit_log_string(ab, "read");
> > > > > - break;
> > > > > + return "read";
> > > > > case MAY_WRITE:
> > > > > - audit_log_string(ab, "trace");
> > > > > - break;
> > > > > + return "trace";
> > > > > case AA_MAY_BE_READ:
> > > > > - audit_log_string(ab, "readby");
> > > > > - break;
> > > > > + return "readby";
> > > > > case AA_MAY_BE_TRACED:
> > > > > - audit_log_string(ab, "tracedby");
> > > > > - break;
> > > > > + return "tracedby";
> > > > > }
> > > > > + return "";
> > > >
> > > > Are we okay with this returning an empty string ("") in this case?
> > > > Should it be a question mark ("?")?
> > > >
> > > > My guess is that userspace parsing should be okay since it still has
> > > > quotes, I'm just not sure if we wanted to use a question mark as we do
> > > > in other cases where the field value is empty/unknown.
> > >
> > > Previously, it would have been an empty value, not even double quotes.
> > > "?" might be an improvement.
> >
> > Did you want to fix that now in this patch, or leave it to later? As
> > I said above, I'm not too bothered by it with the quotes so either way
> > is fine by me.
>
> I'd defer to Steve, otherwise I'd say leave it, since there wasn't
> anything there before and this makes that more evident.
>
> > John, I'm assuming you are okay with this patch?
With no comments from John or Steve in the past week, I've gone ahead
and merged the patch into audit/next.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3] audit: report audit wait metric in audit status reply
On Wed, Jul 15, 2020 at 9:30 PM Paul Moore wrote: > On Wed, Jul 8, 2020 at 7:13 PM Paul Moore wrote: > > On Sat, Jul 4, 2020 at 11:15 AM Max Englander > > wrote: > > > > > > In environments where the preservation of audit events and predictable > > > usage of system memory are prioritized, admins may use a combination of > > > --backlog_wait_time and -b options at the risk of degraded performance > > > resulting from backlog waiting. In some cases, this risk may be > > > preferred to lost events or unbounded memory usage. Ideally, this risk > > > can be mitigated by making adjustments when backlog waiting is detected. > > > > > > However, detection can be difficult using the currently available > > > metrics. For example, an admin attempting to debug degraded performance > > > may falsely believe a full backlog indicates backlog waiting. It may > > > turn out the backlog frequently fills up but drains quickly. > > > > > > To make it easier to reliably track degraded performance to backlog > > > waiting, this patch makes the following changes: > > > > > > Add a new field backlog_wait_time_total to the audit status reply. > > > Initialize this field to zero. Add to this field the total time spent > > > by the current task on scheduled timeouts while the backlog limit is > > > exceeded. Reset field to zero upon request via AUDIT_SET. > > > > > > Tested on Ubuntu 18.04 using complementary changes to the > > > audit-userspace and audit-testsuite: > > > - https://github.com/linux-audit/audit-userspace/pull/134 > > > - https://github.com/linux-audit/audit-testsuite/pull/97 > > > > > > Signed-off-by: Max Englander > > > --- > > > Patch changelogs between v1 and v2: > > > - Instead of printing a warning when backlog waiting occurs, add > > > duration of backlog waiting to cumulative sum, and report this > > > sum in audit status reply. > > > > > > Patch changelogs between v2 and v3: > > > - Rename backlog_wait_sum to backlog_wait_time_actual. > > > - Drop unneeded and unwanted header flags > > >AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM and > > >AUDIT_VERSION_BACKLOG_WAIT_SUM. > > > - Increment backlog_wait_time_actual counter after every call to > > >schedule_timeout rather than once after enqueuing (or losing) an > > >audit record. > > > - Add support for resetting backlog_wait_time_actual counter to zero > > >upon request via AUDIT_SET. > > > > > > include/uapi/linux/audit.h | 18 +++--- > > > kernel/audit.c | 35 +-- > > > 2 files changed, 36 insertions(+), 17 deletions(-) > > > > This looks okay to me, thanks for the fixes Max. > > > > Steve, does the associated userspace patch look okay to you? > > Steve, any comments on the userspace patch? Did I miss a reply in my > inbox perhaps? > > If I don't see any feedback by the end of the week I'll plan on > merging this into audit/next. It's been over two weeks with no comment, so I went ahead and merged this into audit/next. Thanks for your patience Max! -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak84 v4] audit: purge audit_log_string from the intra-kernel audit API
On 7/21/20 8:19 AM, Paul Moore wrote:
> On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs wrote:
>> On 2020-07-14 16:29, Paul Moore wrote:
>>> On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs wrote:
On 2020-07-14 12:21, Paul Moore wrote:
> On Mon, Jul 13, 2020 at 3:52 PM Richard Guy Briggs
> wrote:
>>
>> audit_log_string() was inteded to be an internal audit function and
>> since there are only two internal uses, remove them. Purge all external
>> uses of it by restructuring code to use an existing audit_log_format()
>> or using audit_log_format().
>>
>> Please see the upstream issue
>> https://github.com/linux-audit/audit-kernel/issues/84
>>
>> Signed-off-by: Richard Guy Briggs
>> ---
>> Passes audit-testsuite.
>>
>> Changelog:
>> v4
>> - use double quotes in all replaced audit_log_string() calls
>>
>> v3
>> - fix two warning: non-void function does not return a value in all
>> control paths
>> Reported-by: kernel test robot
>>
>> v2
>> - restructure to piggyback on existing audit_log_format() calls,
>> checking quoting needs for each.
>>
>> v1 Vlad Dronov
>> -
>> https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf
>>
>> include/linux/audit.h | 5 -
>> kernel/audit.c| 4 ++--
>> security/apparmor/audit.c | 10 --
>> security/apparmor/file.c | 25 +++--
>> security/apparmor/ipc.c | 46
>> +++---
>> security/apparmor/net.c | 14 --
>> security/lsm_audit.c | 4 ++--
>> 7 files changed, 46 insertions(+), 62 deletions(-)
>
> Thanks for restoring the quotes, just one question below ...
>
>> diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
>> index 4ecedffbdd33..fe36d112aad9 100644
>> --- a/security/apparmor/ipc.c
>> +++ b/security/apparmor/ipc.c
>> @@ -20,25 +20,23 @@
>>
>> /**
>> * audit_ptrace_mask - convert mask to permission string
>> - * @buffer: buffer to write string to (NOT NULL)
>> * @mask: permission mask to convert
>> + *
>> + * Returns: pointer to static string
>> */
>> -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask)
>> +static const char *audit_ptrace_mask(u32 mask)
>> {
>> switch (mask) {
>> case MAY_READ:
>> - audit_log_string(ab, "read");
>> - break;
>> + return "read";
>> case MAY_WRITE:
>> - audit_log_string(ab, "trace");
>> - break;
>> + return "trace";
>> case AA_MAY_BE_READ:
>> - audit_log_string(ab, "readby");
>> - break;
>> + return "readby";
>> case AA_MAY_BE_TRACED:
>> - audit_log_string(ab, "tracedby");
>> - break;
>> + return "tracedby";
>> }
>> + return "";
>
> Are we okay with this returning an empty string ("") in this case?
> Should it be a question mark ("?")?
>
> My guess is that userspace parsing should be okay since it still has
> quotes, I'm just not sure if we wanted to use a question mark as we do
> in other cases where the field value is empty/unknown.
Previously, it would have been an empty value, not even double quotes.
"?" might be an improvement.
>>>
>>> Did you want to fix that now in this patch, or leave it to later? As
>>> I said above, I'm not too bothered by it with the quotes so either way
>>> is fine by me.
>>
>> I'd defer to Steve, otherwise I'd say leave it, since there wasn't
>> anything there before and this makes that more evident.
>>
>>> John, I'm assuming you are okay with this patch?
>
> With no comments from John or Steve in the past week, I've gone ahead
> and merged the patch into audit/next.
>
sorry, for some reason I thought a new iteration of this was coming.
the patch is fine, the empty unknown value should be possible here
so changing it to "?" won't affect anything.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak84 v4] audit: purge audit_log_string from the intra-kernel audit API
On Tue, Jul 21, 2020 at 3:31 PM John Johansen
wrote:
> On 7/21/20 8:19 AM, Paul Moore wrote:
> > On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs wrote:
> >> On 2020-07-14 16:29, Paul Moore wrote:
> >>> On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs
> >>> wrote:
> On 2020-07-14 12:21, Paul Moore wrote:
> > On Mon, Jul 13, 2020 at 3:52 PM Richard Guy Briggs
> > wrote:
> >>
> >> audit_log_string() was inteded to be an internal audit function and
> >> since there are only two internal uses, remove them. Purge all
> >> external
> >> uses of it by restructuring code to use an existing audit_log_format()
> >> or using audit_log_format().
> >>
> >> Please see the upstream issue
> >> https://github.com/linux-audit/audit-kernel/issues/84
> >>
> >> Signed-off-by: Richard Guy Briggs
> >> ---
> >> Passes audit-testsuite.
> >>
> >> Changelog:
> >> v4
> >> - use double quotes in all replaced audit_log_string() calls
> >>
> >> v3
> >> - fix two warning: non-void function does not return a value in all
> >> control paths
> >> Reported-by: kernel test robot
> >>
> >> v2
> >> - restructure to piggyback on existing audit_log_format() calls,
> >> checking quoting needs for each.
> >>
> >> v1 Vlad Dronov
> >> -
> >> https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf
> >>
> >> include/linux/audit.h | 5 -
> >> kernel/audit.c| 4 ++--
> >> security/apparmor/audit.c | 10 --
> >> security/apparmor/file.c | 25 +++--
> >> security/apparmor/ipc.c | 46
> >> +++---
> >> security/apparmor/net.c | 14 --
> >> security/lsm_audit.c | 4 ++--
> >> 7 files changed, 46 insertions(+), 62 deletions(-)
> >
> > Thanks for restoring the quotes, just one question below ...
> >
> >> diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
> >> index 4ecedffbdd33..fe36d112aad9 100644
> >> --- a/security/apparmor/ipc.c
> >> +++ b/security/apparmor/ipc.c
> >> @@ -20,25 +20,23 @@
> >>
> >> /**
> >> * audit_ptrace_mask - convert mask to permission string
> >> - * @buffer: buffer to write string to (NOT NULL)
> >> * @mask: permission mask to convert
> >> + *
> >> + * Returns: pointer to static string
> >> */
> >> -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask)
> >> +static const char *audit_ptrace_mask(u32 mask)
> >> {
> >> switch (mask) {
> >> case MAY_READ:
> >> - audit_log_string(ab, "read");
> >> - break;
> >> + return "read";
> >> case MAY_WRITE:
> >> - audit_log_string(ab, "trace");
> >> - break;
> >> + return "trace";
> >> case AA_MAY_BE_READ:
> >> - audit_log_string(ab, "readby");
> >> - break;
> >> + return "readby";
> >> case AA_MAY_BE_TRACED:
> >> - audit_log_string(ab, "tracedby");
> >> - break;
> >> + return "tracedby";
> >> }
> >> + return "";
> >
> > Are we okay with this returning an empty string ("") in this case?
> > Should it be a question mark ("?")?
> >
> > My guess is that userspace parsing should be okay since it still has
> > quotes, I'm just not sure if we wanted to use a question mark as we do
> > in other cases where the field value is empty/unknown.
>
> Previously, it would have been an empty value, not even double quotes.
> "?" might be an improvement.
> >>>
> >>> Did you want to fix that now in this patch, or leave it to later? As
> >>> I said above, I'm not too bothered by it with the quotes so either way
> >>> is fine by me.
> >>
> >> I'd defer to Steve, otherwise I'd say leave it, since there wasn't
> >> anything there before and this makes that more evident.
> >>
> >>> John, I'm assuming you are okay with this patch?
> >
> > With no comments from John or Steve in the past week, I've gone ahead
> > and merged the patch into audit/next.
>
> sorry, for some reason I thought a new iteration of this was coming.
>
> the patch is fine, the empty unknown value should be possible here
> so changing it to "?" won't affect anything.
Yeah, I was kind of on the fence about requiring a new version from
Richard. I think "?" is arguably the right approach, but I don't
think it matters enough to force the issue. If it proves to be
problematic we can fix it later.
Regardless, it's in audit/next now.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V9 10/13] audit: add support for containerid to network namespaces
On 2020-07-05 11:11, Paul Moore wrote:
> On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote:
> >
> > This also adds support to qualify NETFILTER_PKT records.
> >
> > 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 be 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 audit container identifiiers.
> >
> > Add/increment the audit container identifier on:
> > - initial setting of the audit container identifier via /proc
> > - clone/fork call that inherits an audit container identifier
> > - unshare call that inherits an audit container identifier
> > - setns call that inherits an audit container identifier
> > Delete/decrement the audit container identifier on:
> > - an inherited audit container identifier dropped when child set
> > - process exit
> > - unshare call that drops a net namespace
> > - setns call that drops a net namespace
> >
> > Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> > event standalone records. Iterate through all potential audit container
> > identifiers associated with a network namespace.
> >
> > Please see the github audit kernel issue for contid net support:
> > https://github.com/linux-audit/audit-kernel/issues/92
> > Please see the github audit testsuiite issue for the test case:
> > https://github.com/linux-audit/audit-testsuite/issues/64
> > Please see the github audit wiki for the feature overview:
> > https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > Signed-off-by: Richard Guy Briggs
> > Acked-by: Neil Horman
> > Reviewed-by: Ondrej Mosnacek
> > ---
> > include/linux/audit.h| 20 ++
> > kernel/audit.c | 156
> > ++-
> > kernel/nsproxy.c | 4 ++
> > net/netfilter/nft_log.c | 11 +++-
> > net/netfilter/xt_AUDIT.c | 11 +++-
> > 5 files changed, 195 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index c4a755ae0d61..304fbb7c3c5b 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -128,6 +128,13 @@ struct audit_task_info {
> >
> > extern struct audit_task_info init_struct_audit;
> >
> > +struct audit_contobj_netns {
> > + struct list_headlist;
> > + struct audit_contobj*obj;
> > + int count;
>
> This seems like it might be a good candidate for refcount_t, yes?
I considered this before when converting the struct audit_contobj to
refcount_t, but decided against it since any updates are in the context
of a list traversal where it could be added to the list and so the
spinlock is already held anyways.
Is there a more efficent or elegant way of doing the locking around the
two list traversals below (_add and _del)?
I wonder about converting the count to refcount_t and only holding the
spinlock for the list_add_rcu() in the _add case. And for the _del case
holding the spinlock only for the list_del_rcu().
These are the only two locations items are added or deleted from the
lists.
Somewhat related to this is does the list order matter? Items are
currently added at the end of the list which likely makes locking
simpler, though the start of the list is a simple change. However,
unless we understand the profile of read use of these lists for
reporting contid use in audit_log_netns_contid_list() I don't think
order matters significantly. It could be that reporting of a contid
goes down in frequency over the lifetime of a contid that inserting them
at the beginning of the list would be best. This is not a visible
implementation detail so later optimization should pose no problem.
> > + struct rcu_head rcu;
> > +};
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 997c34178ee8..a862721dfd9b 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -437,6 +452,136 @@ static struct sock *audit_get_sk(const struct net
> > *net)
> > return aunet->sk;
> > }
> >
> > +void audit_netns_contid_add(struct net *net, struct audit_contobj *cont)
> > +{
> > + struct audit_net *aunet;
> > + struct list_head *contobj_list;
> > + struct audit_contobj_netns *contns;
> > +
> > + if (!net)
> > + return;
> > + if (!cont)
> > + return;
> > + aunet = net_generic(net, audit_net_id);
> > + if (!aunet)
> > + return;
> > + contobj_list = &aunet->contobj_list;
> > + rcu_read_lock();
> > + spin_lock(&aunet->contobj_list_lock);
> > + list_for_each_entry_rcu(contns, contobj_list, list)
> > + if (contns->obj == cont) {
> > +
Re: null pointer dereference regression in 5.7
On Sat, Jul 18, 2020 at 2:56 PM Dominick Grift
wrote:
> On 7/18/20 8:40 PM, bauen1 wrote:
> > Hi,
> > After upgrading from linux 5.6 to 5.7 on my debian machines with selinux
> > I've started seeing this null pointer dereference in the audit system. I've
> > included shortened logs for 5.6 without the error and from 5.7 with the
> > error from my laptop. I've also seen it happen in a VM and a server, but
> > don't have the logs anymore. Grift was able to reproduced (presumably) the
> > same issue on fedora with 5.8-rc4.
> >
> > Steps to reproduce:
> > Write an selinux policy with a domain for systemd-user-runtime-dir and
> > audit all permissions of the dir class. E.g. `(auditallow
> > systemd_user_runtime_dir_t all_types (dir (all)))`
> > Switch to permissive mode.
> > Create a new user and login, log out and wait a few seconds for systemd to
> > stop user-runtime-dir@.service
>
> This should be a reproducer:
>
> echo "(auditallow systemd_logind_t file_type (dir (all)))" > mytest.cil
> && sudo semodule -i mytest.cil
> reboot
Thanks bauen1 and Dominick.
Richard, you broke it, you bought it :) Did you want to take a closer
look at this? If you can't let me know. Based on a quick look, my
gut feeling is that either context->pwd is never set properly or it is
getting free'd prematurely; I'm highly suspicious of the latter but
the former seems like it might be a reasonable place to start.
> > I believe this issue was made visible by
> > 1320a4052ea11eb2879eb7361da15a106a780972.
> > Now a AUDIT_PATH event is also generated by default and
> > systemd-user-runtime-dir is making syscalls that audit_log_name can't
> > handle.
> >
> > I hope this is enough info to find the root cause.
> > - bauen1
> >
> > Log without crash (5.6):
> >
> > Jul 18 14:26:36 jh-mba kernel: Linux version 5.6.0-2-amd64
> > (debian-ker...@lists.debian.org) (gcc version 9.3.0 (Debian 9.3.0-13)) #1
> > SMP Debian 5.6.14-2 (2020-06-09)
> > Jul 18 14:27:53 jh-mba audit[1]: SERVICE_STOP pid=1 uid=0 auid=4294967295
> > ses=4294967295 subj=system_u:system_r:init_t:s0 msg='unit=user@1001
> > comm="systemd" exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=?
> > res=success'
> > Jul 18 14:27:53 jh-mba systemd[1]: Stopping User Runtime Directory
> > /run/user/1001...
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { read } for
> > pid=3178 comm="systemd-user-ru" name="dconf" dev="tmpfs" ino=41325
> > scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:gconf_tmp_t:s0 tclass=dir permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { open } for
> > pid=3178 comm="systemd-user-ru" path="/run/user/1001/dconf" dev="tmpfs"
> > ino=41325 scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:gconf_tmp_t:s0 tclass=dir permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { getattr } for
> > pid=3178 comm="systemd-user-ru" path="/run/user/1001/dconf" dev="tmpfs"
> > ino=41325 scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:gconf_tmp_t:s0 tclass=dir permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { search } for
> > pid=3178 comm="systemd-user-ru" name="dconf" dev="tmpfs" ino=41325
> > scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:gconf_tmp_t:s0 tclass=dir permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { write } for
> > pid=3178 comm="systemd-user-ru" name="dconf" dev="tmpfs" ino=41325
> > scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:gconf_tmp_t:s0 tclass=dir permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { remove_name } for
> > pid=3178 comm="systemd-user-ru" name="user" dev="tmpfs" ino=41326
> > scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:gconf_tmp_t:s0 tclass=dir permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { unlink } for
> > pid=3178 comm="systemd-user-ru" name="user" dev="tmpfs" ino=41326
> > scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:gconf_tmp_t:s0 tclass=file permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { rmdir } for
> > pid=3178 comm="systemd-user-ru" name="dconf" dev="tmpfs" ino=41325
> > scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:gconf_tmp_t:s0 tclass=dir permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { read } for
> > pid=3178 comm="systemd-user-ru" name="gvfs" dev="tmpfs" ino=42315
> > scontext=system_u:system_r:systemd_user_runtime_dir_t:s0
> > tcontext=user_u:object_r:user_tmp_t:s0 tclass=dir permissive=1
> > Jul 18 14:27:53 jh-mba audit[3178]: AVC avc: denied { open } for
> > pid=3178 comm="systemd-user-ru" path="/run/user/1001/gvfs" dev="tmpfs"
> > ino=42315 scontext=system_u:syste
Re: null pointer dereference regression in 5.7
On Tue, Jul 21, 2020 at 6:30 PM Paul Moore wrote:
> Richard, you broke it, you bought it :) Did you want to take a closer
> look at this? If you can't let me know. Based on a quick look, my
> gut feeling is that either context->pwd is never set properly or it is
> getting free'd prematurely; I'm highly suspicious of the latter but
> the former seems like it might be a reasonable place to start.
Actually, yes, I'm pretty certain the problem is that context->pwd is
never set in this case.
Normally context->pwd would be set by a call to
audit_getname()/__audit_getname(), but if there audit context is a
dummy context, that is skipped and context->pwd is never set.
Normally that is fine, expect with Richard's patch if the kernel
explicitly calls audit_log_start() we mark the context as ... not a
dummy? smart? I'm not sure of the right term here ... which then
triggers all the usual logging one would expect. In this particular
case, a SELinux AVC, the audit_log_start() happens *after* the
pathname has been resolved and the audit_getname() calls are made;
thus in this case context->pwd is not valid when the normal audit
logging takes place on exit and things explode in predictable fashion.
Unfortunately, it is beginning to look like 1320a4052ea1 ("audit:
trigger accompanying records when no rules present") may be more
dangerous than initially thought. I'm borderline tempted to just
revert this patch, but I'll leave this open for discussion ...
Richard, I think you need to go through the code and audit all of the
functions that store data in an audit context that are skipped when
there is a dummy context to see which fields are potentially unset,
and then look at all the end of task/syscall code to make sure the
necessary set/unset checks are in place.
This should be a priority.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3] audit: report audit wait metric in audit status reply
On Tue, Jul 21, 2020 at 11:26:53AM -0400, Paul Moore wrote: > On Wed, Jul 15, 2020 at 9:30 PM Paul Moore wrote: > > On Wed, Jul 8, 2020 at 7:13 PM Paul Moore wrote: > > > On Sat, Jul 4, 2020 at 11:15 AM Max Englander > > > wrote: > > > > > > > > In environments where the preservation of audit events and predictable > > > > usage of system memory are prioritized, admins may use a combination of > > > > --backlog_wait_time and -b options at the risk of degraded performance > > > > resulting from backlog waiting. In some cases, this risk may be > > > > preferred to lost events or unbounded memory usage. Ideally, this risk > > > > can be mitigated by making adjustments when backlog waiting is detected. > > > > > > > > However, detection can be difficult using the currently available > > > > metrics. For example, an admin attempting to debug degraded performance > > > > may falsely believe a full backlog indicates backlog waiting. It may > > > > turn out the backlog frequently fills up but drains quickly. > > > > > > > > To make it easier to reliably track degraded performance to backlog > > > > waiting, this patch makes the following changes: > > > > > > > > Add a new field backlog_wait_time_total to the audit status reply. > > > > Initialize this field to zero. Add to this field the total time spent > > > > by the current task on scheduled timeouts while the backlog limit is > > > > exceeded. Reset field to zero upon request via AUDIT_SET. > > > > > > > > Tested on Ubuntu 18.04 using complementary changes to the > > > > audit-userspace and audit-testsuite: > > > > - https://github.com/linux-audit/audit-userspace/pull/134 > > > > - https://github.com/linux-audit/audit-testsuite/pull/97 > > > > > > > > Signed-off-by: Max Englander > > > > --- > > > > Patch changelogs between v1 and v2: > > > > - Instead of printing a warning when backlog waiting occurs, add > > > > duration of backlog waiting to cumulative sum, and report this > > > > sum in audit status reply. > > > > > > > > Patch changelogs between v2 and v3: > > > > - Rename backlog_wait_sum to backlog_wait_time_actual. > > > > - Drop unneeded and unwanted header flags > > > >AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM and > > > >AUDIT_VERSION_BACKLOG_WAIT_SUM. > > > > - Increment backlog_wait_time_actual counter after every call to > > > >schedule_timeout rather than once after enqueuing (or losing) an > > > >audit record. > > > > - Add support for resetting backlog_wait_time_actual counter to zero > > > >upon request via AUDIT_SET. > > > > > > > > include/uapi/linux/audit.h | 18 +++--- > > > > kernel/audit.c | 35 +-- > > > > 2 files changed, 36 insertions(+), 17 deletions(-) > > > > > > This looks okay to me, thanks for the fixes Max. > > > > > > Steve, does the associated userspace patch look okay to you? > > > > Steve, any comments on the userspace patch? Did I miss a reply in my > > inbox perhaps? > > > > If I don't see any feedback by the end of the week I'll plan on > > merging this into audit/next. > > It's been over two weeks with no comment, so I went ahead and merged > this into audit/next. Thanks for your patience Max! Excellent, glad to hear it! Thank you (and Richard, Steve) for the guidance and interesting discussion along the way. > > -- > paul moore > www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: null pointer dereference regression in 5.7
On 2020-07-21 18:45, Paul Moore wrote:
> On Tue, Jul 21, 2020 at 6:30 PM Paul Moore wrote:
> > Richard, you broke it, you bought it :) Did you want to take a closer
> > look at this? If you can't let me know. Based on a quick look, my
> > gut feeling is that either context->pwd is never set properly or it is
> > getting free'd prematurely; I'm highly suspicious of the latter but
> > the former seems like it might be a reasonable place to start.
>
> Actually, yes, I'm pretty certain the problem is that context->pwd is
> never set in this case.
I'll have a look. This review is also related to ghak122 and
potentially missing PATH records...
> Normally context->pwd would be set by a call to
> audit_getname()/__audit_getname(), but if there audit context is a
> dummy context, that is skipped and context->pwd is never set.
> Normally that is fine, expect with Richard's patch if the kernel
> explicitly calls audit_log_start() we mark the context as ... not a
> dummy? smart? I'm not sure of the right term here ... which then
> triggers all the usual logging one would expect. In this particular
> case, a SELinux AVC, the audit_log_start() happens *after* the
> pathname has been resolved and the audit_getname() calls are made;
> thus in this case context->pwd is not valid when the normal audit
> logging takes place on exit and things explode in predictable fashion.
>
> Unfortunately, it is beginning to look like 1320a4052ea1 ("audit:
> trigger accompanying records when no rules present") may be more
> dangerous than initially thought. I'm borderline tempted to just
> revert this patch, but I'll leave this open for discussion ...
> Richard, I think you need to go through the code and audit all of the
> functions that store data in an audit context that are skipped when
> there is a dummy context to see which fields are potentially unset,
> and then look at all the end of task/syscall code to make sure the
> necessary set/unset checks are in place.
>
> This should be a priority.
>
> --
> paul moore
> www.paul-moore.com
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: null pointer dereference regression in 5.7
On 2020-07-21 18:45, Paul Moore wrote:
> On Tue, Jul 21, 2020 at 6:30 PM Paul Moore wrote:
> > Richard, you broke it, you bought it :) Did you want to take a closer
> > look at this? If you can't let me know. Based on a quick look, my
> > gut feeling is that either context->pwd is never set properly or it is
> > getting free'd prematurely; I'm highly suspicious of the latter but
> > the former seems like it might be a reasonable place to start.
>
> Actually, yes, I'm pretty certain the problem is that context->pwd is
> never set in this case.
Does the ghak96 upstream patch in audit/next on 5.8-rc1 fix it?
d7481b24b816 ("audit: issue CWD record to accompany LSM_AUDIT_DATA_*
records")
The avc is generated by common_lsm_audit() which calls
dump_common_audit_data() that now calls audit_getcwd() on the 5
LSM_AUDIT_DATA_* types that deal with paths.
> Normally context->pwd would be set by a call to
> audit_getname()/__audit_getname(), but if there audit context is a
> dummy context, that is skipped and context->pwd is never set.
> Normally that is fine, expect with Richard's patch if the kernel
> explicitly calls audit_log_start() we mark the context as ... not a
> dummy? smart? I'm not sure of the right term here ... which then
> triggers all the usual logging one would expect. In this particular
> case, a SELinux AVC, the audit_log_start() happens *after* the
> pathname has been resolved and the audit_getname() calls are made;
> thus in this case context->pwd is not valid when the normal audit
> logging takes place on exit and things explode in predictable fashion.
The first two AVCs that were accompanied by syscalls had "items=0" but
the one that blew up had "items=2" so it appears the paths were already
present in the context, but missing the pwd.
> Unfortunately, it is beginning to look like 1320a4052ea1 ("audit:
> trigger accompanying records when no rules present") may be more
> dangerous than initially thought. I'm borderline tempted to just
> revert this patch, but I'll leave this open for discussion ...
> Richard, I think you need to go through the code and audit all of the
> functions that store data in an audit context that are skipped when
> there is a dummy context to see which fields are potentially unset,
> and then look at all the end of task/syscall code to make sure the
> necessary set/unset checks are in place.
Auditing all the callers is not a small task, but I agree it may be
necessary.
> This should be a priority.
>
> paul moore
- RGB
--
Richard Guy Briggs
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
