Re: [PATCH v2] audit: do not panic kernel on invalid audit parameter
On 2018-02-21 15:51, Greg Edwards wrote:
> On Wed, Feb 21, 2018 at 04:08:25PM -0500, Paul Moore wrote:
> > On February 21, 2018 11:19:09 AM 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.
> >>
> >> Instead, print the error indicating an invalid audit parameter value,
> >> but leave auditing enabled.
> >
> > Thanks for the quick follow-up, it's actually a little *too* quick if
> > I'm honest, I still haven't fully thought through all the different
> > options here :)
> >
> > However, in the interest in capitalizing on your enthusiasm and
> > willingness to help, here are some of the things I was thinking about,
> > in no particular order:
> >
> > #1 - We might want to consider accepting both "0" and "off" as
> > acceptable inputs. It should be a trivial change, and if we are going
> > to default to on/enabled it seems like we should make a reasonable
> > effort to do the right thing when people attempt to disable audit
> > (unfortunately the kernel command line parameters seem to use both "0"
> > and "off" so we can't blame people too much when they use "off").
>
> Yes, I think this would be a good idea, and for what it's worth,
> 'audit=off' worked until 4.15. One of our CI tests that verifies
> upstream kernels picked this up starting with 4.15.
Huh, at first I wondered if the earlier audit init was at play here, but
now I'm suspecting
80ab4df62706b882922c3bb0b05ce2c8ab10828a
("audit: don't use simple_strtol() anymore")
is the primary culprit, exacerbated by earlier init from the same
patchset.
> For example, booting a 4.14.20 kernel (defconfig + kvmconfig):
>
> [0.00] Linux version 4.14.20 (gedwards@psuche) (gcc version 7.3.1
> 20180130 (Red Hat 7.3.1-2) (GCC)) #1 SMP Wed Feb 21 15:14:25 M
> ST 2018
> [0.00] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
> ...
> [0.00] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8
> audit=off
> [0.00] audit: disabled (until reboot)
>
>
> > #2 - If panic("") doesn't work, does pr_err("")? If it
> > does, I would be curious to understand why.
>
> Yes, pr_err() does work. Booting 4.16-rc2 (defconfig + kvmconfig) with
> this patch:
>
> [0.00] Linux version 4.16.0-rc2+ (gedwards@psuche) (gcc version 7.3.1
> 20180130 (Red Hat 7.3.1-2) (GCC)) #1 SMP Wed Feb 21 15:23:10 MST 2018
> [0.00] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
> ...
> [0.00] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8
> audit=off
> [0.00] audit: invalid 'audit' parameter value (off)
> [0.00] audit: enabled (after initialization)
>
>
> I suspect what is happening with the current audit_enable() code is the
> serial console has not been fully initialized yet by the time we call
> panic(), so we never see the early printk messages queued up. I will
> try and confirm.
>
> > #3 - Related to #2 above, but there are other calls to panic() and
> > pr_*() in audit_enable() that should probably be re-evaluated and
> > changed. If we can't notify users/admins here, why are we trying?
>
> I haven't looked at those other calls to panic(), but I would bet they
> display the same behavior.
>
> > #4 - Related to #2 and #3, if we can't emit messages in audit_enable()
> > we need to find a way to "remember" that the user specified a bogus
> > audit setting and let them know as soon as we can. One possibility
> > might be to overload the audit_default variable (most places seem to
> > treat it as a true/false value) with a "AUDIT_DEFAULT_INVALID" value
> > (make it non-zero, say "3"?) and we could display a message in
> > audit_init() or similar. Full disclosure, this *should* work ... I
> > think ... but I might be missing some crucial detail.
>
> I'm unclear why we would need this, given that #2 above does work. This
> is the first time I've ever looked at the audit code, though. I was
> just doing a drive-by. ;)
>
> > I realize this is probably much more than you bargained for when you
> > first submitted your patch, and if you're not interested in taking
> > this any further I understand however, if you are willing to play
> > a bit more I would be very grateful :)
>
> Sure, I'm happy to look at the above.
>
> Greg
- 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: return on memory error to avoid null pointer dereference
On 2018-02-21 19:02, Paul Moore wrote: > On Wed, Feb 21, 2018 at 6:49 PM, Paul Moore wrote: > > On Wed, Feb 21, 2018 at 4:30 AM, 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. > >> > >> Passes audit-testsuite. > >> 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(+) > > > > Thanks, merged. > > > > In the future a "[PATCH v2]" prefix would be appreciated for patches > > like this, it makes things a little easier in my inbox. (Sorry, forgot in haste to get that fixed one out...) > After merging this I went through all the other callers to see if they > suffered the same mistake and everyone except for IMA was checking the > returned pointer for NULL. Upon looking at the IMA code, and the > audit code which is called, I realized we are actually "ok" as > audit_log_task_info(), audit_log_format(), audit_log_end(), and others > all check for a NULL audit_buffer at the very top of the functions. > I'm going to leave this patch merged, it's a good practice after all, > but I don't believe that unpatched systems are in any danger of > oops'ing here. On review, agreed. My ctags/cscope DBs need regeneration, so I hadn't noticed that the functions to which I was led weren't the ones I was seeking, and while these three do check, not all functions that accept a struct audit_buffer pointer parameter don't check for NULL. Now that I check, I only find audit_expand (whose callers are all protected) and audit_log_d_path (whose callers all appear to be protected), the latter of which I've spent a bit of time staring at of late (ghak8, ghak21...). We're ok. > >> diff --git a/kernel/audit.c b/kernel/audit.c > >> index 5c25449..2de74be 100644 > >> --- a/kernel/audit.c > >> +++ b/kernel/audit.c > >> @@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 > >> old_feature, u32 new_feature > >> return; > >> > >> ab = audit_log_start(NULL, 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 > > -- > paul moore > www.paul-moore.com - 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: return on memory error to avoid null pointer dereference
I think if we went back and looked at history we'd see that all of the code originally had none of the if(!ab) checks after allocation and they just sorta slowly crept in over time. I prefer this pattern, but it used to be the opposite everywhere. On Wed, 2018-02-21 at 19:02 -0500, Paul Moore wrote: > On Wed, Feb 21, 2018 at 6:49 PM, Paul Moore > wrote: > > On Wed, Feb 21, 2018 at 4:30 AM, 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. > > > > > > Passes audit-testsuite. > > > 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(+) > > > > Thanks, merged. > > > > In the future a "[PATCH v2]" prefix would be appreciated for > > patches > > like this, it makes things a little easier in my inbox. > > After merging this I went through all the other callers to see if > they > suffered the same mistake and everyone except for IMA was checking > the > returned pointer for NULL. Upon looking at the IMA code, and the > audit code which is called, I realized we are actually "ok" as > audit_log_task_info(), audit_log_format(), audit_log_end(), and > others > all check for a NULL audit_buffer at the very top of the functions. > I'm going to leave this patch merged, it's a good practice after all, > but I don't believe that unpatched systems are in any danger of > oops'ing here. > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > index 5c25449..2de74be 100644 > > > --- a/kernel/audit.c > > > +++ b/kernel/audit.c > > > @@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int > > > which, u32 old_feature, u32 new_feature > > > return; > > > > > > ab = audit_log_start(NULL, 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: return on memory error to avoid null pointer dereference
On Wed, Feb 21, 2018 at 6:49 PM, Paul Moore wrote: > On Wed, Feb 21, 2018 at 4:30 AM, 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. >> >> Passes audit-testsuite. >> 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(+) > > Thanks, merged. > > In the future a "[PATCH v2]" prefix would be appreciated for patches > like this, it makes things a little easier in my inbox. After merging this I went through all the other callers to see if they suffered the same mistake and everyone except for IMA was checking the returned pointer for NULL. Upon looking at the IMA code, and the audit code which is called, I realized we are actually "ok" as audit_log_task_info(), audit_log_format(), audit_log_end(), and others all check for a NULL audit_buffer at the very top of the functions. I'm going to leave this patch merged, it's a good practice after all, but I don't believe that unpatched systems are in any danger of oops'ing here. >> diff --git a/kernel/audit.c b/kernel/audit.c >> index 5c25449..2de74be 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 >> old_feature, u32 new_feature >> return; >> >> ab = audit_log_start(NULL, 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 -- 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: return on memory error to avoid null pointer dereference
On Wed, Feb 21, 2018 at 4:30 AM, 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. > > Passes audit-testsuite. > 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(+) Thanks, merged. In the future a "[PATCH v2]" prefix would be appreciated for patches like this, it makes things a little easier in my inbox. > diff --git a/kernel/audit.c b/kernel/audit.c > index 5c25449..2de74be 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 > old_feature, u32 new_feature > return; > > ab = audit_log_start(NULL, 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 -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] audit: do not panic kernel on invalid audit parameter
On Wed, Feb 21, 2018 at 04:08:25PM -0500, Paul Moore wrote:
> On February 21, 2018 11:19:09 AM 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.
>>
>> Instead, print the error indicating an invalid audit parameter value,
>> but leave auditing enabled.
>
> Thanks for the quick follow-up, it's actually a little *too* quick if
> I'm honest, I still haven't fully thought through all the different
> options here :)
>
> However, in the interest in capitalizing on your enthusiasm and
> willingness to help, here are some of the things I was thinking about,
> in no particular order:
>
> #1 - We might want to consider accepting both "0" and "off" as
> acceptable inputs. It should be a trivial change, and if we are going
> to default to on/enabled it seems like we should make a reasonable
> effort to do the right thing when people attempt to disable audit
> (unfortunately the kernel command line parameters seem to use both "0"
> and "off" so we can't blame people too much when they use "off").
Yes, I think this would be a good idea, and for what it's worth,
'audit=off' worked until 4.15. One of our CI tests that verifies
upstream kernels picked this up starting with 4.15.
For example, booting a 4.14.20 kernel (defconfig + kvmconfig):
[0.00] Linux version 4.14.20 (gedwards@psuche) (gcc version 7.3.1
20180130 (Red Hat 7.3.1-2) (GCC)) #1 SMP Wed Feb 21 15:14:25 M
ST 2018
[0.00] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
...
[0.00] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8
audit=off
[0.00] audit: disabled (until reboot)
> #2 - If panic("") doesn't work, does pr_err("")? If it
> does, I would be curious to understand why.
Yes, pr_err() does work. Booting 4.16-rc2 (defconfig + kvmconfig) with
this patch:
[0.00] Linux version 4.16.0-rc2+ (gedwards@psuche) (gcc version 7.3.1
20180130 (Red Hat 7.3.1-2) (GCC)) #1 SMP Wed Feb 21 15:23:10 MST 2018
[0.00] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
...
[0.00] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8
audit=off
[0.00] audit: invalid 'audit' parameter value (off)
[0.00] audit: enabled (after initialization)
I suspect what is happening with the current audit_enable() code is the
serial console has not been fully initialized yet by the time we call
panic(), so we never see the early printk messages queued up. I will
try and confirm.
> #3 - Related to #2 above, but there are other calls to panic() and
> pr_*() in audit_enable() that should probably be re-evaluated and
> changed. If we can't notify users/admins here, why are we trying?
I haven't looked at those other calls to panic(), but I would bet they
display the same behavior.
> #4 - Related to #2 and #3, if we can't emit messages in audit_enable()
> we need to find a way to "remember" that the user specified a bogus
> audit setting and let them know as soon as we can. One possibility
> might be to overload the audit_default variable (most places seem to
> treat it as a true/false value) with a "AUDIT_DEFAULT_INVALID" value
> (make it non-zero, say "3"?) and we could display a message in
> audit_init() or similar. Full disclosure, this *should* work ... I
> think ... but I might be missing some crucial detail.
I'm unclear why we would need this, given that #2 above does work. This
is the first time I've ever looked at the audit code, though. I was
just doing a drive-by. ;)
> I realize this is probably much more than you bargained for when you
> first submitted your patch, and if you're not interested in taking
> this any further I understand however, if you are willing to play
> a bit more I would be very grateful :)
Sure, I'm happy to look at the above.
Greg
--
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
On Wed, Feb 21, 2018 at 3:46 AM, Ingo Molnar wrote:
>
> * Paul Moore wrote:
>
>> 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
>
> Could you please explain the audit_ctl_lock()/unlock() primitive you are
> introducing there? You seem to be implementing some sort of recursive locking
> primitive, but in a strange way.
>
> AFAICS the primary problem appears to be this code path:
>
> audit_receive() -> audit_receive_msg() -> AUDIT_TTY_SET ->
> audit_log_common_recv_msg() -> audit_log_start()
>
> where we can arrive already holding the lock.
>
> I.e. recursive mutex, kinda.
>
> What's the thinking there? Neither the changelog nor the code explains this.
I don't really go into great detail in the changelog, or comments in
the code, because I'm not really doing anything new with respect to
locking in this commit. The patch simply wraps the existing
mutex_{lock,unlock}() calls so that we can track the mutex owner. It
doesn't fundamentally change the locking, it's a quick patch to get
rid our our __mutex_owner() usage as Peter doesn't want anyone,
outside the mutex code, to use that function.
Based on your comments above, I'm guessing some of the
misunderstanding comes from the
__mutex_owner()/audit_ctl_owner_current() call in audit_log_start().
We try to determine the mutex/lock owner in audit_log_start() not
because we are trying to avoid a recursive lock, we do the check as an
optimization to skip the normal queue managment so that the lock
holder isn't subject to the same rescheduling/queue-management (is
"queue calming" a term?) as regular tasks.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] audit: do not panic kernel on invalid audit parameter
On Wednesday, February 21, 2018 4:08:25 PM EST Paul Moore wrote:
> On February 21, 2018 11:19:09 AM 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.
> >
> > Instead, print the error indicating an invalid audit parameter value,
> > but leave auditing enabled.
> >
> > Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
> > Signed-off-by: Greg Edwards
> > ---
> >
> > Changes v1 -> v2:
> > - default to auditing enabled for the error case
> >
> > kernel/audit.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Thanks for the quick follow-up, it's actually a little *too* quick if I'm
> honest, I still haven't fully thought through all the different options
> here :)
>
> However, in the interest in capitalizing on your enthusiasm and willingness
> to help, here are some of the things I was thinking about, in no
> particular order:
>
> #1 - We might want to consider accepting both "0" and "off" as acceptable
> inputs. It should be a trivial change, and if we are going to default to
> on/enabled it seems like we should make a reasonable effort to do the
> right thing when people attempt to disable audit (unfortunately the kernel
> command line parameters seem to use both "0" and "off" so we can't blame
> people too much when they use "off").
>
> #2 - If panic("") doesn't work, does pr_err("")? If it does, I
> would be curious to understand why.
>
> #3 - Related to #2 above, but there are other calls to panic() and pr_*()
> in audit_enable() that should probably be re-evaluated and changed. If we
> can't notify users/admins here, why are we trying?
>
> #4 - Related to #2 and #3, if we can't emit messages in audit_enable() we
> need to find a way to "remember" that the user specified a bogus audit
> setting and let them know as soon as we can. One possibility might be to
> overload the audit_default variable (most places seem to treat it as a
> true/false value) with a "AUDIT_DEFAULT_INVALID" value (make it non-zero,
> say "3"?) and we could display a message in audit_init() or similar. Full
> disclosure, this *should* work ... I think ... but I might be missing some
> crucial detail.
Well, auditd will probably have a big problem starting up and that should be
a big clue. Also, this could be remembered in a way that a fault indication
is returned by auditctl -s? Loading audit rules leads to checking audit
status which journald keeps around.
-Steve
> I realize this is probably much more than you bargained for when you first
> submitted your patch, and if you're not interested in taking this any
> further I understand however, if you are willing to play a bit more I
> would be very grateful :)
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 227db99b0f19..9b80e9895107 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, &val))
> > - panic("audit: invalid 'audit' parameter value (%s)\n", str);
> > + if (kstrtol(str, 0, &val)) {
> > + pr_err("invalid 'audit' parameter value (%s)\n", str);
> > + val = AUDIT_ON;
> > + }
> >
> > audit_default = (val ? AUDIT_ON : AUDIT_OFF);
> >
> > if (audit_default == AUDIT_OFF)
>
> --
> paul moore
> www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] audit: do not panic kernel on invalid audit parameter
On February 21, 2018 11:19:09 AM 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.
>
> Instead, print the error indicating an invalid audit parameter value,
> but leave auditing enabled.
>
> Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
> Signed-off-by: Greg Edwards
> ---
> Changes v1 -> v2:
> - default to auditing enabled for the error case
>
> kernel/audit.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
Thanks for the quick follow-up, it's actually a little *too* quick if I'm
honest, I still haven't fully thought through all the different options here :)
However, in the interest in capitalizing on your enthusiasm and willingness to
help, here are some of the things I was thinking about, in no particular order:
#1 - We might want to consider accepting both "0" and "off" as acceptable
inputs. It should be a trivial change, and if we are going to default to
on/enabled it seems like we should make a reasonable effort to do the right
thing when people attempt to disable audit (unfortunately the kernel command
line parameters seem to use both "0" and "off" so we can't blame people too
much when they use "off").
#2 - If panic("") doesn't work, does pr_err("")? If it does, I would
be curious to understand why.
#3 - Related to #2 above, but there are other calls to panic() and pr_*() in
audit_enable() that should probably be re-evaluated and changed. If we can't
notify users/admins here, why are we trying?
#4 - Related to #2 and #3, if we can't emit messages in audit_enable() we need
to find a way to "remember" that the user specified a bogus audit setting and
let them know as soon as we can. One possibility might be to overload the
audit_default variable (most places seem to treat it as a true/false value)
with a "AUDIT_DEFAULT_INVALID" value (make it non-zero, say "3"?) and we could
display a message in audit_init() or similar. Full disclosure, this *should*
work ... I think ... but I might be missing some crucial detail.
I realize this is probably much more than you bargained for when you first
submitted your patch, and if you're not interested in taking this any further I
understand however, if you are willing to play a bit more I would be very
grateful :)
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 227db99b0f19..9b80e9895107 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, &val))
> - panic("audit: invalid 'audit' parameter value (%s)\n", str);
> + if (kstrtol(str, 0, &val)) {
> + pr_err("invalid 'audit' parameter value (%s)\n", str);
> + val = AUDIT_ON;
> + }
> 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
[PATCH v2] audit: do not panic kernel on invalid audit parameter
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.
Instead, print the error indicating an invalid audit parameter value,
but leave auditing enabled.
Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
Signed-off-by: Greg Edwards
---
Changes v1 -> v2:
- default to auditing enabled for the error case
kernel/audit.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..9b80e9895107 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, &val))
- panic("audit: invalid 'audit' parameter value (%s)\n", str);
+ if (kstrtol(str, 0, &val)) {
+ pr_err("invalid 'audit' parameter value (%s)\n", str);
+ val = AUDIT_ON;
+ }
audit_default = (val ? AUDIT_ON : AUDIT_OFF);
if (audit_default == AUDIT_OFF)
--
2.14.3
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH] audit: do not panic kernel on invalid audit parameter
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.
This seems overly harsh. Instead, print the error indicating an invalid
audit parameter value and leave auditing disabled.
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, &val))
- panic("audit: invalid 'audit' parameter value (%s)\n", str);
+ if (kstrtol(str, 0, &val)) {
+ 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
--
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
On Wed, Feb 21, 2018 at 09:46:02AM +0100, Ingo Molnar wrote:
> AFAICS the primary problem appears to be this code path:
>
> audit_receive() -> audit_receive_msg() -> AUDIT_TTY_SET ->
> audit_log_common_recv_msg() -> audit_log_start()
>
> where we can arrive already holding the lock.
>
> I.e. recursive mutex, kinda.
I _think_ something like the below ought to work, but I've no idea how
to even begin testing audit.
---
kernel/audit.c | 31 ---
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..24175754f79d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -184,6 +184,9 @@ static char *audit_feature_names[2] = {
/* Serialize requests from userspace. */
DEFINE_MUTEX(audit_cmd_mutex);
+static struct audit_buffer *__audit_log_start(struct audit_context *ctx, gfp_t
gfp_mask,
+int type, bool recursive);
+
/* AUDIT_BUFSIZ is the size of the temporary buffer used for formatting
* audit records. Since printk uses a 1024 byte buffer, this buffer
* should be at least that large. */
@@ -357,7 +360,7 @@ static int audit_log_config_change(char *function_name, u32
new, u32 old,
struct audit_buffer *ab;
int rc = 0;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = __audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE, true);
if (unlikely(!ab))
return rc;
audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
@@ -1024,7 +1027,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(NULL, GFP_KERNEL, msg_type, true);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
@@ -1057,7 +1060,7 @@ static void audit_log_feature_change(int which, u32
old_feature, u32 new_feature
if (audit_enabled == AUDIT_OFF)
return;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ ab = __audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE, true);
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,
@@ -1578,6 +1581,12 @@ static int __init audit_enable(char *str)
if (audit_default == AUDIT_OFF)
audit_initialized = AUDIT_DISABLED;
+ /*
+* Normally audit_set_enabled() would need to be called under
+* @audit_cmd_mutex, however since audit_do_config_change() will not in
+* fact call audit_log_config_change() when 'audit_enabled ==
+* AUDIT_OFF', we can use it here without issue.
+*/
if (audit_set_enabled(audit_default))
panic("audit: error setting audit state (%d)\n", audit_default);
@@ -1690,8 +1699,8 @@ static inline void audit_get_stamp(struct audit_context
*ctx,
* will be written at syscall exit. If there is no associated task, then
* task context (ctx) should be NULL.
*/
-struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
-int type)
+static struct audit_buffer *__audit_log_start(struct audit_context *ctx, gfp_t
gfp_mask,
+int type, bool recursive)
{
struct audit_buffer *ab;
struct timespec64 t;
@@ -1703,6 +1712,9 @@ struct audit_buffer *audit_log_start(struct audit_context
*ctx, gfp_t gfp_mask,
if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
return NULL;
+ if (recursive)
+ lockdep_assert_held(&audit_cmd_mutex);
+
/* NOTE: don't ever fail/sleep on these two conditions:
* 1. auditd generated record - since we need auditd to drain the
*queue; also, when we are checking for auditd, compare PIDs using
@@ -1710,8 +1722,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(&audit_cmd_mutex {
+ if (!(auditd_test_task(current) || recursive)) {
long stime = audit_backlog_wait_time;
while (audit_backlog_limit &&
@@ -1753,6 +1764,12 @@ struct audit_buffer *audit_log_start(struct
audit_context *ctx, gfp_t gfp_mask,
return ab;
}
+struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
+int type)
+{
+ return __audit_log_start(ctx, gfp_mask, type, false);
+}
+
/**
* audit_expand - expand s
Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking
* Paul Moore wrote: > 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 Could you please explain the audit_ctl_lock()/unlock() primitive you are introducing there? You seem to be implementing some sort of recursive locking primitive, but in a strange way. AFAICS the primary problem appears to be this code path: audit_receive() -> audit_receive_msg() -> AUDIT_TTY_SET -> audit_log_common_recv_msg() -> audit_log_start() where we can arrive already holding the lock. I.e. recursive mutex, kinda. What's the thinking there? Neither the changelog nor the code explains this. Thanks, Ingo -- 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
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. >> 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 -- 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
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. Passes audit-testsuite. 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 5c25449..2de74be 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature return; ab = audit_log_start(NULL, 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
