audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context
cover audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context
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
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.
--- kernel/auditsc.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
kernel/auditsc.c +3 -1
@@ -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);
Signature · 1 line
1.5.5.1
Quoted 29 lines · Oleg Nesterov
If
audit_filter_task()nacks the new thread it makes sense to clearTIF_SYSCALL_AUDITwhich can be copied from parent bydup_task_struct().A wrong
TIF_SYSCALL_AUDITis not really bad, but it triggers the "slow" audit paths inentry.S.Signed-off-by Oleg Nesterov <[email protected]>--- kernel/auditsc.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)kernel/auditsc.c +3
-2@@ -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); --
Quoted 3 lines
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
Signature · 2 lines
Kees Cook Chrome OS Security
Quoted 27 lines · Kees Cook
Quoted 18 lines · Oleg Nesterov
kernel/auditsc.c +3
-2@@ -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); --Quoted 3 lines
1.5.5.1
Interestingly, I see no other callers of "
clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT)" :)
Quoted 3 lines
This patch seems right, since
TIF_SYSCALL_AUDITis set only whenaudit_filter_task!=AUDIT_DISABLED. Though maybe this should be explicitly handled indup_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.