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

[Kees Cook <keescook@chromium.org>]

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@email.froward.int.ebiederm.org .

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