Mailing List Archive

On Friday, August 30, 2013 03:06:46 PM Richard Guy Briggs wrote:
> On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote:
> > Btw. audit looks unmaintained... if you are going to take care of
> > this code, perhaps you can look at
> >
> > http://marc.info/?l=linux-kernel&m=137589907108485
> > http://marc.info/?l=linux-kernel&m=137590271809664

You don't want to clear the TIF audit flag when context == NULL. What that will
do is make a bunch of inauditable processes. There are times when audit is
disabled and then re-enabled later. If the flag gets cleared, then a task's
syscall will never enter the auditing framework from kernel/entry_64.S.

That flag is 0 when auditing has never ever been enabled. If auditing is
enabled, it should always be a 1 unless the task filter has determined that
this process should not be audited ever. In practice, this is almost never
used. But ensuring the TIF_SYSCALL_AUDIT set to 1 on all processes is why we
have the boot argument. Not setting audit=1 on the boot arguments means that
any process running before the audit daemon enables auditing can never ever be
audited because the only place its set is when processes are cloned.

Hope this clears up the use. NAK to the patch, it'll break auditing.

-Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Sorry for delay, vacation.

First of all, I do not pretend I understand this code. This was mostly
the question, and in fact I mostly asked about audit_bprm() in 0/1.

However,

On 08/30, Steve Grubb wrote:
> On Friday, August 30, 2013 03:06:46 PM Richard Guy Briggs wrote:
> > On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote:
> > > Btw. audit looks unmaintained... if you are going to take care of
> > > this code, perhaps you can look at
> > >
> > > http://marc.info/?l=linux-kernel&m=137589907108485
> > > http://marc.info/?l=linux-kernel&m=137590271809664
>
> You don't want to clear the TIF audit flag when context == NULL. What that will
> do is make a bunch of inauditable processes. There are times when audit is
> disabled and then re-enabled later. If the flag gets cleared, then a task's
> syscall will never enter the auditing framework from kernel/entry_64.S.
>
> That flag is 0 when auditing has never ever been enabled. If auditing is
> enabled, it should always be a 1 unless the task filter has determined that
> this process should not be audited ever. In practice, this is almost never
> used. But ensuring the TIF_SYSCALL_AUDIT set to 1 on all processes is why we
> have the boot argument. Not setting audit=1 on the boot arguments means that
> any process running before the audit daemon enables auditing can never ever be
> audited because the only place its set is when processes are cloned.

Then why audit_alloc() doesn't set TIF_SYSCALL_AUDIT unconditionally?

And I do not understand "when context == NULL" above. Say, audit_syscall_entry()
does nothing if !audit_context, and nobody except copy_process() does
audit_alloc(). So why do we need to trigger the audit's paths if it is NULL?

> Hope this clears up the use. NAK to the patch, it'll break auditing.

Not really, but thanks for your reply anyway.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 09/08, Oleg Nesterov wrote:
>
> First of all, I do not pretend I understand this code. This was mostly
> the question, and in fact I mostly asked about audit_bprm() in 0/1.
>
> However,
>
> On 08/30, Steve Grubb wrote:
> > On Friday, August 30, 2013 03:06:46 PM Richard Guy Briggs wrote:
> > > On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote:
> > > > Btw. audit looks unmaintained... if you are going to take care of
> > > > this code, perhaps you can look at
> > > >
> > > > http://marc.info/?l=linux-kernel&m=137589907108485
> > > > http://marc.info/?l=linux-kernel&m=137590271809664
> >
> > You don't want to clear the TIF audit flag when context == NULL. What that will
> > do is make a bunch of inauditable processes. There are times when audit is
> > disabled and then re-enabled later. If the flag gets cleared, then a task's
> > syscall will never enter the auditing framework from kernel/entry_64.S.
> >
> > That flag is 0 when auditing has never ever been enabled. If auditing is
> > enabled, it should always be a 1 unless the task filter has determined that
> > this process should not be audited ever. In practice, this is almost never
> > used. But ensuring the TIF_SYSCALL_AUDIT set to 1 on all processes is why we
> > have the boot argument. Not setting audit=1 on the boot arguments means that
> > any process running before the audit daemon enables auditing can never ever be
> > audited because the only place its set is when processes are cloned.
>
> Then why audit_alloc() doesn't set TIF_SYSCALL_AUDIT unconditionally?
>
> And I do not understand "when context == NULL" above. Say, audit_syscall_entry()
> does nothing if !audit_context, and nobody except copy_process() does
> audit_alloc(). So why do we need to trigger the audit's paths if it is NULL?
>
> > Hope this clears up the use. NAK to the patch, it'll break auditing.
>
> Not really, but thanks for your reply anyway.

