On Wed, May 1, 2024 at 4:04 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> 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);
>
Tetsuo, it's been a while since we've heard from you in this thread -
are you still planning to work on this? If not, would you object if
someone else took over this patchset?
--
paul-moore.com
