[PATCH v3 0/3] fs/exec: remove current->in_execve flag

[PATCH v3 0/3] fs/exec: remove current->in_execve flag

[Tetsuo Handa]

[This is for Linux 6.8-rc4. Please apply directly, for tomoyo's git tree
 in osdn.net is still down. I'm planning to move tomoyo's tree to github
 or sourceforge.net because there seems to be no plan to recover osdn.net.]

This is a follow up series for removing current->in_execve flag.
https://lkml.kernel.org/r/b5a12ecd-468d-4b50-9f8c-17ae2a2560b4@I-love.SAKURA.ne.jp

[PATCH v3 1/3] LSM: add security_execve_abort() hook
[PATCH v3 2/3] tomoyo: replace current->in_execve flag with security_execve_abort() hook
[PATCH v3 3/3] fs/exec: remove current->in_execve flag

 fs/exec.c                     |    4 +---
 include/linux/lsm_hook_defs.h |    1 +
 include/linux/sched.h         |    3 ---
 include/linux/security.h      |    5 +++++
 security/security.c           |   11 +++++++++++
 security/tomoyo/tomoyo.c      |   22 +++++-----------------
 6 files changed, 23 insertions(+), 23 deletions(-)

Changes in v3:

  Removed excess function parameter description, reported by
  kernel test robot <lkp@intel.com>.

  Added Acked-by: from Serge Hallyn.

Changes in v2:

  Replace security_bprm_aborting_creds(const struct linux_binprm *bprm) with
  security_execve_abort(void), suggested by Eric W. Biederman.

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

[Tetsuo Handa]

A regression caused by commit 978ffcbf00d8 ("execve: open the executable
file before doing anything else") has been fixed by commit 4759ff71f23e
("exec: Check __FMODE_EXEC instead of in_execve for LSMs") and commit
3eab830189d9 ("uselib: remove use of __FMODE_EXEC"). While fixing this
regression, Linus commented that we want to remove current->in_execve flag.

The current->in_execve flag was introduced by commit f9ce1f1cda8b ("Add
in_execve flag into task_struct.") when TOMOYO LSM was merged, and the
reason was explained in commit f7433243770c ("LSM adapter functions.").

In short, TOMOYO's design is not compatible with COW credential model
introduced in Linux 2.6.29, and the current->in_execve flag was added for
emulating security_bprm_free() hook which has been removed by introduction
of COW credential model.

security_task_alloc()/security_task_free() hooks have been removed by
commit f1752eec6145 ("CRED: Detach the credentials from task_struct"),
and these hooks have been revived by commit 1a2a4d06e1e9 ("security:
create task_free security callback") and commit e4e55b47ed9a ("LSM: Revive
security_task_alloc() hook and per "struct task_struct" security blob.").

But security_bprm_free() hook did not revive until now. Now that Linus
wants TOMOYO to stop carrying state across two independent execve() calls,
and TOMOYO can stop carrying state if a hook for restoring previous state
upon failed execve() call were provided, this patch revives the hook.

Since security_bprm_committing_creds() and security_bprm_committed_creds()
hooks are called when an execve() request succeeded, we don't need to call
security_bprm_free() hook when an execve() request succeeded. Therefore,
this patch adds security_execve_abort() hook which is called only when an
execve() request failed after successful prepare_bprm_creds() call.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Serge E. Hallyn <serge@hallyn.com>
---
 fs/exec.c                     |  1 +
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  5 +++++
 security/security.c           | 11 +++++++++++
 4 files changed, 18 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index af4fbb61cd53..d6d35a06fd08 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1521,6 +1521,7 @@ static void free_bprm(struct linux_binprm *bprm)
 	if (bprm->cred) {
 		mutex_unlock(&current->signal->cred_guard_mutex);
 		abort_creds(bprm->cred);
+		security_execve_abort();
 	}
 	do_close_execat(bprm->file);
 	if (bprm->executable)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 76458b6d53da..fd100ab71a33 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -54,6 +54,7 @@ LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct f
 LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
 LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, const struct linux_binprm *bprm)
 LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, const struct linux_binprm *bprm)
+LSM_HOOK(void, LSM_RET_VOID, execve_abort, void)
 LSM_HOOK(int, 0, fs_context_submount, struct fs_context *fc, struct super_block *reference)
 LSM_HOOK(int, 0, fs_context_dup, struct fs_context *fc,
 	 struct fs_context *src_sc)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..31532b30c4f0 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -299,6 +299,7 @@ int security_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *
 int security_bprm_check(struct linux_binprm *bprm);
 void security_bprm_committing_creds(const struct linux_binprm *bprm);
 void security_bprm_committed_creds(const struct linux_binprm *bprm);
+void security_execve_abort(void);
 int security_fs_context_submount(struct fs_context *fc, struct super_block *reference);
 int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc);
 int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param);
