| Message ID | 20200910202107.3799376-4-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fork brute force attack mitigation (fbfam) | expand |
On Thu, Sep 10, 2020 at 10:21 PM Kees Cook <keescook@chromium.org> wrote: > Use the previous defined api to manage statistics calling it accordingly > when a task forks, calls execve or exits. You defined functions that return error codes in the previous patch, but here you ignore the return values. That's a bad idea. You should probably check the return value in execve() (and fail the execution in the case where memory allocation fails), and make it so that the other functions always succeed.
On Thu, Sep 10, 2020 at 01:21:04PM -0700, Kees Cook wrote: > From: John Wood <john.wood@gmx.com> > > Use the previous defined api to manage statistics calling it accordingly > when a task forks, calls execve or exits. > > Signed-off-by: John Wood <john.wood@gmx.com> > --- > fs/exec.c | 2 ++ > kernel/exit.c | 2 ++ > kernel/fork.c | 4 ++++ > 3 files changed, 8 insertions(+) > > diff --git a/fs/exec.c b/fs/exec.c > index a91003e28eaa..b30118674d32 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -71,6 +71,7 @@ > #include "internal.h" > > #include <trace/events/sched.h> > +#include <fbfam/fbfam.h> > > static int bprm_creds_from_file(struct linux_binprm *bprm); > > @@ -1940,6 +1941,7 @@ static int bprm_execve(struct linux_binprm *bprm, > task_numa_free(current, false); > if (displaced) > put_files_struct(displaced); > + fbfam_execve(); As mentioned in the other emails, I think this could trivially be converted into an LSM: all the hooks are available AFAICT. If you only want to introspect execve _happening_, you can use bprm_creds_for_exec which is called a few lines above. Otherwise, my prior suggestion ("the exec has happened" hook via brpm_cred_committing, etc). > return retval; > > out: > diff --git a/kernel/exit.c b/kernel/exit.c > index 733e80f334e7..39a6139dcf31 100644 > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -67,6 +67,7 @@ > #include <linux/uaccess.h> > #include <asm/unistd.h> > #include <asm/mmu_context.h> > +#include <fbfam/fbfam.h> > > static void __unhash_process(struct task_struct *p, bool group_dead) > { > @@ -852,6 +853,7 @@ void __noreturn do_exit(long code) > __this_cpu_add(dirty_throttle_leaks, tsk->nr_dirtied); > exit_rcu(); > exit_tasks_rcu_finish(); > + fbfam_exit(); > > lockdep_free_task(tsk); > do_task_dead(); The place for this would be put_task_struct, and the LSM hook is task_free. :) (The only caveat with task_free hook is that it may be called in non-process context due to being freed during RCU, etc. In practice, this is unlikely to cause problems.) > diff --git a/kernel/fork.c b/kernel/fork.c > index 49677d668de4..c933838450a8 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -107,6 +107,8 @@ > #define CREATE_TRACE_POINTS > #include <trace/events/task.h> > > +#include <fbfam/fbfam.h> > + > /* > * Minimum number of threads to boot the kernel > */ > @@ -941,6 +943,8 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node) > #ifdef CONFIG_MEMCG > tsk->active_memcg = NULL; > #endif > + > + fbfam_fork(tsk); > return tsk; Since you don't need "orig", this is also trivially an LSM hook. dup_task_struct() is called by copy_process(), which will also call the task_alloc LSM hook later on. > > free_stack: > -- > 2.25.1 >
On Thu, 10 Sep 2020 16:33:38 -0700 Kees Cook <keescook@chromium.org> wrote: > > @@ -1940,6 +1941,7 @@ static int bprm_execve(struct linux_binprm *bprm, > > task_numa_free(current, false); > > if (displaced) > > put_files_struct(displaced); > > + fbfam_execve(); > > As mentioned in the other emails, I think this could trivially be > converted into an LSM: all the hooks are available AFAICT. If you only > want to introspect execve _happening_, you can use bprm_creds_for_exec > which is called a few lines above. Otherwise, my prior suggestion ("the > exec has happened" hook via brpm_cred_committing, etc). And if its information only, you could just register a callback to the trace_sched_process_exec() tracepoint and do whatever you want then. The tracepoints are available for anyone to attach to. Not just tracing. -- Steve
On Tue, 29 Sep 2020 19:47:12 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Thu, 10 Sep 2020 16:33:38 -0700 > Kees Cook <keescook@chromium.org> wrote: > > > > @@ -1940,6 +1941,7 @@ static int bprm_execve(struct linux_binprm *bprm, > > > task_numa_free(current, false); > > > if (displaced) > > > put_files_struct(displaced); > > > + fbfam_execve(); > > > > As mentioned in the other emails, I think this could trivially be > > converted into an LSM: all the hooks are available AFAICT. If you only > > want to introspect execve _happening_, you can use bprm_creds_for_exec > > which is called a few lines above. Otherwise, my prior suggestion ("the > > exec has happened" hook via brpm_cred_committing, etc). > > And if its information only, you could just register a callback to the > trace_sched_process_exec() tracepoint and do whatever you want then. > > The tracepoints are available for anyone to attach to. Not just tracing. > And there's also trace_sched_process_fork() and trace_sched_process_exit(). -- Steve
Hi Steven, On Tue, Sep 29, 2020 at 07:49:24PM -0400, Steven Rostedt wrote: > On Tue, 29 Sep 2020 19:47:12 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Thu, 10 Sep 2020 16:33:38 -0700 > > Kees Cook <keescook@chromium.org> wrote: > > > > > > @@ -1940,6 +1941,7 @@ static int bprm_execve(struct linux_binprm *bprm, > > > > task_numa_free(current, false); > > > > if (displaced) > > > > put_files_struct(displaced); > > > > + fbfam_execve(); > > > > > > As mentioned in the other emails, I think this could trivially be > > > converted into an LSM: all the hooks are available AFAICT. If you only > > > want to introspect execve _happening_, you can use bprm_creds_for_exec > > > which is called a few lines above. Otherwise, my prior suggestion ("the > > > exec has happened" hook via brpm_cred_committing, etc). > > > > And if its information only, you could just register a callback to the > > trace_sched_process_exec() tracepoint and do whatever you want then. > > > > The tracepoints are available for anyone to attach to. Not just tracing. > > > And there's also trace_sched_process_fork() and > trace_sched_process_exit(). Since this feature requires a pointer to the statistical data in the task_struct structure, and the LSM allows this using the security blobs, I think that the best for now is convert all the code to an LSM. Anyway, thanks for the suggestion. > -- Steve Thanks, John Wood
diff --git a/fs/exec.c b/fs/exec.c index a91003e28eaa..b30118674d32 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -71,6 +71,7 @@ #include "internal.h" #include <trace/events/sched.h> +#include <fbfam/fbfam.h> static int bprm_creds_from_file(struct linux_binprm *bprm); @@ -1940,6 +1941,7 @@ static int bprm_execve(struct linux_binprm *bprm, task_numa_free(current, false); if (displaced) put_files_struct(displaced); + fbfam_execve(); return retval; out: diff --git a/kernel/exit.c b/kernel/exit.c index 733e80f334e7..39a6139dcf31 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -67,6 +67,7 @@ #include <linux/uaccess.h> #include <asm/unistd.h> #include <asm/mmu_context.h> +#include <fbfam/fbfam.h> static void __unhash_process(struct task_struct *p, bool group_dead) { @@ -852,6 +853,7 @@ void __noreturn do_exit(long code) __this_cpu_add(dirty_throttle_leaks, tsk->nr_dirtied); exit_rcu(); exit_tasks_rcu_finish(); + fbfam_exit(); lockdep_free_task(tsk); do_task_dead(); diff --git a/kernel/fork.c b/kernel/fork.c index 49677d668de4..c933838450a8 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -107,6 +107,8 @@ #define CREATE_TRACE_POINTS #include <trace/events/task.h> +#include <fbfam/fbfam.h> + /* * Minimum number of threads to boot the kernel */ @@ -941,6 +943,8 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node) #ifdef CONFIG_MEMCG tsk->active_memcg = NULL; #endif + + fbfam_fork(tsk); return tsk; free_stack: