On Wed, Feb 7, 2024 at 11:01 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Wed, Feb 7, 2024 at 9:34 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> > On Wed, Feb 07, 2024 at 08:10:55PM +0900, Tetsuo Handa wrote:
> > > On 2024/02/07 9:00, Paul Moore wrote:
> > > >> @@ -1223,6 +1223,17 @@ void security_bprm_committed_creds(const struct linux_binprm *bprm)
> > > >> call_void_hook(bprm_committed_creds, bprm);
> > > >> }
> > > >>
> > > >> +/**
> > > >> + * security_execve_abort() - Notify that exec() has failed
> > > >> + *
> > > >> + * This hook is for undoing changes which cannot be discarded by
> > > >> + * abort_creds().
> > > >> + */
> > > >> +void security_execve_abort(void)
> > > >> +{
> > > >> + call_void_hook(execve_abort);
> > > >> +}
> > > >
> > > > I don't have a problem with reinstating something like
> > > > security_bprm_free(), but I don't like the name security_execve_abort(),
> > > > especially given that it is being called from alloc_bprm() as well as
> > > > all of the execve code. At the risk of bikeshedding this, I'd be much
> > > > happier if this hook were renamed to security_bprm_free() and the
> > > > hook's description explained that this hook is called when a linux_bprm
> > > > instance is being destroyed, after the bprm creds have been released,
> > > > and is intended to cleanup any internal LSM state associated with the
> > > > linux_bprm instance.
> > > >
> > > > Are you okay with that?
> > >
> > > Hmm, that will bring us back to v1 of this series.
> > >
> > > v3 was based on Eric W. Biederman's suggestion
> > >
> > > If you aren't going to change your design your new hook should be:
> > > security_execve_revert(current);
> > > Or maybe:
> > > security_execve_abort(current);
> > >
> > > At least then it is based upon the reality that you plan to revert
> > > changes to current->security. Saying anything about creds or bprm when
> > > you don't touch them, makes no sense at all. Causing people to
> > > completely misunderstand what is going on, and making it more likely
> > > they will change the code in ways that will break TOMOYO.
> > >
> > > at https://lkml.kernel.org/r/8734ug9fbt.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx .
> >
> > Yeah, I'd agree with Eric on this: it's not about bprm freeing, it's
> > catching the execve failure. I think the name is accurate -- it mirrors
> > the abort_creds() call.
>
> I'm sorry, but I would still much rather prefer security_bprm_free()
> both to reflect the nature of the caller as well as to abstract away a
> particular use case; this is also why I suggested a different hook
> description for the function header block.
>
> If you really want this to be focused on reverting the execvc changes
> (I do agree with Eric about "revert" over "abort") then please move
> this hook out of free_bprm() and into do_execveat_common() and
> kernel_execve().
>
> 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?
--
paul-moore.com