@@ -648,6 +649,10 @@ static inline void security_bprm_committed_creds(const struct linux_binprm *bprm
 {
 }
 
+static inline void security_execve_abort(void)
+{
+}
+
 static inline int security_fs_context_submount(struct fs_context *fc,
 					   struct super_block *reference)
 {
diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..10adc4d3c5e0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -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);
+}
+
 /**
  * security_fs_context_submount() - Initialise fc->security
  * @fc: new filesystem context
-- 
2.18.4

[PATCH v3 2/3] tomoyo: replace current->in_execve flag with security_execve_abort() hook

[Tetsuo Handa]

TOMOYO was using current->in_execve flag in order to restore previous state
when previous execve() request failed. Since security_execve_abort() hook
was added, switch to use it.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Serge E. Hallyn <serge@hallyn.com>
---
 security/tomoyo/tomoyo.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 04a92c3d65d4..a11dba3a9753 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -18,34 +18,22 @@ struct tomoyo_domain_info *tomoyo_domain(void)
 {
 	struct tomoyo_task *s = tomoyo_task(current);
 
-	if (s->old_domain_info && !current->in_execve) {
-		atomic_dec(&s->old_domain_info->users);
-		s->old_domain_info = NULL;
-	}
 	return s->domain_info;
 }
 
 /**
- * tomoyo_cred_prepare - Target for security_prepare_creds().
- *
- * @new: Pointer to "struct cred".
- * @old: Pointer to "struct cred".
- * @gfp: Memory allocation flags.
- *
- * Returns 0.
+ * tomoyo_execve_abort - Target for security_execve_abort().
  */
