date:20180223

Re: [PATCH 0/2] audit boot parameter cleanups

2018-02-23 Thread Paul Moore

On Fri, Feb 23, 2018 at 11:07 AM, Richard Guy Briggs  wrote:
> On 2018-02-22 17:22, Greg Edwards wrote:
>> One of our CI tests was booting upstream kernels with the "audit=off" kernel
>> parameter.  This was our error; it should have been "audit=0".  However,
>> in 4.15 the verification of the boot parameter got more strict in 
>> 80ab4df62706
>> ("audit: don't use simple_strtol() anymore"), and our errant boot parameter
>> value starting panic'ing the system.
>>
>> The problem is this happens so early in boot, the console isn't initialized 
>> yet
>> and you don't see the panic message.  You have no idea what the problem is
>> unless you add an "earlyprintk" boot option, e.g.
>> earlyprintk=serial,ttyS0,115200n8.

Ah ha, that helps explain things - thank you!

>> Fix this by having the boot parameter setup function just save the boot
>> parameter value, and process it later from a call in audit_init().  The 
>> console
>> is initialized by this point, and you can see any panic messages without 
>> having
>> to use an earlyprintk option.
>
> This part all looks good.

I had forgotten how tricky this code can be ... I believe there are a
few issues with patch 1/2 and how it initializes audit (it breaks
auditing for PID 1), but I need to double check a few things first.

>> Additionally, add "on" and "off" as valid audit boot parameter values.
>
> This part is a step in the right direction, but I've got minor concerns
> about variations on "0" and "1" that will no longer work, since any
> non-zero integer worked previously and will no longer do so.
>
> I would have still used the integer conversion but checked explicitly
> for "on" and "off" prior to testing for an integer.

I agree with Richard that testing for "on"/"off" independently, and
first, would be a good idea.

I also just realized that we probably can't default to enabling audit,
at least not how I was thinking anyway.

Anyway, a bit of an apology, I had hoped to review this before I ended
my day today, but I didn't leave myself enough time ... I'll try to
provide proper feedback this weekend, if that doesn't happen you
should see something early next week.

Thanks again for your help thus far, additional hands are always welcome!

>> Greg Edwards (2):
>>   audit: move processing of "audit" boot param to audit_init()
>>   audit: add "on"/"off" as valid boot parameter values
>>
>>  Documentation/admin-guide/kernel-parameters.txt | 14 +++
>>  kernel/audit.c  | 49 
>> -
>>  2 files changed, 39 insertions(+), 24 deletions(-)
>
> - 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

-- 
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: track the owner of the command mutex ourselves

2018-02-23 Thread Paul Moore

On Thu, Feb 22, 2018 at 3:40 AM, Richard Guy Briggs  wrote:
> On 2018-02-20 11:55, Paul Moore wrote:
>> 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 
>
> This is what I was trying to accomplish here through several iterations,
> but your solution looks more graceful:
>
> https://www.redhat.com/archives/linux-audit/2015-October/msg00073.html

Yes, I credited you with the mutex owner idea in the patch from last
March that kickstarted the queue rework; while your patch didn't quite
work out I still think the idea is solid.

> /me has no dog...
>
> Reviewed-by: Richard Guy Briggs 

I haven't heard any objections over the past few days, just
misunderstanding on how the locking/queues work, so I'm going to go
ahead and merge this into audit/next.  At the very least it should
satisfy Peter's concerns with our usage of __mutex_owner().

>> ---
>>  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

Re: [PATCH 0/2] audit boot parameter cleanups

2018-02-23 Thread Richard Guy Briggs

On 2018-02-22 17:22, Greg Edwards wrote:
> One of our CI tests was booting upstream kernels with the "audit=off" kernel
> parameter.  This was our error; it should have been "audit=0".  However,
> in 4.15 the verification of the boot parameter got more strict in 80ab4df62706
> ("audit: don't use simple_strtol() anymore"), and our errant boot parameter
> value starting panic'ing the system.
> 
> The problem is this happens so early in boot, the console isn't initialized 
> yet
> and you don't see the panic message.  You have no idea what the problem is
> unless you add an "earlyprintk" boot option, e.g.
> earlyprintk=serial,ttyS0,115200n8.
> 
> Fix this by having the boot parameter setup function just save the boot
> parameter value, and process it later from a call in audit_init().  The 
> console
> is initialized by this point, and you can see any panic messages without 
> having
> to use an earlyprintk option.

