* [PATCH 0/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context
@ 2013-08-07 18:05 Oleg Nesterov
2013-08-07 18:05 ` [PATCH 1/1] " Oleg Nesterov
0 siblings, 1 reply; 4+ messages in thread
From: Oleg Nesterov @ 2013-08-07 18:05 UTC (permalink / raw)
To: Al Viro, Eric Paris, Kees Cook; +Cc: Andrew Morton, Zach Levis, linux-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context
2013-08-07 18:05 [PATCH 0/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context Oleg Nesterov
@ 2013-08-07 18:05 ` Oleg Nesterov
2013-08-07 18:53 ` Kees Cook
0 siblings, 1 reply; 4+ messages in thread
From: Oleg Nesterov @ 2013-08-07 18:05 UTC (permalink / raw)
To: Al Viro, Eric Paris, Kees Cook; +Cc: Andrew Morton, Zach Levis, linux-kernel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context
2013-08-07 18:05 ` [PATCH 1/1] " Oleg Nesterov
@ 2013-08-07 18:53 ` Kees Cook
2013-08-07 19:06 ` Oleg Nesterov
0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2013-08-07 18:53 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Al Viro, Eric Paris, Andrew Morton, Zach Levis, LKML
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context
2013-08-07 18:53 ` Kees Cook
@ 2013-08-07 19:06 ` Oleg Nesterov
0 siblings, 0 replies; 4+ messages in thread
From: Oleg Nesterov @ 2013-08-07 19:06 UTC (permalink / raw)
To: Kees Cook; +Cc: Al Viro, Eric Paris, Andrew Morton, Zach Levis, LKML
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-08-07 19:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-07 18:05 [PATCH 0/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context Oleg Nesterov
2013-08-07 18:05 ` [PATCH 1/1] " Oleg Nesterov
2013-08-07 18:53 ` Kees Cook
2013-08-07 19:06 ` Oleg Nesterov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.