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

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

[Oleg Nesterov]

Even if I am right the problem is really minor, the patch should
be ignored without the authoritative acks.

I am only sending it because I am curious, and I would like to ask
another question about audit.


search_binary_handler()->audit_bprm(bprm) looks a bit strange, and
I can't understand if this is on purpose or not.

It adds the the AUDIT_EXECVE record every time the (recursive)
search_binary_handler() is called for audit_log_exit() which flushes
them all. But probably we only need a single record ?

And it seems that audit_aux_data_execve->envc is not used at all.

Oleg.

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

[Oleg Nesterov]

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

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

[Kees Cook]

On Wed, Aug 7, 2013 at 11:05 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> 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
>
>

Interestingly, I see no other callers of "clear_tsk_thread_flag(tsk,
TIF_SYSCALL_AUDIT)" :)

This patch seems right, since TIF_SYSCALL_AUDIT is set only when
audit_filter_task != AUDIT_DISABLED. Though maybe this should be
explicitly handled in dup_task_struct()?

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Oleg Nesterov]

On 08/07, Kees Cook wrote:
>
> On Wed, Aug 7, 2013 at 11:05 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > 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
> >
> >
>
> Interestingly, I see no other callers of "clear_tsk_thread_flag(tsk,
> TIF_SYSCALL_AUDIT)" :)
>
> This patch seems right, since TIF_SYSCALL_AUDIT is set only when
> audit_filter_task != AUDIT_DISABLED. Though maybe this should be
> explicitly handled in dup_task_struct()?

Personally I agree.

But note that we have more clear-tif-after-fork's. IOW, I think we
should probably add copy_tif_flags() or something which acts like
copy_flags(). TIF_SYSCALL_TRACE, TIF_SYSCALL_EMU, TIF_NEED_RESCHED
at least. Until then we should follow the copy-them-all-then-clear
rule.

Besides, if we add the new clear_tsk_thread_flag() into copy_process's
path we probably need ifdef or ifdef'ed helper.

Oleg.