This part all looks good.

> Additionally, add "on" and "off" as valid audit boot parameter values.

This part is a step in the right direction, but I've got minor concerns
about variations on "0" and "1" that will no longer work, since any
non-zero integer worked previously and will no longer do so.

I would have still used the integer conversion but checked explicitly
for "on" and "off" prior to testing for an integer.

> Greg Edwards (2):
>   audit: move processing of "audit" boot param to audit_init()
>   audit: add "on"/"off" as valid boot parameter values
> 
>  Documentation/admin-guide/kernel-parameters.txt | 14 +++
>  kernel/audit.c  | 49 
> -
>  2 files changed, 39 insertions(+), 24 deletions(-)

- 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 0/2] audit boot parameter cleanups

2018-02-23 Thread Greg Edwards

One of our CI tests was booting upstream kernels with the "audit=off" kernel
parameter.  This was our error; it should have been "audit=0".  However,
in 4.15 the verification of the boot parameter got more strict in 80ab4df62706
("audit: don't use simple_strtol() anymore"), and our errant boot parameter
value starting panic'ing the system.

The problem is this happens so early in boot, the console isn't initialized yet
and you don't see the panic message.  You have no idea what the problem is
unless you add an "earlyprintk" boot option, e.g.
earlyprintk=serial,ttyS0,115200n8.

Fix this by having the boot parameter setup function just save the boot
parameter value, and process it later from a call in audit_init().  The console
is initialized by this point, and you can see any panic messages without having
to use an earlyprintk option.

Additionally, add "on" and "off" as valid audit boot parameter values.

Greg Edwards (2):
  audit: move processing of "audit" boot param to audit_init()
  audit: add "on"/"off" as valid boot parameter values

 Documentation/admin-guide/kernel-parameters.txt | 14 +++
 kernel/audit.c  | 49 -
 2 files changed, 39 insertions(+), 24 deletions(-)

-- 
2.14.3

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

[PATCH 1/2] audit: move processing of "audit" boot param to audit_init()

2018-02-23 Thread Greg Edwards

The processing of the "audit" boot parameter is handled before the
console has been initialized.  We therefore miss any panic messages if
we fail to verify the boot parameter or set the audit state, unless we
also enable earlyprintk.

Instead, have the boot parameter function just save the parameter value
and process it later from audit_init(), which is a postcore_initcall()
function.

Signed-off-by: Greg Edwards 
---
 kernel/audit.c | 48 +++-
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..3fb11bcb4408 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -99,6 +99,9 @@ static u32audit_failure = AUDIT_FAIL_PRINTK;
 /* private audit network namespace index */
 static unsigned int audit_net_id;
 
+/* 'audit' boot parameter value */
+static char *audit_boot;
+
 /**
  * struct audit_net - audit private network namespace data
  * @sk: communication socket
@@ -1528,11 +1531,35 @@ static struct pernet_operations audit_net_ops 
__net_initdata = {
.size = sizeof(struct audit_net),
 };
 
+/* Process kernel command-line parameter at boot time.  audit=0 or audit=1. */
+static void __init audit_enable(void)
+{
+   long val;
+
+   if (!audit_boot)
+   return;
+
+   if (kstrtol(audit_boot, 0, ))
+   panic("audit: invalid 'audit' parameter value (%s)\n",
+ audit_boot);
+   audit_default = (val ? AUDIT_ON : AUDIT_OFF);
+
+   if (audit_default == AUDIT_OFF)
+   audit_initialized = AUDIT_DISABLED;
+   if (audit_set_enabled(audit_default))
+   panic("audit: error setting audit state (%d)\n", audit_default);
+
+   pr_info("%s\n", audit_default ?
+   "enabled (after initialization)" : "disabled (until reboot)");
+}
+
 /* Initialize audit support at boot time. */
 static int __init audit_init(void)
 {
int i;
 
+   audit_enable();
+
if (audit_initialized == AUDIT_DISABLED)
return 0;
 
@@ -1567,26 +1594,13 @@ static int __init audit_init(void)
 }
 postcore_initcall(audit_init);
 
