linux-kernel - Re: [PATCH v10 1/3] Add a new LSM-supporting anonymous inode interface

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: <20201104202721.GB1796392@gmail.com>
Date:   Wed, 4 Nov 2020 12:27:21 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Lokesh Gidra <lokeshgidra@...gle.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        James Morris <jmorris@...ei.org>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...isplace.org>,
        Daniel Colascione <dancol@...col.org>,
        Kees Cook <keescook@...omium.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        KP Singh <kpsingh@...gle.com>,
        David Howells <dhowells@...hat.com>,
        Thomas Cedeno <thomascedeno@...gle.com>,
        Anders Roxell <anders.roxell@...aro.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Matthew Garrett <matthewgarrett@...gle.com>,
        Aaron Goidel <acgoide@...ho.nsa.gov>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Joel Fernandes (Google)" <joel@...lfernandes.org>,
        YueHaibing <yuehaibing@...wei.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Alexey Budankov <alexey.budankov@...ux.intel.com>,
        Adrian Reber <areber@...hat.com>,
        Aleksa Sarai <cyphar@...har.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        kaleshsingh@...gle.com, calin@...gle.com, surenb@...gle.com,
        nnk@...gle.com, jeffv@...gle.com, kernel-team@...roid.com,
        Daniel Colascione <dancol@...gle.com>
Subject: Re: [PATCH v10 1/3] Add a new LSM-supporting anonymous inode
 interface

At a high level this patch looks fine to me, but a few nits below.  Also as I
mentioned on the cover letter, it seems this should be split into two patches --
one for the fs changes and one for the security changes.

On Sun, Oct 11, 2020 at 01:29:34AM -0700, Lokesh Gidra wrote:
> +static struct inode *anon_inode_make_secure_inode(
> +	const char *name,
> +	const struct inode *context_inode)
>  {
> -	struct file *file;
> +	struct inode *inode;
> +	const struct qstr qname = QSTR_INIT(name, strlen(name));
> +	int error;
> +
> +	inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> +	if (IS_ERR(inode))
> +		return inode;
> +	inode->i_flags &= ~S_PRIVATE;

The comment for alloc_anon_inode() still claims that it uses a single inode.
It would be helpful to fix that comment.

> +/**
> + * Like anon_inode_getfd() creates a new file, but by hooking it to a new anon
> + * inode, rather than to the same singleton inode. Also adds the @context_inode
> + * argument to allow security modules to control creation of the new file. Once
> + * the security module makes the decision, the context_inode is no longer needed
> + * and hence reference to it is not held.
> + */

The first sentence seems a bit off, gramatically.  Also, it seems there should
be a hint here as to why anyone would care whether the inode is singleton or
not.  Remember, people will be reading this code years down the line, and people
may not understand the exact problem you are trying to solve.

Would the following be accurate, or am I misunderstanding something?

/**
 * Like anon_inode_getfd(), but create a new !S_PRIVATE anon inode rather than
 * reuse the singleton anon inode, and call the init_security_anon() LSM hook.
 * This allows the inode to have its own security context and for a LSM to
 * reject creation of the inode.  An optional @context_inode argument is also
 * added to provide the logical "parent" of the new inode.  The LSM may use
 * @context_inode in init_security_anon(), but a reference to it is not held.
 */

> diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
> index d0d7d96261ad..6ab840ee93bc 100644
> --- a/include/linux/anon_inodes.h
> +++ b/include/linux/anon_inodes.h
> @@ -10,12 +10,20 @@
>  #define _LINUX_ANON_INODES_H
>  
>  struct file_operations;
> +struct inode;
>  
>  struct file *anon_inode_getfile(const char *name,
>  				const struct file_operations *fops,
>  				void *priv, int flags);
> +
> +int anon_inode_getfd_secure(const char *name,
> +			    const struct file_operations *fops,
> +			    void *priv, int flags,
> +			    const struct inode *context_inode);
> +
>  int anon_inode_getfd(const char *name, const struct file_operations *fops,
>  		     void *priv, int flags);
>  
> +

Unwanted whitespace change here.

> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 9e2e3e63719d..586186f1184b 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -233,6 +233,15 @@
>   *	Returns 0 if @name and @value have been successfully set,
>   *	-EOPNOTSUPP if no security attribute is needed, or
>   *	-ENOMEM on memory allocation failure.
> + * @inode_init_security_anon:
> + *      Set up the incore security field for the new anonymous inode
> + *      and return whether the inode creation is permitted by the security
> + *      module or not.
> + *      @inode contains the inode structure
> + *      @name name of the anonymous inode class
> + *      @context_inode optional related inode
> + *	Returns 0 on success, -EACCESS if the security module denies the
> + *	creation of this inode, or another -errno upon other errors.

EACCES, not EACCESS.  The spelling mistakes of decades past are still with us...

- Eric