On Tue, Apr 09, 2024 at 08:25:45PM +0200, Marco Elver wrote:
> On Tue, Apr 09, 2024 at 08:46AM -0700, Kees Cook wrote:
> [...]
> > > + trace_new_exec(current, bprm);
> > > +
> >
> > All other steps in this function have explicit comments about
> > what/why/etc. Please add some kind of comment describing why the
> > tracepoint is where it is, etc.
>
> I beefed up the tracepoint documentation, and wrote a little paragraph
> above where it's called to reinforce what we want.
>
> [...]
> > What about binfmt_misc, and binfmt_script? You may want bprm->interp
> > too?
>
> Good points. I'll make the below changes for v2:
>
> diff --git a/fs/exec.c b/fs/exec.c
> index ab778ae1fc06..472b9f7b40e8 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1268,6 +1268,12 @@ int begin_new_exec(struct linux_binprm * bprm)
> if (retval)
> return retval;
>
> + /*
> + * This tracepoint marks the point before flushing the old exec where
> + * the current task is still unchanged, but errors are fatal (point of
> + * no return). The later "sched_process_exec" tracepoint is called after
> + * the current task has successfully switched to the new exec.
> + */
> trace_new_exec(current, bprm);
>
> /*
> diff --git a/include/trace/events/task.h b/include/trace/events/task.h
> index 8853dc44783d..623d9af777c1 100644
> --- a/include/trace/events/task.h
> +++ b/include/trace/events/task.h
> @@ -61,8 +61,11 @@ TRACE_EVENT(task_rename,
> * @task: pointer to the current task
> * @bprm: pointer to linux_binprm used for new exec
> *
> - * Called before flushing the old exec, but at the point of no return during
> - * switching to the new exec.
> + * Called before flushing the old exec, where @task is still unchanged, but at
> + * the point of no return during switching to the new exec. At the point it is
> + * called the exec will either succeed, or on failure terminate the task. Also
> + * see the "sched_process_exec" tracepoint, which is called right after @task
> + * has successfully switched to the new exec.
> */
> TRACE_EVENT(new_exec,
>
> @@ -71,19 +74,22 @@ TRACE_EVENT(new_exec,
> TP_ARGS(task, bprm),
>
> TP_STRUCT__entry(
> + __string( interp, bprm->interp )
> __string( filename, bprm->filename )
> __field( pid_t, pid )
> __string( comm, task->comm )
> ),
>
> TP_fast_assign(
> + __assign_str(interp, bprm->interp);
> __assign_str(filename, bprm->filename);
> __entry->pid = task->pid;
> __assign_str(comm, task->comm);
> ),
>
> - TP_printk("filename=%s pid=%d comm=%s",
> - __get_str(filename), __entry->pid, __get_str(comm))
> + TP_printk("interp=%s filename=%s pid=%d comm=%s",
> + __get_str(interp), __get_str(filename),
> + __entry->pid, __get_str(comm))
> );
>
> #endif
Looks good; I await v2, and Steven's Ack. :)
--
Kees Cook