-/* Process kernel command-line parameter at boot time.  audit=0 or audit=1. */
-static int __init audit_enable(char *str)
+/* Store kernel command-line parameter at boot time.  audit=0 or audit=1. */
+static int __init audit_set(char *str)
 {
-   long val;
-
-   if (kstrtol(str, 0, ))
-   panic("audit: invalid 'audit' parameter value (%s)\n", str);
-   audit_default = (val ? AUDIT_ON : AUDIT_OFF);
-
-   if (audit_default == AUDIT_OFF)
-   audit_initialized = AUDIT_DISABLED;
-   if (audit_set_enabled(audit_default))
-   panic("audit: error setting audit state (%d)\n", audit_default);
-
-   pr_info("%s\n", audit_default ?
-   "enabled (after initialization)" : "disabled (until reboot)");
-
+   audit_boot = str;
return 1;
 }
-__setup("audit=", audit_enable);
+__setup("audit=", audit_set);
 
 /* Process kernel command-line parameter at boot time.
  * audit_backlog_limit= */
-- 
2.14.3

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

[PATCH 2/2] audit: add "on"/"off" as valid boot parameter values

2018-02-23 Thread Greg Edwards

Modify the "audit" boot parameter to also accept "on" or "off" as valid
parameter values.  Update the documentation accordingly.

Signed-off-by: Greg Edwards 
---
 Documentation/admin-guide/kernel-parameters.txt | 14 +++---
 kernel/audit.c  |  9 +
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..0b926779315c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -389,15 +389,15 @@
Use software keyboard repeat
 
audit=  [KNL] Enable the audit sub-system
-   Format: { "0" | "1" } (0 = disabled, 1 = enabled)
-   0 - kernel audit is disabled and can not be enabled
-   until the next reboot
+   Format: { "0" | "1" | "off" | "on" }
+   0 | off - kernel audit is disabled and can not be
+   enabled until the next reboot
unset - kernel audit is initialized but disabled and
will be fully enabled by the userspace auditd.
-   1 - kernel audit is initialized and partially enabled,
-   storing at most audit_backlog_limit messages in
-   RAM until it is fully enabled by the userspace
-   auditd.
+   1 | on - kernel audit is initialized and partially
+   enabled, storing at most audit_backlog_limit
+   messages in RAM until it is fully enabled by the
+   userspace auditd.
Default: unset
 
audit_backlog_limit= [KNL] Set the audit queue size limit.
diff --git a/kernel/audit.c b/kernel/audit.c
index 3fb11bcb4408..8c8304a3ea8f 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1534,15 +1534,16 @@ static struct pernet_operations audit_net_ops 
__net_initdata = {
 /* Process kernel command-line parameter at boot time.  audit=0 or audit=1. */
 static void __init audit_enable(void)
 {
-   long val;
-
if (!audit_boot)
return;
 
-   if (kstrtol(audit_boot, 0, ))
+   if (!strcmp(audit_boot, "1") || !strcmp(audit_boot, "on"))
+   audit_default = AUDIT_ON;
+   else if (!strcmp(audit_boot, "0") || !strcmp(audit_boot, "off"))
+   audit_default = AUDIT_OFF;
+   else
panic("audit: invalid 'audit' parameter value (%s)\n",
  audit_boot);
-   audit_default = (val ? AUDIT_ON : AUDIT_OFF);
 
if (audit_default == AUDIT_OFF)
audit_initialized = AUDIT_DISABLED;
-- 
2.14.3

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

Re: [PATCH 0/2] audit boot parameter cleanups

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

Re: [PATCH 0/2] audit boot parameter cleanups

[PATCH 0/2] audit boot parameter cleanups

[PATCH 1/2] audit: move processing of "audit" boot param to audit_init()

[PATCH 2/2] audit: add "on"/"off" as valid boot parameter values

6 matches

Site Navigation

Mail list logo

Footer information