linux-kernel - Re: [PATCH v8 07/19] ima: Move dentry 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]

Date:   Tue, 18 Jan 2022 15:42:14 -0500
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Stefan Berger <stefanb@...ux.ibm.com>,
        Stefan Berger <stefanb@...ux.vnet.ibm.com>,
        linux-integrity@...r.kernel.org
Cc:     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
Subject: Re: [PATCH v8 07/19] ima: Move dentry into ima_namespace and others
 onto stack

On Tue, 2022-01-18 at 15:12 -0500, Stefan Berger wrote:
> On 1/13/22 15:28, Mimi Zohar wrote:
> > Hi Stefan,
> >
> > Nobody refers to the IMA securityfs files as dentries.  The Subject
> > line is suppose to provide a hint about the patch.  How about changing
> > the "Subject" line to "ima: Move IMA securityfs files into
> > ima_namespaces or onto stack".
> >
> > On Tue, 2022-01-04 at 12:04 -0500, Stefan Berger wrote:
> >> From: Stefan Berger <stefanb@...ux.ibm.com>
> >>
> >> Move the policy file dentry into the ima_namespace for reuse by
> >> virtualized SecurityFS and for being able to remove it from
> >> the filesystem. Move the other dentries onto the stack.
> > Missing is an explanation why the other IMA securityfs files can be on
> > the stack.  Maybe start out by saying that the ns_ima_init securityfs
> > files are never deleted.  Then transition into the IMA namespaced
> > securityfs files and how they will be deleted.
> 
> How about this:
> 
> ima: Move IMA securityfs files into ima_namespace or onto stack
> 
> Move the IMA policy file's dentry into the ima_namespace for reuse by
> virtualized securityfs and for being able to remove the file from the
> filesystem using securityfs_remove().

How about "Move the IMA securityfs policy file ..."

> Move the other files' dentries onto the stack since they are not needed

How about "Move the other IMA securityfs files ..."

> outside the function where they are created in. Also, their cleanup is
> automatically handled by the filesystem upon umount of a virtualized
> secruityfs instance, so they don't need to be explicitly freed anymore.
> 
> When moving the dentry 'ima_policy' into ima_namespace rename it to
> 'policy_dentry' to clarify its datatype and avoid a name clash with
> 'int ima_policy' from ima_policy.c.

To prevent namespace pollution, static variables need to be prefixed
(e.g. "ima_").  This is not a concern with variables inside the
ima_namespace structure.  Why not just rename the variable "policy".

Refer to the section on "Naming" in Documentation/process/coding-
style.rst.

thanks,

Mimi

> 
> 
> 
> >> Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
> >> ---
> >>   security/integrity/ima/ima.h    |  2 ++
> >>   security/integrity/ima/ima_fs.c | 32 ++++++++++++++++++--------------
> >>   2 files changed, 20 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >> index 82b3f6a98320..224b09617c52 100644
> >> --- a/security/integrity/ima/ima.h
> >> +++ b/security/integrity/ima/ima.h
> >> @@ -140,6 +140,8 @@ struct ima_namespace {
> >>   	struct mutex ima_write_mutex;
> >>   	unsigned long ima_fs_flags;
> >>   	int valid_policy;
> >> +
> >> +	struct dentry *policy_dentry;
> > None of the other securityfs files are renamed.  Why is "ima_policy"
> > being renamed to "policy_dentry"?  If there is a need, it should be
> > documented in the patch description.
> >
> > thanks,
> >
> > Mimi
> >
> >>   } __randomize_layout;
> >>   extern struct ima_namespace init_ima_ns;
> >>