Re: [PATCH v5 1/3] Add a new LSM-supporting anonymous inode interface
On Wed, Apr 01, 2020 at 02:39:01PM -0700, Daniel Colascione wrote: > This change adds two new functions, anon_inode_getfile_secure and > anon_inode_getfd_secure, that create anonymous-node files with > individual non-S_PRIVATE inodes to which security modules can apply > policy. Existing callers continue using the original singleton-inode > kind of anonymous-inode file. We can transition anonymous inode users > to the new kind of anonymous inode in individual patches for the sake > of bisection and review. > > The new functions accept an optional context_inode parameter that > callers can use to provide additional contextual information to > security modules, e.g., indicating that one anonymous struct file is a > logical child of another, allowing a security model to propagate > security information from one to the other. > > Signed-off-by: Daniel Colascione > --- > fs/anon_inodes.c| 191 > include/linux/anon_inodes.h | 13 +++ > include/linux/lsm_hooks.h | 11 +++ > include/linux/security.h| 3 + > security/security.c | 9 ++ > 5 files changed, 186 insertions(+), 41 deletions(-) > > diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c > index 89714308c25b..f87f221167cf 100644 > --- a/fs/anon_inodes.c > +++ b/fs/anon_inodes.c > @@ -55,61 +55,108 @@ static struct file_system_type anon_inode_fs_type = { > .kill_sb= kill_anon_super, > }; > > -/** > - * anon_inode_getfile - creates a new file instance by hooking it up to an > - * anonymous inode, and a dentry that describe the > "class" > - * of the file > - * > - * @name:[in]name of the "class" of the new file > - * @fops:[in]file operations for the new file > - * @priv:[in]private data for the new file (will be file's > private_data) > - * @flags: [in]flags > - * > - * Creates a new file by hooking it on a single inode. This is useful for > files > - * that do not need to have a full-fledged inode in order to operate > correctly. > - * All the files created with anon_inode_getfile() will share a single inode, > - * hence saving memory and avoiding code duplication for the > file/inode/dentry > - * setup. Returns the newly created file* or an error pointer. > - */ > -struct file *anon_inode_getfile(const char *name, > - const struct file_operations *fops, > - void *priv, int flags) > +static struct inode *anon_inode_make_secure_inode( > + const char *name, > + const struct inode *context_inode) > +{ > + 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; > + error = security_inode_init_security_anon( > + inode, , context_inode); > + if (error) { > + iput(inode); > + return ERR_PTR(error); > + } > + return inode; > +} > + > +struct file *_anon_inode_getfile(const char *name, > + const struct file_operations *fops, > + void *priv, int flags, > + const struct inode *context_inode, > + bool secure) Unnecessarily global function. > { > + struct inode *inode; > struct file *file; > > - if (IS_ERR(anon_inode_inode)) > - return ERR_PTR(-ENODEV); > + if (secure) { > + inode = anon_inode_make_secure_inode( > + name, context_inode); > + if (IS_ERR(inode)) > + return ERR_PTR(PTR_ERR(inode)); Use ERR_CAST(), not ERR_PTR(PTR_ERR()). > /** > - * anon_inode_getfd - creates a new file instance by hooking it up to an > - *anonymous inode, and a dentry that describe the "class" > - *of the file > + * anon_inode_getfile_secure - creates a new file instance by hooking > + * it up to a new anonymous inode and a > + * dentry that describe the "class" of the > + * file. Make it possible to use security > + * modules to control access to the > + * new file. > + * > + * @name:[in]name of the "class" of the new file > + * @fops:[in]file operations for the new file > + * @priv:[in]private data for the new file (will be file's > private_data) > + * @flags: [in]flags > + * > + * Creates a new file by hooking it on an unspecified inode. This is > + * useful for files that do not need to have a full-fledged filesystem > + * to operate correctly. All the files created with > + * anon_inode_getfile_secure() will have distinct inodes, avoiding > + * code duplication for the
Re: [PATCH v5 1/3] Add a new LSM-supporting anonymous inode interface
On Wed, 1 Apr 2020, Daniel Colascione wrote: > This change adds two new functions, anon_inode_getfile_secure and > anon_inode_getfd_secure, that create anonymous-node files with > individual non-S_PRIVATE inodes to which security modules can apply > policy. Existing callers continue using the original singleton-inode > kind of anonymous-inode file. We can transition anonymous inode users > to the new kind of anonymous inode in individual patches for the sake > of bisection and review. > > The new functions accept an optional context_inode parameter that > callers can use to provide additional contextual information to > security modules, e.g., indicating that one anonymous struct file is a > logical child of another, allowing a security model to propagate > security information from one to the other. > > Signed-off-by: Daniel Colascione Al, Andrew, wondering if you could look at these anon inode changes before we merge this? > --- > fs/anon_inodes.c| 191 > include/linux/anon_inodes.h | 13 +++ > include/linux/lsm_hooks.h | 11 +++ > include/linux/security.h| 3 + > security/security.c | 9 ++ > 5 files changed, 186 insertions(+), 41 deletions(-) > > diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c > index 89714308c25b..f87f221167cf 100644 > --- a/fs/anon_inodes.c > +++ b/fs/anon_inodes.c > @@ -55,61 +55,108 @@ static struct file_system_type anon_inode_fs_type = { > .kill_sb= kill_anon_super, > }; > > -/** > - * anon_inode_getfile - creates a new file instance by hooking it up to an > - * anonymous inode, and a dentry that describe the > "class" > - * of the file > - * > - * @name:[in]name of the "class" of the new file > - * @fops:[in]file operations for the new file > - * @priv:[in]private data for the new file (will be file's > private_data) > - * @flags: [in]flags > - * > - * Creates a new file by hooking it on a single inode. This is useful for > files > - * that do not need to have a full-fledged inode in order to operate > correctly. > - * All the files created with anon_inode_getfile() will share a single inode, > - * hence saving memory and avoiding code duplication for the > file/inode/dentry > - * setup. Returns the newly created file* or an error pointer. > - */ > -struct file *anon_inode_getfile(const char *name, > - const struct file_operations *fops, > - void *priv, int flags) > +static struct inode *anon_inode_make_secure_inode( > + const char *name, > + const struct inode *context_inode) > +{ > + 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; > + error = security_inode_init_security_anon( > + inode, , context_inode); > + if (error) { > + iput(inode); > + return ERR_PTR(error); > + } > + return inode; > +} > + > +struct file *_anon_inode_getfile(const char *name, > + const struct file_operations *fops, > + void *priv, int flags, > + const struct inode *context_inode, > + bool secure) > { > + struct inode *inode; > struct file *file; > > - if (IS_ERR(anon_inode_inode)) > - return ERR_PTR(-ENODEV); > + if (secure) { > + inode = anon_inode_make_secure_inode( > + name, context_inode); > + if (IS_ERR(inode)) > + return ERR_PTR(PTR_ERR(inode)); > + } else { > + inode = anon_inode_inode; > + if (IS_ERR(inode)) > + return ERR_PTR(-ENODEV); > + /* > + * We know the anon_inode inode count is always > + * greater than zero, so ihold() is safe. > + */ > + ihold(inode); > + } > > - if (fops->owner && !try_module_get(fops->owner)) > - return ERR_PTR(-ENOENT); > + if (fops->owner && !try_module_get(fops->owner)) { > + file = ERR_PTR(-ENOENT); > + goto err; > + } > > - /* > - * We know the anon_inode inode count is always greater than zero, > - * so ihold() is safe. > - */ > - ihold(anon_inode_inode); > - file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name, > + file = alloc_file_pseudo(inode, anon_inode_mnt, name, >flags & (O_ACCMODE | O_NONBLOCK), fops); > if (IS_ERR(file)) > goto err; > > - file->f_mapping = anon_inode_inode->i_mapping; > + file->f_mapping = inode->i_mapping; > > file->private_data = priv; > >