Re: [PATCH] audit: return on memory error to avoid null pointer dereference

[Richard Guy Briggs]

On 2018-02-21 01:47, Richard Guy Briggs wrote:
> If there is a memory allocation error when trying to change an audit
> kernel feature value, the ignored allocation error will trigger a NULL
> pointer dereference oops on subsequent use of that pointer.  Return
> instead.
> 
> See: https://github.com/linux-audit/audit-kernel/issues/76
> Signed-off-by: Richard Guy Briggs 

Self-NACK.  It was based on local development and won't compile on
upstream.  Fix pending.

> ---
>  kernel/audit.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 196d327..31cb11d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1063,6 +1063,8 @@ static void audit_log_feature_change(int which, u32 
> old_feature, u32 new_feature
>   return;
>  
>   ab = audit_log_start(context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> + if (!ab)
> + return;
>   audit_log_task_info(ab, current);
>   audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u 
> res=%d",
>audit_feature_names[which], !!old_feature, 
> !!new_feature,
> -- 
> 1.8.3.1
> 

- 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

[PATCH] audit: return on memory error to avoid null pointer dereference

[Richard Guy Briggs]

If there is a memory allocation error when trying to change an audit
kernel feature value, the ignored allocation error will trigger a NULL
pointer dereference oops on subsequent use of that pointer.  Return
instead.

See: https://github.com/linux-audit/audit-kernel/issues/76
Signed-off-by: Richard Guy Briggs 
---
 kernel/audit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 196d327..31cb11d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1063,6 +1063,8 @@ static void audit_log_feature_change(int which, u32 
old_feature, u32 new_feature
return;
 
ab = audit_log_start(context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+   if (!ab)
+   return;
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u 
res=%d",
 audit_feature_names[which], !!old_feature, 
!!new_feature,
-- 
1.8.3.1

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

Re: [PATCH] audit: do not panic kernel on invalid audit parameter

[Richard Guy Briggs]

On 2018-02-20 16:45, Paul Moore wrote:
> On Tue, Feb 20, 2018 at 4:33 PM, Greg Edwards  wrote:
> > If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
> > the kernel panics very early in boot with no output on the console
> > indicating the problem.
> 
> I'm guessing the problem is that there was too much info dumped to the
> console and the error message was lost (there is one, to say there is
> "no output" isn't completely correct), is that what happened?  Or was
> there honestly *no* output on the console?
> 
> > This seems overly harsh.  Instead, print the error indicating an invalid
> > audit parameter value and leave auditing disabled.
> 
> There are some audit requirements which appear rather bizarre at
> times, e.g. the need to panic the kernel instead of losing an audit
> event.  Steve is the one who follows most of these audit requirements
> so I'm going to wait until he has a chance to look at this.
> 
> There is also another issue in this patch, on error you have the audit
> subsystem default to off, we may want to change this to default to on
> in case of error (fail safely).

Like Paul, I would have to support the default to on in case of error.

> > Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
> > Signed-off-by: Greg Edwards 
> > ---
> >  kernel/audit.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 227db99b0f19..d8af7682d6a3 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str)
> >  {
> > long val;
> >
> > -   if (kstrtol(str, 0, ))
> > -   panic("audit: invalid 'audit' parameter value (%s)\n", str);
> > +   if (kstrtol(str, 0, )) {
> > +   pr_err("invalid 'audit' parameter value (%s)\n", str);
> > +   val = AUDIT_OFF;
> > +   }
> > audit_default = (val ? AUDIT_ON : AUDIT_OFF);
> >
> > if (audit_default == AUDIT_OFF)
> > --
> > 2.14.3
> >
> 
> 
> 
> -- 
> 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: [PATCH] audit: do not panic kernel on invalid audit parameter

[Paul Moore]

On Tue, Feb 20, 2018 at 5:00 PM, Greg Edwards  wrote:
> On Tue, Feb 20, 2018 at 04:45:26PM -0500, Paul Moore wrote:
>> On Tue, Feb 20, 2018 at 4:33 PM, Greg Edwards  wrote:
>>> If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
>>> the kernel panics very early in boot with no output on the console
>>> indicating the problem.
>>
>> I'm guessing the problem is that there was too much info dumped to the
>> console and the error message was lost (there is one, to say there is
>> "no output" isn't completely correct), is that what happened?  Or was
>> there honestly *no* output on the console?
>
> Booting a 4.16-rc2 VM with defconfig + kvmconfig with the 'audit=off'
> boot parameter (my mistake), the only output you get is:
>
> .
>
> Not terribly enlightening.

Oooo, fun.

I wonder if the call to panic() is happening before the kernel
initializes the console.  Ungh.

I'll have to play with this some, but if that is the case we may not
be able to display anything meaningful, and we may just have to
default to "on".

>>> This seems overly harsh.  Instead, print the error indicating an invalid
>>> audit parameter value and leave auditing disabled.
>>
>> There are some audit requirements which appear rather bizarre at
>> times, e.g. the need to panic the kernel instead of losing an audit
>> event.  Steve is the one who follows most of these audit requirements
>> so I'm going to wait until he has a chance to look at this.
>>
>> There is also another issue in this patch, on error you have the audit
>> subsystem default to off, we may want to change this to default to on
>> in case of error (fail safely).
>
> Sure, that is fine.  I just took a stab at what to do for the error
> case.  I'm happy to default it to enabled, if that would be more
> appropriate.
>
> Greg

-- 
paul moore
www.paul-moore.com

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

Re: [PATCH] audit: do not panic kernel on invalid audit parameter

[Paul Moore]

On Tue, Feb 20, 2018 at 4:33 PM, Greg Edwards  wrote:
> If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
> the kernel panics very early in boot with no output on the console
> indicating the problem.

I'm guessing the problem is that there was too much info dumped to the
console and the error message was lost (there is one, to say there is
"no output" isn't completely correct), is that what happened?  Or was
there honestly *no* output on the console?

> This seems overly harsh.  Instead, print the error indicating an invalid
> audit parameter value and leave auditing disabled.

There are some audit requirements which appear rather bizarre at
times, e.g. the need to panic the kernel instead of losing an audit
event.  Steve is the one who follows most of these audit requirements
so I'm going to wait until he has a chance to look at this.

There is also another issue in this patch, on error you have the audit
subsystem default to off, we may want to change this to default to on
in case of error (fail safely).

> Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
> Signed-off-by: Greg Edwards 
> ---
>  kernel/audit.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 227db99b0f19..d8af7682d6a3 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str)
>  {
> long val;
>
> -   if (kstrtol(str, 0, ))
> -   panic("audit: invalid 'audit' parameter value (%s)\n", str);
> +   if (kstrtol(str, 0, )) {
> +   pr_err("invalid 'audit' parameter value (%s)\n", str);
> +   val = AUDIT_OFF;
> +   }
> audit_default = (val ? AUDIT_ON : AUDIT_OFF);
>
> if (audit_default == AUDIT_OFF)
> --
> 2.14.3
>



-- 
paul moore
www.paul-moore.com

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

Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking

[Paul Moore]

On Tue, Feb 20, 2018 at 10:18 AM, Peter Zijlstra  wrote:
> On Tue, Feb 20, 2018 at 09:51:08AM -0500, Paul Moore wrote:
>> On Tue, Feb 20, 2018 at 9:06 AM, Peter Zijlstra  wrote:
>
>> > It's not at all clear to me what that code does, I just stumbled upon
>> > __mutex_owner() outside of the mutex code itself and went WTF.
>>
>> If you don't want people to use __mutex_owner() outside of the mutex
>> code I might suggest adding a rather serious comment at the top of the
>> function, because right now I don't see anything suggesting that
>> function shouldn't be used.  Yes, there is the double underscore
>> prefix, but that can mean a few different things these days.
>
> Find below.
>
>> > The comment (aside from having the most horribly style) ...
>>
>> Yeah, your dog is ugly too.  Notice how neither comment is constructive?
>
> I'm sure you've seen this one:
>
>   https://lkml.org/lkml/2016/7/8/625

Yep.  I stand behind my earlier comment in this thread.

>> > Maybe if you could explain how that code is supposed to work and why it
>> > doesn't know if it holds a lock I could make a suggestion...
>>
>> I just spent a few minutes looking back over the bits available in
>> include/linux/mutex.h and I'm not seeing anything beyond
>> __mutex_owner() which would allow us to determine the mutex owning
>> task.  It's probably easiest for us to just track ownership ourselves.
>> I'll put together a patch later today.
>
> Note that up until recently the mutex implementation didn't even have a
> consistent owner field. And the thing is, it's very easy to use wrong,
> only today I've seen a patch do: "__mutex_owner() == task", where task
> was allowed to be !current, which is just wrong.

Arguably all the more reason why a strongly worded warning is
important (which I see you've included below, feel free to include my
Reviewed-by).

> Looking through kernel/audit.c I'm not even sure I see how you would end
> up in audit_log_start() with audit_cmd_mutex held.
>
> Can you give me a few code paths that trigger this? Simple git-grep is
> failing me.

Basically look at the code in audit_receive_msg(), but I wasn't asking
your opinion on how we should rewrite the audit subsystem, I was just
asking how one could determine if the current task was holding a given
mutex in a way that was acceptable to you.  Based on your comments,
and some further inspection of the mutex code, it appears that is/was
not something that the core mutex code wants to support/make-visible.
Which is perfectly fine, I just wanted to make sure I wasn't missing
something before I went ahead and wrote a wrapper around the mutex
code for use by audit.

FWIW, I just put together the following patch which removes the
__mutex_owner() call from audit and doesn't appear to break anything
on the audit side (you're CC'd on the patch).  It has only been
lightly tested, but I'm going to bang on it for a day or so and if I
hear no objections I'll merge it into audit/next.

* https://www.redhat.com/archives/linux-audit/2018-February/msg00066.html

> ---
> Subject: mutex: Add comment to __mutex_owner()
> From: Peter Zijlstra 
> Date: Tue Feb 20 16:01:36 CET 2018
>
> Attempt to deter usage, this is not a public interface. It is entirely
> possibly to implement a conformant mutex without having this owner
> field (in fact, we used to have that).
>
> Signed-off-by: Peter Zijlstra (Intel) 
> ---
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -66,6 +66,11 @@ struct mutex {
>  #endif
>  };
>
> +/*
> + * Internal helper function; C doesn't allow us to hide it :/
> + *
> + * DO NOT USE (outside of mutex code).
> + */
>  static inline struct task_struct *__mutex_owner(struct mutex *lock)
>  {
> return (struct task_struct *)(atomic_long_read(>owner) & ~0x07);

Reviewed-by: Paul Moore 

-- 
paul moore
www.paul-moore.com

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

[PATCH] audit: track the owner of the command mutex ourselves

[Paul Moore]

From: Paul Moore 

Evidently the __mutex_owner() function was never intended for use
outside the core mutex code, so build a thing locking wrapper around
the mutex code which allows us to track the mutex owner.

One, arguably positive, side effect is that this allows us to hide
the audit_cmd_mutex inside of kernel/audit.c behind the lock/unlock
functions.

Reported-by: Peter Zijlstra 
Signed-off-by: Paul Moore 
---
 kernel/audit.c  |   66 +++
 kernel/audit.h  |3 ++
 kernel/audit_tree.c |8 +++---
 3 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 5c2544984375..3c4f6f3d7041 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -181,9 +181,21 @@ static char *audit_feature_names[2] = {
"loginuid_immutable",
 };
 
-
-/* Serialize requests from userspace. */
-DEFINE_MUTEX(audit_cmd_mutex);
+/**
+ * struct audit_ctl_mutex - serialize requests from userspace
+ * @lock: the mutex used for locking
+ * @owner: the task which owns the lock
+ *
+ * Description:
+ * This is the lock struct used to ensure we only process userspace requests
+ * in an orderly fashion.  We can't simply use a mutex/lock here because we
+ * need to track lock ownership so we don't end up blocking the lock owner in
+ * audit_log_start() or similar.
+ */
+static struct audit_ctl_mutex {
+   struct mutex lock;
+   void *owner;
+} audit_cmd_mutex;
 
 /* AUDIT_BUFSIZ is the size of the temporary buffer used for formatting
  * audit records.  Since printk uses a 1024 byte buffer, this buffer
@@ -227,6 +239,36 @@ int auditd_test_task(struct task_struct *task)
return rc;
 }
 
+/**
+ * audit_ctl_lock - Take the audit control lock
+ */
+void audit_ctl_lock(void)
+{
+   mutex_lock(_cmd_mutex.lock);
+   audit_cmd_mutex.owner = current;
+}
+
+/**
+ * audit_ctl_unlock - Drop the audit control lock
+ */
+void audit_ctl_unlock(void)
+{
+   audit_cmd_mutex.owner = NULL;
+   mutex_unlock(_cmd_mutex.lock);
+}
+
+/**
+ * audit_ctl_owner_current - Test to see if the current task owns the lock
+ *
+ * Description:
+ * Return true if the current task owns the audit control lock, false if it
+ * doesn't own the lock.
+ */
+static bool audit_ctl_owner_current(void)
+{
+   return (current == audit_cmd_mutex.owner);
+}
+
 /**
  * auditd_pid_vnr - Return the auditd PID relative to the namespace
  *
@@ -861,8 +903,8 @@ int audit_send_list(void *_dest)
struct sock *sk = audit_get_sk(dest->net);
 
/* wait for parent to finish and send an ACK */
-   mutex_lock(_cmd_mutex);
-   mutex_unlock(_cmd_mutex);
+   audit_ctl_lock();
+   audit_ctl_unlock();
 
while ((skb = __skb_dequeue(>q)) != NULL)
netlink_unicast(sk, skb, dest->portid, 0);
@@ -903,8 +945,8 @@ static int audit_send_reply_thread(void *arg)
struct audit_reply *reply = (struct audit_reply *)arg;
struct sock *sk = audit_get_sk(reply->net);
 
-   mutex_lock(_cmd_mutex);
-   mutex_unlock(_cmd_mutex);
+   audit_ctl_lock();
+   audit_ctl_unlock();
 
/* Ignore failure. It'll only happen if the sender goes away,
   because our timeout is set to infinite. */
@@ -1467,7 +1509,7 @@ static void audit_receive(struct sk_buff  *skb)
nlh = nlmsg_hdr(skb);
len = skb->len;
 
-   mutex_lock(_cmd_mutex);
+   audit_ctl_lock();
while (nlmsg_ok(nlh, len)) {
err = audit_receive_msg(skb, nlh);
/* if err or if this message says it wants a response */
@@ -1476,7 +1518,7 @@ static void audit_receive(struct sk_buff  *skb)
 
nlh = nlmsg_next(nlh, );
}
-   mutex_unlock(_cmd_mutex);
+   audit_ctl_unlock();
 }
 
 /* Run custom bind function on netlink socket group connect or bind requests. 
*/
@@ -1548,6 +1590,9 @@ static int __init audit_init(void)
for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
INIT_LIST_HEAD(_inode_hash[i]);
 
+   mutex_init(_cmd_mutex.lock);
+   audit_cmd_mutex.owner = NULL;
+
pr_info("initializing netlink subsys (%s)\n",
audit_default ? "enabled" : "disabled");
register_pernet_subsys(_net_ops);
@@ -1711,8 +1756,7 @@ struct audit_buffer *audit_log_start(struct audit_context 
*ctx, gfp_t gfp_mask,
 *using a PID anchored in the caller's namespace
 * 2. generator holding the audit_cmd_mutex - we don't want to block
 *while holding the mutex */
-   if (!(auditd_test_task(current) ||
- (current == __mutex_owner(_cmd_mutex {
+   if (!(auditd_test_task(current) || audit_ctl_owner_current())) {
long stime = audit_backlog_wait_time;
 
while (audit_backlog_limit &&
diff --git a/kernel/audit.h b/kernel/audit.h
index af5bc59487ed..214e14948370 100644
--- a/kernel/audit.h
+++ 

Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking

[Peter Zijlstra]

On Tue, Feb 20, 2018 at 09:51:08AM -0500, Paul Moore wrote:
> On Tue, Feb 20, 2018 at 9:06 AM, Peter Zijlstra  wrote:

> > It's not at all clear to me what that code does, I just stumbled upon
> > __mutex_owner() outside of the mutex code itself and went WTF.
> 
> If you don't want people to use __mutex_owner() outside of the mutex
> code I might suggest adding a rather serious comment at the top of the
> function, because right now I don't see anything suggesting that
> function shouldn't be used.  Yes, there is the double underscore
> prefix, but that can mean a few different things these days.

Find below.

> > The comment (aside from having the most horribly style) ...
> 
> Yeah, your dog is ugly too.  Notice how neither comment is constructive?

I'm sure you've seen this one:

  https://lkml.org/lkml/2016/7/8/625

It's all about reading code; inconsistent and unbalanced styles are just
_really_ hard on the brain.

> > ... is wrong too, because it claims it will not block when we hold that 
> > lock, while,
> > afaict, it will in fact do just that.
> 
> A mutex blocks when it is held, but the audit_log_start() function
> should not block for the task that currently holds the
> audit_cmd_mutex; that is what the comment is meant to convey.  I
> believe the comment makes sense, but I did write it so I'll concede
> that I'm probably the not best judge.  If anyone would like to offer a
> different wording I'm happy to consider it.

The comment uses 'sleep' which is typically used to mean anything that
schedules, but then it does the schedule_timeout() thing.

> > Maybe if you could explain how that code is supposed to work and why it
> > doesn't know if it holds a lock I could make a suggestion...
> 
> I just spent a few minutes looking back over the bits available in
> include/linux/mutex.h and I'm not seeing anything beyond
> __mutex_owner() which would allow us to determine the mutex owning
> task.  It's probably easiest for us to just track ownership ourselves.
> I'll put together a patch later today.

Note that up until recently the mutex implementation didn't even have a
consistent owner field. And the thing is, it's very easy to use wrong,
only today I've seen a patch do: "__mutex_owner() == task", where task
was allowed to be !current, which is just wrong.

Looking through kernel/audit.c I'm not even sure I see how you would end
up in audit_log_start() with audit_cmd_mutex held.

Can you give me a few code paths that trigger this? Simple git-grep is
failing me.


---
Subject: mutex: Add comment to __mutex_owner()
From: Peter Zijlstra 
Date: Tue Feb 20 16:01:36 CET 2018

Attempt to deter usage, this is not a public interface. It is entirely
possibly to implement a conformant mutex without having this owner
field (in fact, we used to have that).

Signed-off-by: Peter Zijlstra (Intel) 
---
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -66,6 +66,11 @@ struct mutex {
 #endif
 };
 
+/*
+ * Internal helper function; C doesn't allow us to hide it :/
+ *
+ * DO NOT USE (outside of mutex code).
+ */
 static inline struct task_struct *__mutex_owner(struct mutex *lock)
 {
return (struct task_struct *)(atomic_long_read(>owner) & ~0x07);

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

Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking

[Paul Moore]

On Tue, Feb 20, 2018 at 9:06 AM, Peter Zijlstra  wrote:
> On Tue, Feb 20, 2018 at 08:25:21AM -0500, Paul Moore wrote:
>> On Tue, Feb 20, 2018 at 7:37 AM, Peter Zijlstra  wrote:
>> > On Tue, Mar 28, 2017 at 02:30:56PM +0200, Greg Kroah-Hartman wrote:
>> >> 4.10-stable review patch.  If anyone has any objections, please let me 
>> >> know.
>> >
>> >> + if (!(auditd_test_task(current) ||
>> >> +   (current == __mutex_owner(_cmd_mutex {
>> >> + long stime = audit_backlog_wait_time;
>> >
>> > Since I cannot find the original email on lkml, NAK on this.
>> > __mutex_owner() is not a general purpose helper function.
>>
>> Since this code also exists in the current kernel, I need to ask what
>> recommended alternatives exist for determining the mutex owner?
>>
>> I imagine we could track the mutex owner separately in the audit
>> subsystem, but I'd much prefer to leverage an existing mechanism if
>> possible.
>
> It's not at all clear to me what that code does, I just stumbled upon
> __mutex_owner() outside of the mutex code itself and went WTF.

If you don't want people to use __mutex_owner() outside of the mutex
code I might suggest adding a rather serious comment at the top of the
function, because right now I don't see anything suggesting that
function shouldn't be used.  Yes, there is the double underscore
prefix, but that can mean a few different things these days.

> The comment (aside from having the most horribly style) ...

Yeah, your dog is ugly too.  Notice how neither comment is constructive?

> ... is wrong too, because it claims it will not block when we hold that lock, 
> while,
> afaict, it will in fact do just that.

A mutex blocks when it is held, but the audit_log_start() function
should not block for the task that currently holds the
audit_cmd_mutex; that is what the comment is meant to convey.  I
believe the comment makes sense, but I did write it so I'll concede
that I'm probably the not best judge.  If anyone would like to offer a
different wording I'm happy to consider it.

> Maybe if you could explain how that code is supposed to work and why it
> doesn't know if it holds a lock I could make a suggestion...

I just spent a few minutes looking back over the bits available in
include/linux/mutex.h and I'm not seeing anything beyond
__mutex_owner() which would allow us to determine the mutex owning
task.  It's probably easiest for us to just track ownership ourselves.
I'll put together a patch later today.

-- 
paul moore
www.paul-moore.com

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

Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking

[Peter Zijlstra]

On Tue, Feb 20, 2018 at 08:25:21AM -0500, Paul Moore wrote:
> On Tue, Feb 20, 2018 at 7:37 AM, Peter Zijlstra  wrote:
> > On Tue, Mar 28, 2017 at 02:30:56PM +0200, Greg Kroah-Hartman wrote:
> >> 4.10-stable review patch.  If anyone has any objections, please let me 
> >> know.
> >
> >> + if (!(auditd_test_task(current) ||
> >> +   (current == __mutex_owner(_cmd_mutex {
> >> + long stime = audit_backlog_wait_time;
> >
> > Since I cannot find the original email on lkml, NAK on this.
> > __mutex_owner() is not a general purpose helper function.
> 
> Since this code also exists in the current kernel, I need to ask what
> recommended alternatives exist for determining the mutex owner?
> 
> I imagine we could track the mutex owner separately in the audit
> subsystem, but I'd much prefer to leverage an existing mechanism if
> possible.

It's not at all clear to me what that code does, I just stumbled upon
__mutex_owner() outside of the mutex code itself and went WTF.

The comment (aside from having the most horribly style) is wrong too,
because it claims it will not block when we hold that lock, while,
afaict, it will in fact do just that.

Maybe if you could explain how that code is supposed to work and why it
doesn't know if it holds a lock I could make a suggestion...

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

Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking

[Paul Moore]

On Tue, Feb 20, 2018 at 7:37 AM, Peter Zijlstra  wrote:
> On Tue, Mar 28, 2017 at 02:30:56PM +0200, Greg Kroah-Hartman wrote:
>> 4.10-stable review patch.  If anyone has any objections, please let me know.
>
>> + if (!(auditd_test_task(current) ||
>> +   (current == __mutex_owner(_cmd_mutex {
>> + long stime = audit_backlog_wait_time;
>
> Since I cannot find the original email on lkml, NAK on this.
> __mutex_owner() is not a general purpose helper function.

Since this code also exists in the current kernel, I need to ask what
recommended alternatives exist for determining the mutex owner?

I imagine we could track the mutex owner separately in the audit
subsystem, but I'd much prefer to leverage an existing mechanism if
possible.

-- 
paul moore
www.paul-moore.com

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