So, Steve, do you still think that patch was wrong? Attached below
just in case.

Oleg.

[PATCH 1/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context

If audit_filter_task() nacks the new thread it makes sense
to clear TIF_SYSCALL_AUDIT which can be copied from parent
by dup_task_struct().

A wrong TIF_SYSCALL_AUDIT is not really bad, but it triggers
the "slow" audit paths in entry.S.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/auditsc.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 9845cb3..95293ab 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -943,8 +943,10 @@ int audit_alloc(struct task_struct *tsk)
return 0; /* Return if not auditing. */

state = audit_filter_task(tsk, &key);
- if (state == AUDIT_DISABLED)
+ if (state == AUDIT_DISABLED) {
+ clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
return 0;
+ }

if (!(context = audit_alloc_context(state))) {
kfree(key);
--
1.5.5.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sunday, September 08, 2013 05:54:35 PM Oleg Nesterov wrote:
> Sorry for delay, vacation.
>
> First of all, I do not pretend I understand this code. This was mostly
> the question, and in fact I mostly asked about audit_bprm() in 0/1.
>
> However,
>
> On 08/30, Steve Grubb wrote:
> > On Friday, August 30, 2013 03:06:46 PM Richard Guy Briggs wrote:
> > > On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote:
> > > > Btw. audit looks unmaintained... if you are going to take care of
> > > > this code, perhaps you can look at
> > > >
> > > > http://marc.info/?l=linux-kernel&m=137589907108485
> > > > http://marc.info/?l=linux-kernel&m=137590271809664
> >
> > You don't want to clear the TIF audit flag when context == NULL. What that
> > will do is make a bunch of inauditable processes. There are times when
> > audit is disabled and then re-enabled later. If the flag gets cleared,
> > then a task's syscall will never enter the auditing framework from
> > kernel/entry_64.S.
> >
> > That flag is 0 when auditing has never ever been enabled. If auditing is
> > enabled, it should always be a 1 unless the task filter has determined
> > that
> > this process should not be audited ever. In practice, this is almost never
> > used. But ensuring the TIF_SYSCALL_AUDIT set to 1 on all processes is why
> > we have the boot argument. Not setting audit=1 on the boot arguments
> > means that any process running before the audit daemon enables auditing
> > can never ever be audited because the only place its set is when
> > processes are cloned.
>
> Then why audit_alloc() doesn't set TIF_SYSCALL_AUDIT unconditionally?

The code I'm looking at does right at the end of the function.


> And I do not understand "when context == NULL" above. Say,
> audit_syscall_entry() does nothing if !audit_context, and nobody except
> copy_process() does audit_alloc(). So why do we need to trigger the audit's
> paths if it is NULL?

Because if you enter the audit framework, that means auditing has been turned
on at some point in the past, and could be turned back on at some point in the
future. If auditing has never been enabled, then you never enter the audit
framework at all. If the context is NULL, then audit is not currently enabled.

-Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tuesday, September 10, 2013 07:20:33 PM Oleg Nesterov wrote:
> On 09/08, Oleg Nesterov wrote:
> > First of all, I do not pretend I understand this code. This was mostly
> > the question, and in fact I mostly asked about audit_bprm() in 0/1.
> >
> > However,
> >
> > On 08/30, Steve Grubb wrote:
> > > On Friday, August 30, 2013 03:06:46 PM Richard Guy Briggs wrote:
> > > > On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote:
> > > > > Btw. audit looks unmaintained... if you are going to take care of
> > > > > this code, perhaps you can look at
> > > > >
> > > > > http://marc.info/?l=linux-kernel&m=137589907108485
> > > > > http://marc.info/?l=linux-kernel&m=137590271809664
> > >
> > > You don't want to clear the TIF audit flag when context == NULL. What
> > > that will do is make a bunch of inauditable processes. There are times
> > > when audit is disabled and then re-enabled later. If the flag gets
> > > cleared, then a task's syscall will never enter the auditing framework
> > > from kernel/entry_64.S.
> > >
> > > That flag is 0 when auditing has never ever been enabled. If auditing is
> > > enabled, it should always be a 1 unless the task filter has determined
> > > that
> > > this process should not be audited ever. In practice, this is almost
> > > never
> > > used. But ensuring the TIF_SYSCALL_AUDIT set to 1 on all processes is
> > > why we have the boot argument. Not setting audit=1 on the boot
> > > arguments means that any process running before the audit daemon
> > > enables auditing can never ever be audited because the only place its
> > > set is when processes are cloned.>
> > Then why audit_alloc() doesn't set TIF_SYSCALL_AUDIT unconditionally?
> >
> > And I do not understand "when context == NULL" above. Say,
> > audit_syscall_entry() does nothing if !audit_context, and nobody except
> > copy_process() does audit_alloc(). So why do we need to trigger the
> > audit's paths if it is NULL?>
> > > Hope this clears up the use. NAK to the patch, it'll break auditing.
> >
> > Not really, but thanks for your reply anyway.
>
> So, Steve, do you still think that patch was wrong? Attached below
> just in case.

I think this looks OK. If the task filter NACK's auditing the process, then
clearing the flag is probably correct. I have design notes from back around the
2.6.7 kernel saying this was the intention.

ACK.

-Steve


> [PATCH 1/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context
>
> If audit_filter_task() nacks the new thread it makes sense
> to clear TIF_SYSCALL_AUDIT which can be copied from parent
> by dup_task_struct().
>
> A wrong TIF_SYSCALL_AUDIT is not really bad, but it triggers
> the "slow" audit paths in entry.S.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/auditsc.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9845cb3..95293ab 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -943,8 +943,10 @@ int audit_alloc(struct task_struct *tsk)
> return 0; /* Return if not auditing. */
>
> state = audit_filter_task(tsk, &key);
> - if (state == AUDIT_DISABLED)
> + if (state == AUDIT_DISABLED) {
> + clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
> return 0;
> + }
>
> if (!(context = audit_alloc_context(state))) {
> kfree(key);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 09/13, Steve Grubb wrote:
>
> On Sunday, September 08, 2013 05:54:35 PM Oleg Nesterov wrote:
> >
> > Then why audit_alloc() doesn't set TIF_SYSCALL_AUDIT unconditionally?
>
> The code I'm looking at does right at the end of the function.

The code I'm looking at does right at the end too ;) but it also
returns at the start if audit_filter_task() returns AUDIT_DISABLED.

> > And I do not understand "when context == NULL" above. Say,
> > audit_syscall_entry() does nothing if !audit_context, and nobody except
> > copy_process() does audit_alloc(). So why do we need to trigger the audit's
> > paths if it is NULL?
>
> Because if you enter the audit framework,

framework? TIF_SYSCALL_AUDIT has only meaning in entry.S, we need it
to ensure that the audited task can't miss audit_syscall_*().

> that means auditing has been turned
> on at some point in the past, and could be turned back on at some point in the
> future.

And this will change nothing, afaics (wrt TIF_SYSCALL_AUDIT).

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 09/13, Steve Grubb wrote:
>
> On Tuesday, September 10, 2013 07:20:33 PM Oleg Nesterov wrote:
> >
> > So, Steve, do you still think that patch was wrong? Attached below
> > just in case.
>
> I think this looks OK. If the task filter NACK's auditing the process, then
> clearing the flag is probably correct. I have design notes from back around the
> 2.6.7 kernel saying this was the intention.

Then I do not really understand your previous email... Nevermind ;)

> ACK.

Thanks. I'll resend this patch with your ack applied.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/