From: John Wood <[email protected]>
Use the previous defined api to manage statistics calling it accordingly
when a task forks, calls execve or exits.
Signed-off-by: John Wood <[email protected]>
---
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();
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:
--
2.25.1
On Thu, Sep 10, 2020 at 10:21 PM Kees Cook <[email protected]> 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 <[email protected]>
>
> Use the previous defined api to manage statistics calling it accordingly
> when a task forks, calls execve or exits.
>
> Signed-off-by: John Wood <[email protected]>
> ---
> 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
>
--
Kees Cook
On Thu, 10 Sep 2020 16:33:38 -0700
Kees Cook <[email protected]> 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 <[email protected]> wrote:
> On Thu, 10 Sep 2020 16:33:38 -0700
> Kees Cook <[email protected]> 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 <[email protected]> wrote:
>
> > On Thu, 10 Sep 2020 16:33:38 -0700
> > Kees Cook <[email protected]> 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