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

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

[Tetsuo Handa]

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 1/3] LSM: add security_bprm_aborting_creds() hook
[PATCH 2/3] tomoyo: replace current->in_execve flag with security_bprm_aborting_creds() hook
[PATCH 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           |   14 ++++++++++++++
 security/tomoyo/tomoyo.c      |   22 ++++++----------------
 6 files changed, 27 insertions(+), 22 deletions(-)

(Please apply directly to linux.git tree if this series is OK, for
 currently TOMOYO's tree is unavailable.)

[PATCH 1/3] LSM: add security_bprm_aborting_creds() 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_bprm_aborting_creds() 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>
---
 fs/exec.c                     |  1 +
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  5 +++++
 security/security.c           | 14 ++++++++++++++
 4 files changed, 21 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index af4fbb61cd53..9d198cd9a75c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1519,6 +1519,7 @@ static void free_bprm(struct linux_binprm *bprm)
 	}
 	free_arg_pages(bprm);
 	if (bprm->cred) {
+		security_bprm_aborting_creds(bprm);
 		mutex_unlock(&current->signal->cred_guard_mutex);
 		abort_creds(bprm->cred);
 	}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 185924c56378..ec44ff913cee 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_aborting_creds, const struct linux_binprm *bprm)
 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..cf78fb14ef3c 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_aborting_creds(const struct linux_binprm *bprm);
 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_aborting_creds(const struct linux_binprm *bprm)
+{
+}
+
 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 0144a98d3712..f426841e576a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1223,6 +1223,20 @@ void security_bprm_committed_creds(const struct linux_binprm *bprm)
 	call_void_hook(bprm_committed_creds, bprm);
 }
 