-static int tomoyo_cred_prepare(struct cred *new, const struct cred *old,
-			       gfp_t gfp)
+static void tomoyo_execve_abort(void)
 {
-	/* Restore old_domain_info saved by previous execve() request. */
+	/* Restore old_domain_info saved by execve() request. */
 	struct tomoyo_task *s = tomoyo_task(current);
 
-	if (s->old_domain_info && !current->in_execve) {
+	if (s->old_domain_info) {
 		atomic_dec(&s->domain_info->users);
 		s->domain_info = s->old_domain_info;
 		s->old_domain_info = NULL;
 	}
-	return 0;
 }
 
 /**
@@ -554,8 +542,8 @@ static const struct lsm_id tomoyo_lsmid = {
  * registering TOMOYO.
  */
 static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
-	LSM_HOOK_INIT(cred_prepare, tomoyo_cred_prepare),
 	LSM_HOOK_INIT(bprm_committed_creds, tomoyo_bprm_committed_creds),
+	LSM_HOOK_INIT(execve_abort, tomoyo_execve_abort),
 	LSM_HOOK_INIT(task_alloc, tomoyo_task_alloc),
 	LSM_HOOK_INIT(task_free, tomoyo_task_free),
 #ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER
-- 
2.18.4

[PATCH v3 3/3] fs/exec: remove current->in_execve flag

[Tetsuo Handa]

Addition of security_execve_abort() hook made it possible to remove
this flag.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Serge E. Hallyn <serge@hallyn.com>
---
 fs/exec.c             | 3 ---
 include/linux/sched.h | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index d6d35a06fd08..c197573b2940 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1865,7 +1865,6 @@ static int bprm_execve(struct linux_binprm *bprm)
 	 * where setuid-ness is evaluated.
 	 */
 	check_unsafe_exec(bprm);
-	current->in_execve = 1;
 	sched_mm_cid_before_execve(current);
 
 	sched_exec();
@@ -1882,7 +1881,6 @@ static int bprm_execve(struct linux_binprm *bprm)
 	sched_mm_cid_after_execve(current);
 	/* execve succeeded */
 	current->fs->in_exec = 0;
-	current->in_execve = 0;
 	rseq_execve(current);
 	user_events_execve(current);
 	acct_update_integrals(current);
@@ -1901,7 +1899,6 @@ static int bprm_execve(struct linux_binprm *bprm)
 
 	sched_mm_cid_after_execve(current);
 	current->fs->in_exec = 0;
-	current->in_execve = 0;
 
 	return retval;
 }
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ffe8f618ab86..66ada87249b1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -919,9 +919,6 @@ struct task_struct {
 #ifdef CONFIG_RT_MUTEXES
 	unsigned			sched_rt_mutex:1;
 #endif
-
-	/* Bit to tell TOMOYO we're in execve(): */
-	unsigned			in_execve:1;
 	unsigned			in_iowait:1;
 #ifndef TIF_RESTORE_SIGMASK
 	unsigned			restore_sigmask:1;
-- 
2.18.4

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

[Paul Moore]

On Feb  6, 2024 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> A regression caused by commit 978ffcbf00d8 ("execve: open the executable
> file before doing anything else") has been fixed by commit 4759ff71f23e
> ("exec: Check __FMODE_EXEC instead of in_execve for LSMs") and commit
> 3eab830189d9 ("uselib: remove use of __FMODE_EXEC"). While fixing this
> regression, Linus commented that we want to remove current->in_execve flag.
> 
> The current->in_execve flag was introduced by commit f9ce1f1cda8b ("Add
> in_execve flag into task_struct.") when TOMOYO LSM was merged, and the
> reason was explained in commit f7433243770c ("LSM adapter functions.").
> 
> In short, TOMOYO's design is not compatible with COW credential model
> introduced in Linux 2.6.29, and the current->in_execve flag was added for
> emulating security_bprm_free() hook which has been removed by introduction
> of COW credential model.

If you wanted to mention the relevant commit where security_bprm_free()
was removed, it was a6f76f23d297 ("CRED: Make execve() take advantage
of copy-on-write credentials").

> security_task_alloc()/security_task_free() hooks have been removed by
> commit f1752eec6145 ("CRED: Detach the credentials from task_struct"),
> and these hooks have been revived by commit 1a2a4d06e1e9 ("security:
> create task_free security callback") and commit e4e55b47ed9a ("LSM: Revive
> security_task_alloc() hook and per "struct task_struct" security blob.").
> 
> But security_bprm_free() hook did not revive until now. Now that Linus
> wants TOMOYO to stop carrying state across two independent execve() calls,
> and TOMOYO can stop carrying state if a hook for restoring previous state
> upon failed execve() call were provided, this patch revives the hook.
> 
> Since security_bprm_committing_creds() and security_bprm_committed_creds()
> hooks are called when an execve() request succeeded, we don't need to call
> security_bprm_free() hook when an execve() request succeeded. Therefore,
> this patch adds security_execve_abort() hook which is called only when an
> execve() request failed after successful prepare_bprm_creds() call.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Acked-by: Serge E. Hallyn <serge@hallyn.com>
> ---
>  fs/exec.c                     |  1 +
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  5 +++++
>  security/security.c           | 11 +++++++++++
>  4 files changed, 18 insertions(+)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index af4fbb61cd53..d6d35a06fd08 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1521,6 +1521,7 @@ static void free_bprm(struct linux_binprm *bprm)
>  	if (bprm->cred) {
>  		mutex_unlock(&current->signal->cred_guard_mutex);
>  		abort_creds(bprm->cred);
> +		security_execve_abort();
>  	}
>  	do_close_execat(bprm->file);
>  	if (bprm->executable)
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 76458b6d53da..fd100ab71a33 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -54,6 +54,7 @@ LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct f
>  LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
>  LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, const struct linux_binprm *bprm)
>  LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, const struct linux_binprm *bprm)
> +LSM_HOOK(void, LSM_RET_VOID, execve_abort, void)
>  LSM_HOOK(int, 0, fs_context_submount, struct fs_context *fc, struct super_block *reference)
>  LSM_HOOK(int, 0, fs_context_dup, struct fs_context *fc,
>  	 struct fs_context *src_sc)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d0eb20f90b26..31532b30c4f0 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -299,6 +299,7 @@ int security_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *
>  int security_bprm_check(struct linux_binprm *bprm);
>  void security_bprm_committing_creds(const struct linux_binprm *bprm);
>  void security_bprm_committed_creds(const struct linux_binprm *bprm);
> +void security_execve_abort(void);
>  int security_fs_context_submount(struct fs_context *fc, struct super_block *reference);
>  int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc);
>  int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param);
> @@ -648,6 +649,10 @@ static inline void security_bprm_committed_creds(const struct linux_binprm *bprm
>  {
>  }
>  
> +static inline void security_execve_abort(void)
> +{
> +}
> +
>  static inline int security_fs_context_submount(struct fs_context *fc,
>  					   struct super_block *reference)
>  {
> diff --git a/security/security.c b/security/security.c
> index 3aaad75c9ce8..10adc4d3c5e0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -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?

--
paul-moore.com

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

[Tetsuo Handa]

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 .

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

[Kees Cook]

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

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

[Paul Moore]

On Wed, Feb 7, 2024 at 9:34 AM Kees Cook <keescook@chromium.org> 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@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

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

[Paul Moore]

On Wed, Feb 7, 2024 at 11:01 AM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Feb 7, 2024 at 9:34 AM Kees Cook <keescook@chromium.org> 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@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.

Hi Tetsuo, I just wanted to check on this and see if you've been able
to make any progress?

-- 
paul-moore.com

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

[Tetsuo Handa]

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.
If everyone is fine with below one, I'll post v4 patchset.

 fs/exec.c                     |    1 +
 include/linux/lsm_hook_defs.h |    1 +
 include/linux/security.h      |    5 +++++
 security/security.c           |   12 ++++++++++++
 4 files changed, 19 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index af4fbb61cd53..cbee988445c6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1530,6 +1530,7 @@ static void free_bprm(struct linux_binprm *bprm)
 		kfree(bprm->interp);
 	kfree(bprm->fdpath);
 	kfree(bprm);
+	security_bprm_free();
 }
 
 static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int flags)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 76458b6d53da..0ef298231de2 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -54,6 +54,7 @@ LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct f
 LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
 LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, const struct linux_binprm *bprm)
 LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, const struct linux_binprm *bprm)
