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.
-Kees
--
Kees Cook