+/**
+ * security_bprm_aborting_creds() - Destroy creds for a process during exec()
+ * @bprm: binary program information
+ *
+ * Prepare to destroy the new security attributes which became unused due to
+ * failed execve operation.  @bprm points to the linux_binprm structure.
+ * This hook is a good place to undo changes which cannot be discarded by
+ * abort_creds().
+ */
+void security_bprm_aborting_creds(const struct linux_binprm *bprm)
+{
+	call_void_hook(bprm_aborting_creds, bprm);
+}
+
 /**
  * security_fs_context_submount() - Initialise fc->security
  * @fc: new filesystem context
-- 
2.18.4

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

[Eric W. Biederman]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

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

How is it not compatible?  Especially how is TOMOYO's design not
compatible with how things are today?

The discussion talks about not allowing reading of executables by programs
that can exec them.

At this point with __FMODE_EXEC being placed on the files for exec,
and with only execve using that mode all of your considerations should
be resolved.

So it appears to me that Tomoyo is currently compatible with COW
credentials even if it was not historically.

As such can we get a cleanup to actually make Tomoyo compatible.
Otherwise because Tomoyo is the only use of whatever you are doing
it will continue to be very easy to break Tomoyo.

The fact that somewhere Tomoyo is modifying a credential that the rest
of the kernel sees as read-only, and making it impossible to just
restore that credential is very concerning from a maintenance
perspective.

Can't Tomoyo simply allow reading of files that have __FMODE_EXEC
set when allow_execve is set, without needing to perform a domain
transition, and later back out that domain transition?


>  include/linux/security.h      |  5 +++++
>  security/security.c           | 14 ++++++++++++++
>  4 files changed, 21 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index af4fbb61cd53..9d198cd9a75c 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1519,6 +1519,7 @@ static void free_bprm(struct linux_binprm *bprm)
>  	}
>  	free_arg_pages(bprm);
>  	if (bprm->cred) {
> +		security_bprm_aborting_creds(bprm);
>  		mutex_unlock(&current->signal->cred_guard_mutex);
>  		abort_creds(bprm->cred);

Why isn't abort_creds calling security_free_cred enough here?
>  	}

Eric

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

[Tetsuo Handa]

On 2024/01/29 13:10, Eric W. Biederman wrote:
>> @@ -1519,6 +1519,7 @@ static void free_bprm(struct linux_binprm *bprm)
>>  	}
>>  	free_arg_pages(bprm);
>>  	if (bprm->cred) {
>> +		security_bprm_aborting_creds(bprm);
>>  		mutex_unlock(&current->signal->cred_guard_mutex);
>>  		abort_creds(bprm->cred);
> 
> Why isn't abort_creds calling security_free_cred enough here?

Because security_cred_free() from put_cred_rcu() is called from RCU callback
rather than from current thread doing execve().
TOMOYO wants to restore attributes of current thread doing execve().

> The fact that somewhere Tomoyo is modifying a credential that the rest
> of the kernel sees as read-only, and making it impossible to just
> restore that credential is very concerning from a maintenance
> perspective.

TOMOYO does not use "struct cred"->security.
TOMOYO uses only "struct task_struct"->security.

  struct lsm_blob_sizes tomoyo_blob_sizes __ro_after_init = {
      .lbs_task = sizeof(struct tomoyo_task),
  };

TOMOYO uses security_task_alloc() for allocating "struct task_struct"->security,
security_task_free() for releasing "struct task_struct"->security,
security_bprm_check() for updating "struct task_struct"->security,
security_bprm_committed_creds() for erasing old "struct task_struct"->security,
security_bprm_aborting_creds() for restoring old "struct task_struct"->security.

Commit a6f76f23d297 ("CRED: Make execve() take advantage of copy-on-write
credentials") made TOMOYO impossible to do above. current->in_execve flag was a
hack for emulating security_bprm_aborting_creds() using security_prepare_creds().

> Can't Tomoyo simply allow reading of files that have __FMODE_EXEC
> set when allow_execve is set, without needing to perform a domain
> transition, and later back out that domain transition?

No. That does not match TOMOYO's design.

allow_execve keyword does not imply "allow opening that file for non-execve() purpose".

Also, performing a domain transition before execve() reaches point of no return is
the TOMOYO's design, but COW credentials does not allow such behavior.

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

[Eric W. Biederman]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2024/01/29 13:10, Eric W. Biederman wrote:
>>> @@ -1519,6 +1519,7 @@ static void free_bprm(struct linux_binprm *bprm)
>>>  	}
>>>  	free_arg_pages(bprm);
>>>  	if (bprm->cred) {
>>> +		security_bprm_aborting_creds(bprm);
>>>  		mutex_unlock(&current->signal->cred_guard_mutex);
>>>  		abort_creds(bprm->cred);
>> 
>> Why isn't abort_creds calling security_free_cred enough here?
>
> Because security_cred_free() from put_cred_rcu() is called from RCU callback
> rather than from current thread doing execve().
> TOMOYO wants to restore attributes of current thread doing execve().

>
>> The fact that somewhere Tomoyo is modifying a credential that the rest
>> of the kernel sees as read-only, and making it impossible to just
>> restore that credential is very concerning from a maintenance
>> perspective.
>
> TOMOYO does not use "struct cred"->security.
> TOMOYO uses only "struct task_struct"->security.
>
>   struct lsm_blob_sizes tomoyo_blob_sizes __ro_after_init = {
>       .lbs_task = sizeof(struct tomoyo_task),
>   };
>
> TOMOYO uses security_task_alloc() for allocating "struct task_struct"->security,
> security_task_free() for releasing "struct task_struct"->security,
> security_bprm_check() for updating "struct task_struct"->security,
> security_bprm_committed_creds() for erasing old "struct task_struct"->security,
> security_bprm_aborting_creds() for restoring old "struct task_struct"->security.
>
> Commit a6f76f23d297 ("CRED: Make execve() take advantage of copy-on-write
> credentials") made TOMOYO impossible to do above. current->in_execve flag was a
> hack for emulating security_bprm_aborting_creds() using security_prepare_creds().
>
>> Can't Tomoyo simply allow reading of files that have __FMODE_EXEC
>> set when allow_execve is set, without needing to perform a domain
>> transition, and later back out that domain transition?
>
> No. That does not match TOMOYO's design.
>
> allow_execve keyword does not imply "allow opening that file for non-execve() purpose".

Huh?  I was proposing using the allow_execve credential to allow opening
and reading the file for execve purpose.  So you don't need to perform
a domain transition early.

> Also, performing a domain transition before execve() reaches point of no return is
> the TOMOYO's design, but COW credentials does not allow such behavior.

My question is simple.  Why can't TOMOYO use the changing of credentials
in task->cred to perform the domain transition?  Why can't TOMOYO work
with the code in execve?

I don't see anything that fundamentally requires you to have the domain
transition early.  All I have seen so far is an assertion that you
are using task->security.  Is there anything except for the reading
of the executable that having the domain transition early allows?

My primary concern with TOMOYO being the odd man out, and using hooks
for purposes arbitrary purposes instead of purposes they are logically
designed to be used for?

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.


What I understand from the documentation you provided about TOMOYO is:
- TOMOYO provides the domain transition early so that the executable
  can be read.
- TOMOYO did that because it could not detect reliably when a file
  was opened for execve and read for execve.

Am I wrong in my understanding?

If that understanding is correct, now that (file->f_mode & __FMODE_EXEC)
is a reliable indication of a file used exclusively for exec then it
should be possible to take advantage of the new information and get
TOMOYO and the rest of the execve playing nicely with each other without
having to add new hooks.


If the maintenance concerns are not enough please consider the situation
from an attack surface concern.  Any hacked together poorly maintained
surface is ripe bugs, and the exploitation of those bugs.  Sometimes
those bugs will break you in obvious ways, other times those bugs
will break you in overlooked and exploitable ways.

Eric

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

[Tetsuo Handa]

On 2024/01/30 3:15, Eric W. Biederman wrote:
> 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.

Fine for me. The current argument is redundant, for nobody will try to
call security_execve_abort() on a remote thread.

> 
> 
> What I understand from the documentation you provided about TOMOYO is:
> - TOMOYO provides the domain transition early so that the executable
>   can be read.
> - TOMOYO did that because it could not detect reliably when a file
>   was opened for execve and read for execve.
> 
> Am I wrong in my understanding?
> 
> If that understanding is correct, now that (file->f_mode & __FMODE_EXEC)
> is a reliable indication of a file used exclusively for exec then it
> should be possible to take advantage of the new information and get
> TOMOYO and the rest of the execve playing nicely with each other without
> having to add new hooks.

current->in_execve flag has two purposes: "whether to check permission" and
"what domain is used for checking permission (if need to check permission)".

One is to distinguish "open from execve()" and "open from uselib()".
This was replaced by the (file->f_mode & __FMODE_EXEC) change, for
__FMODE_EXEC was now removed from uselib(). But this is after all about
"whether to check permission".

The other is to emulate security_execve_abort(). security_execve_abort() is
needed because TOMOYO checks permission for opening interpreter file from
execve() using a domain which the current thread will belong to if execve()
succeeds (whereas DAC checks permission for opening interpreter file from
execve() using credentials which the current thread is currently using).
This is about "what domain is used for checking permission".

Since security_file_open() hook cannot see bprm->cred, TOMOYO cannot know
"what domain is used for checking permission" from security_file_open().
TOMOYO can know only "whether to check permission" from security_file_open().

Since TOMOYO cannot pass bprm->cred to security_file_open() hook using
override_creds()/revert_creds(), TOMOYO is passing "what domain is used for
checking permission" to security_file_open() via "struct task_struct"->security.
"struct task_struct"->security is updated _before_ security_file_open() for the
interpreter file is called.

Since security_execve_abort() was missing, when execve() failed, TOMOYO had
to keep the domain which the current thread would belong to if execve() succeeded.
The kept domain is cleared when TOMOYO finds that previous execve() was finished
(indicated by current->in_execve == 0) or when TOMOYO finds that new execve() is
in progress (indicated by current->in_execve == 0 when security_cred_prepare() is
called).

It is not possible to extract "what domain is used for checking permission" from
"whether file->f_mode includes __FMODE_EXEC". Talking about the
(file->f_mode & __FMODE_EXEC) change (i.e. "whether to check permission") is
pointless when talking about "what domain is used for checking permission".

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

[Tetsuo Handa]

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

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/tomoyo/tomoyo.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 04a92c3d65d4..de572705772a 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -18,34 +18,24 @@ 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.
+ * tomoyo_bprm_aborting_creds - Target for security_bprm_aborting_creds().
  *
- * Returns 0.
+ * @bprm: Pointer to "struct linux_binprm".
  */
-static int tomoyo_cred_prepare(struct cred *new, const struct cred *old,
-			       gfp_t gfp)
+static void tomoyo_bprm_aborting_creds(const struct linux_binprm *bprm)
 {
-	/* 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 +544,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(bprm_aborting_creds, tomoyo_bprm_aborting_creds),
 	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 3/3] fs/exec: remove current->in_execve flag

[Tetsuo Handa]

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

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 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 9d198cd9a75c..f93cfc957e25 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