On Wed, Feb 7, 2024 at 9:34 AM Kees Cook [off-list ref] wrote:

On Wed, Feb 07, 2024 at 08:10:55PM +0900, Tetsuo Handa wrote:

quoted

On 2024/02/07 9:00, Paul Moore wrote:

quoted

quoted

@@ -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.

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.

-- 
paul-moore.com