linux-kernel - Re: [PATCH v11 2/4] fs: add LSM-supporting anon-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] [day] [month] [year] [list]

Date:   Thu, 5 Nov 2020 14:24:19 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Lokesh Gidra <lokeshgidra@...gle.com>
Cc:     Andrea Arcangeli <aarcange@...hat.com>,
        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,
        linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        hch@...radead.org, Daniel Colascione <dancol@...gle.com>
Subject: Re: [PATCH v11 2/4] fs: add LSM-supporting anon-inode interface

On Thu, Nov 05, 2020 at 01:33:22PM -0800, Lokesh Gidra wrote:
> +/**
> + * Like anon_inode_getfd(), but creates 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 relationship with the new inode.  The LSM may use
> + * @context_inode in init_security_anon(), but a reference to it is not held.
> + */
> +int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
> +			    void *priv, int flags,
> +			    const struct inode *context_inode)
> +{
> +	return __anon_inode_getfd(name, fops, priv, flags, context_inode, true);
> +}
> +EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);

inode_init_security_anon(), not init_security_anon().  Also please use a
consistent line width (preferably 80 characters).

> diff --git a/fs/libfs.c b/fs/libfs.c
> index fc34361c1489..5b12228ecc81 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1213,9 +1213,9 @@ static int anon_set_page_dirty(struct page *page)
>  };
>  
>  /*
> - * A single inode exists for all anon_inode files. Contrary to pipes,
> - * anon_inode inodes have no associated per-instance data, so we need
> - * only allocate one of them.
> + * A single inode exists for all anon_inode files, except for the secure ones.
> + * Contrary to pipes and secure anon_inode inodes, ordinary anon_inode inodes
> + * have no associated per-instance data, so we need only allocate one of them.
>   */
>  struct inode *alloc_anon_inode(struct super_block *s)
>  {

This comment is still wrong, and the first sentence contradicts the second one.
There are a lot of callers of alloc_anon_inode() and none of them use the
singleton inode, since alloc_anon_inode() doesn't actually use it.  The
singleton inode is only used by anon_inode_getfile() and anon_inode_getfd(),
which already have comments describing how they use a singleton inode.

IMO, just deleting this comment would be much better than either the original
version or your proposed version.

> diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
> index d0d7d96261ad..6cf447cfceed 100644
> --- a/include/linux/anon_inodes.h
> +++ b/include/linux/anon_inodes.h
> @@ -10,10 +10,15 @@
>  #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);
>  

Keeping declarations in the same order as the definitions can be helpful.

- Eric