linux-kernel - Re: [PATCH v9 10/23] ima: Move IMA securityfs files into ima

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: <20220126094042.l6jqu5swwsyhlt7v@wittgenstein>
Date:   Wed, 26 Jan 2022 10:40:42 +0100
From:   Christian Brauner <brauner@...nel.org>
To:     Stefan Berger <stefanb@...ux.vnet.ibm.com>
Cc:     linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
        serge@...lyn.com, christian.brauner@...ntu.com,
        containers@...ts.linux.dev, dmitry.kasatkin@...il.com,
        ebiederm@...ssion.com, krzysztof.struczynski@...wei.com,
        roberto.sassu@...wei.com, mpeters@...hat.com, lhinds@...hat.com,
        lsturman@...hat.com, puiterwi@...hat.com, jejb@...ux.ibm.com,
        jamjoom@...ibm.com, linux-kernel@...r.kernel.org,
        paul@...l-moore.com, rgb@...hat.com,
        linux-security-module@...r.kernel.org, jmorris@...ei.org,
        Stefan Berger <stefanb@...ux.ibm.com>
Subject: Re: [PATCH v9 10/23] ima: Move IMA securityfs files into
 ima_namespace or onto stack

On Tue, Jan 25, 2022 at 05:46:32PM -0500, Stefan Berger wrote:
> From: Stefan Berger <stefanb@...ux.ibm.com>
> 
> Only the securityfs IMA policy file is ever removed based on Kconfig
> options. For this reason, move the IMA securityfs policy file variable
> 'ima_policy' into the ima_namespace.
> 
> Move the other IMA securityfs files onto the stack since they are not
> needed outside the function where they are created in. Also, their cleanup
> is automatically handled by the filesystem upon umount of a virtualized
> securityfs instance, so they don't need to be explicitly freed.

I'd reverse the explantion in the commit and mention the securityfs
change that makes this move possible which is patch 3 in this version of
the series ("securityfs: rework dentry creation"); something like:

	Earlier we simplified how dentry creation and deletion is manged in
	securityfs. This allows us to move IMA securityfs files from global
	variables directly into ima_fs_ns_init() itself. We can now rely on
	those dentries to be cleaned up when the securityfs instance is cleaned
	when the last reference to it is dropped.
	
	Things are slightly different for the initial ima namespace. In contrast
	to non-initial ima namespaces it has pinning logic binding the lifetime
	of the securityfs superblock to created dentries. We need to keep this
	behavior to not regress userspace. Since ima never removes most of the
	securityfs files the initial securityfs instance stays pinned. This also
	means even for the initial ima namespace we don't need to keep
	references to these dentries anywhere.
	
	The ima_policy file is the exception since ima can end up removing it if
	a non-default policy is written at some point.

Last sentence should be checked for sensibility by ima folks.

> 
> In the failure cleanup path clean up the ima_policy dentry before
> cleaning up the directories.
> 
> Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
> 
> ---

Moving into imans looks good,
Acked-by: Christian Brauner <brauner@...nel.org>