[PATCH v3 1/3] LSM: add security_execve_abort() hook

[Kees Cook]

On Thu, Feb 15, 2024 at 11:33:32PM +0900, Tetsuo Handa wrote:
> On 2024/02/15 6:46, Paul Moore wrote:
> >> To quickly summarize, there are two paths forward that I believe are
> >> acceptable from a LSM perspective, pick either one and send me an
> >> updated patchset.
> >>
> >> 1. Rename the hook to security_bprm_free() and update the LSM hook
> >> description as I mentioned earlier in this thread.
> >>
> >> 2. Rename the hook to security_execve_revert(), move it into the
> >> execve related functions, and update the LSM hook description to
> >> reflect that this hook is for reverting execve related changes to the
> >> current task's internal LSM state beyond what is possible via the
> >> credential hooks.
> > 
> > Hi Tetsuo, I just wanted to check on this and see if you've been able
> > to make any progress?
> > 
> 
> I'm fine with either approach. Just worrying that someone doesn't like
> overhead of unconditionally calling security_bprm_free() hook.

With the coming static calls series, this concern will delightfully go
away. :)

> If everyone is fine with below one, I'll post v4 patchset.

I'm okay with it being security_bprm_free(). One question I had was how
Tomoyo deals with it? I was depending on the earlier hook only being
called in a failure path.

> [...]
> @@ -1530,6 +1530,7 @@ static void free_bprm(struct linux_binprm *bprm)
>  		kfree(bprm->interp);
>  	kfree(bprm->fdpath);
>  	kfree(bprm);
> +	security_bprm_free();
>  }

I'm fine with security_bprm_free(), but this needs to be moved to the
start of free_bprm(), and to pass the bprm itself. This is the pattern we
use for all the other "free" hooks. (Though in this case we don't attach
any security context to the brpm, but there may be state of interest in
it.) i.e.:

diff --git a/fs/exec.c b/fs/exec.c
index 40073142288f..7ec13b104960 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1532,6 +1532,7 @@ static void do_close_execat(struct file *file)
 
 static void free_bprm(struct linux_binprm *bprm)
 {
+	security_bprm_free(bprm);
 	if (bprm->mm) {
 		acct_arg_size(bprm, 0);
 		mmput(bprm->mm);

-- 
Kees Cook