+LSM_HOOK(void, LSM_RET_VOID, bprm_free, void)
 LSM_HOOK(int, 0, fs_context_submount, struct fs_context *fc, struct super_block *reference)
 LSM_HOOK(int, 0, fs_context_dup, struct fs_context *fc,
 	 struct fs_context *src_sc)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..b12d20071a8f 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -299,6 +299,7 @@ int security_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *
 int security_bprm_check(struct linux_binprm *bprm);
 void security_bprm_committing_creds(const struct linux_binprm *bprm);
 void security_bprm_committed_creds(const struct linux_binprm *bprm);
+void security_bprm_free(void);
 int security_fs_context_submount(struct fs_context *fc, struct super_block *reference);
 int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc);
 int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param);
@@ -648,6 +649,10 @@ static inline void security_bprm_committed_creds(const struct linux_binprm *bprm
 {
 }
 
+static inline void security_bprm_free(void)
+{
+}
+
 static inline int security_fs_context_submount(struct fs_context *fc,
 					   struct super_block *reference)
 {
diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..ba2f480b2bdb 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1223,6 +1223,18 @@ void security_bprm_committed_creds(const struct linux_binprm *bprm)
 	call_void_hook(bprm_committed_creds, bprm);
 }
 
+/**
+ * security_bprm_free() - Notify of completion of an exec()
+ *
+ * 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.
+ */
+void security_bprm_free(void)
+{
+	call_void_hook(bprm_free);
+}
+
 /**
  * security_fs_context_submount() - Initialise fc->security
  * @fc: new filesystem context

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

[Paul Moore]

On Thu, Feb 15, 2024 at 9:33 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> 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.
> If everyone is fine with below one, I'll post v4 patchset.

My guess is that based on the previous comments people are going to
prefer option #2 above, but we'll see what everyone says.  I did have
one comment, below ...

>  fs/exec.c                     |    1 +
>  include/linux/lsm_hook_defs.h |    1 +
>  include/linux/security.h      |    5 +++++
>  security/security.c           |   12 ++++++++++++
>  4 files changed, 19 insertions(+)

...

> diff --git a/security/security.c b/security/security.c
> index 3aaad75c9ce8..ba2f480b2bdb 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1223,6 +1223,18 @@ void security_bprm_committed_creds(const struct linux_binprm *bprm)
>         call_void_hook(bprm_committed_creds, bprm);
>  }
>
> +/**
> + * security_bprm_free() - Notify of completion of an exec()

The short summary above doesn't match the summary below.  If we stick
with the security_bprm_free() approach please change "Notify of
completion of an exec()" to "Notify LSMs of a bprm free event" or
similar.

> + * 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.
> + */
> +void security_bprm_free(void)
> +{
> +       call_void_hook(bprm_free);
> +}
> +
>  /**
>   * security_fs_context_submount() - Initialise fc->security
>   * @fc: new filesystem context
>

-- 
paul-moore.com

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

[Kees Cook]

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);

-- 
Kees Cook

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

[Paul Moore]

On Wed, May 1, 2024 at 4:04 PM Kees Cook <keescook@chromium.org> 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

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

[Tetsuo Handa]

On 2024/06/11 5:44, Paul Moore wrote:
>> 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?
> 

You are going to merge static call patches first (though I call it a regression),
aren't you? For me, reviving dynamically appendable hooks (which is about to be
killed by static call patches) has the higher priority than adding
security_bprm_free() hook.

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

[Paul Moore]

On Tue, Jun 11, 2024 at 9:10 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2024/06/11 5:44, Paul Moore wrote:
> >> 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?
>
> You are going to merge static call patches first (though I call it a regression),
> aren't you?

That is the plan, although we need another revision as the latest
draft has a randstruct casting problem.

> For me, reviving dynamically appendable hooks (which is about to be
> killed by static call patches) has the higher priority than adding
> security_bprm_free() hook.

Unfortunately, dynamic hooks do not appear to be something we are
going to support, at least in the near term.  With that understanding,
do you expect to be able to work on the security_bprm_free() hook
patchset?

-- 
paul-moore.com