linux-kernel - Re: [PATCH v2 1/3] LSM: add security_execve

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202402070622.D2DCD9C4@keescook>
Date: Wed, 7 Feb 2024 06:24:57 -0800
From: Kees Cook <keescook@...omium.org>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
	Eric Biederman <ebiederm@...ssion.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	linux-security-module <linux-security-module@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] LSM: add security_execve_abort() hook

On Sat, Feb 03, 2024 at 07:52:54PM +0900, Tetsuo Handa 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.
> 
> 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@...ove.SAKURA.ne.jp>

This looks good to me.

Given this touches execve and is related to the recent execve changes,
shall I carry this in the execve tree for testing and send a PR to Linus
for it before v6.8 releases?

There's already an Ack from Serge, so this seems a reasonable way to go
unless Paul would like it done some other way?

Reviewed-by: Kees Cook <keescook@...omium.org>

-- 
Kees Cook