| Message ID | 162871492283.63873.8743976556992924333.stgit@olly (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Add LSM access controls and auditing to io_uring | expand |
On 11/08/2021 22:48, Paul Moore wrote: > Extending the secure anonymous inode support to other subsystems > requires that we have a secure anon_inode_getfile() variant in > addition to the existing secure anon_inode_getfd() variant. > > Thankfully we can reuse the existing __anon_inode_getfile() function > and just wrap it with the proper arguments. > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > v2: > - no change > v1: > - initial draft > --- > fs/anon_inodes.c | 29 +++++++++++++++++++++++++++++ > include/linux/anon_inodes.h | 4 ++++ > 2 files changed, 33 insertions(+) > > diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c > index a280156138ed..e0c3e33c4177 100644 > --- a/fs/anon_inodes.c > +++ b/fs/anon_inodes.c > @@ -148,6 +148,35 @@ struct file *anon_inode_getfile(const char *name, > } > EXPORT_SYMBOL_GPL(anon_inode_getfile); > > +/** > + * anon_inode_getfile_secure - Like anon_inode_getfile(), but creates a new > + * !S_PRIVATE anon inode rather than reuse the > + * singleton anon inode and calls the > + * inode_init_security_anon() LSM hook. This > + * allows for both the inode to have its own > + * security context and for the LSM to enforce > + * policy on the inode's creation. > + * > + * @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 > + * @context_inode: > + * [in] the logical relationship with the new inode (optional) > + * > + * The LSM may use @context_inode in inode_init_security_anon(), but a > + * reference to it is not held. Returns the newly created file* or an error > + * pointer. See the anon_inode_getfile() documentation for more information. > + */ > +struct file *anon_inode_getfile_secure(const char *name, > + const struct file_operations *fops, > + void *priv, int flags, > + const struct inode *context_inode) > +{ > + return __anon_inode_getfile(name, fops, priv, flags, > + context_inode, true); This is not directly related to this patch but why using the "secure" boolean in __anon_inode_getfile() and __anon_inode_getfd() instead of checking that context_inode is not NULL? This would simplify the code, remove this anon_inode_getfile_secure() wrapper and avoid potential inconsistencies. > +} > + > static int __anon_inode_getfd(const char *name, > const struct file_operations *fops, > void *priv, int flags, > diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h > index 71881a2b6f78..5deaddbd7927 100644 > --- a/include/linux/anon_inodes.h > +++ b/include/linux/anon_inodes.h > @@ -15,6 +15,10 @@ struct inode; > struct file *anon_inode_getfile(const char *name, > const struct file_operations *fops, > void *priv, int flags); > +struct file *anon_inode_getfile_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); > int anon_inode_getfd_secure(const char *name, >
On Thu, Aug 12, 2021 at 5:32 AM Mickaël Salaün <mic@digikod.net> wrote: > On 11/08/2021 22:48, Paul Moore wrote: > > Extending the secure anonymous inode support to other subsystems > > requires that we have a secure anon_inode_getfile() variant in > > addition to the existing secure anon_inode_getfd() variant. > > > > Thankfully we can reuse the existing __anon_inode_getfile() function > > and just wrap it with the proper arguments. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > --- > > v2: > > - no change > > v1: > > - initial draft > > --- > > fs/anon_inodes.c | 29 +++++++++++++++++++++++++++++ > > include/linux/anon_inodes.h | 4 ++++ > > 2 files changed, 33 insertions(+) > > > > diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c > > index a280156138ed..e0c3e33c4177 100644 > > --- a/fs/anon_inodes.c > > +++ b/fs/anon_inodes.c > > @@ -148,6 +148,35 @@ struct file *anon_inode_getfile(const char *name, > > } > > EXPORT_SYMBOL_GPL(anon_inode_getfile); > > > > +/** > > + * anon_inode_getfile_secure - Like anon_inode_getfile(), but creates a new > > + * !S_PRIVATE anon inode rather than reuse the > > + * singleton anon inode and calls the > > + * inode_init_security_anon() LSM hook. This > > + * allows for both the inode to have its own > > + * security context and for the LSM to enforce > > + * policy on the inode's creation. > > + * > > + * @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 > > + * @context_inode: > > + * [in] the logical relationship with the new inode (optional) > > + * > > + * The LSM may use @context_inode in inode_init_security_anon(), but a > > + * reference to it is not held. Returns the newly created file* or an error > > + * pointer. See the anon_inode_getfile() documentation for more information. > > + */ > > +struct file *anon_inode_getfile_secure(const char *name, > > + const struct file_operations *fops, > > + void *priv, int flags, > > + const struct inode *context_inode) > > +{ > > + return __anon_inode_getfile(name, fops, priv, flags, > > + context_inode, true); > > This is not directly related to this patch but why using the "secure" > boolean in __anon_inode_getfile() and __anon_inode_getfd() instead of > checking that context_inode is not NULL? This would simplify the code, > remove this anon_inode_getfile_secure() wrapper and avoid potential > inconsistencies. The issue is that it is acceptable for the context_inode to be either valid or NULL for callers who request the "secure" code path. Look at the SELinux implementation of the anonymous inode hook in selinux_inode_init_security_anon() and you will see that in cases where the context_inode is valid we simply inherit the label from the given inode, whereas if context_inode is NULL we do a type transition using the requesting task and the anonymous inode's "name".
On 12/08/2021 16:32, Paul Moore wrote: > On Thu, Aug 12, 2021 at 5:32 AM Mickaël Salaün <mic@digikod.net> wrote: >> On 11/08/2021 22:48, Paul Moore wrote: >>> Extending the secure anonymous inode support to other subsystems >>> requires that we have a secure anon_inode_getfile() variant in >>> addition to the existing secure anon_inode_getfd() variant. >>> >>> Thankfully we can reuse the existing __anon_inode_getfile() function >>> and just wrap it with the proper arguments. >>> >>> Signed-off-by: Paul Moore <paul@paul-moore.com> >>> >>> --- >>> v2: >>> - no change >>> v1: >>> - initial draft >>> --- >>> fs/anon_inodes.c | 29 +++++++++++++++++++++++++++++ >>> include/linux/anon_inodes.h | 4 ++++ >>> 2 files changed, 33 insertions(+) >>> >>> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c >>> index a280156138ed..e0c3e33c4177 100644 >>> --- a/fs/anon_inodes.c >>> +++ b/fs/anon_inodes.c >>> @@ -148,6 +148,35 @@ struct file *anon_inode_getfile(const char *name, >>> } >>> EXPORT_SYMBOL_GPL(anon_inode_getfile); >>> >>> +/** >>> + * anon_inode_getfile_secure - Like anon_inode_getfile(), but creates a new >>> + * !S_PRIVATE anon inode rather than reuse the >>> + * singleton anon inode and calls the >>> + * inode_init_security_anon() LSM hook. This >>> + * allows for both the inode to have its own >>> + * security context and for the LSM to enforce >>> + * policy on the inode's creation. >>> + * >>> + * @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 >>> + * @context_inode: >>> + * [in] the logical relationship with the new inode (optional) >>> + * >>> + * The LSM may use @context_inode in inode_init_security_anon(), but a >>> + * reference to it is not held. Returns the newly created file* or an error >>> + * pointer. See the anon_inode_getfile() documentation for more information. >>> + */ >>> +struct file *anon_inode_getfile_secure(const char *name, >>> + const struct file_operations *fops, >>> + void *priv, int flags, >>> + const struct inode *context_inode) >>> +{ >>> + return __anon_inode_getfile(name, fops, priv, flags, >>> + context_inode, true); >> >> This is not directly related to this patch but why using the "secure" >> boolean in __anon_inode_getfile() and __anon_inode_getfd() instead of >> checking that context_inode is not NULL? This would simplify the code, >> remove this anon_inode_getfile_secure() wrapper and avoid potential >> inconsistencies. > > The issue is that it is acceptable for the context_inode to be either > valid or NULL for callers who request the "secure" code path. > > Look at the SELinux implementation of the anonymous inode hook in > selinux_inode_init_security_anon() and you will see that in cases > where the context_inode is valid we simply inherit the label from the > given inode, whereas if context_inode is NULL we do a type transition > using the requesting task and the anonymous inode's "name". > Indeed. Acked-by: Mickaël Salaün <mic@linux.microsoft.com>
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c index a280156138ed..e0c3e33c4177 100644 --- a/fs/anon_inodes.c +++ b/fs/anon_inodes.c @@ -148,6 +148,35 @@ struct file *anon_inode_getfile(const char *name, } EXPORT_SYMBOL_GPL(anon_inode_getfile); +/** + * anon_inode_getfile_secure - Like anon_inode_getfile(), but creates a new + * !S_PRIVATE anon inode rather than reuse the + * singleton anon inode and calls the + * inode_init_security_anon() LSM hook. This + * allows for both the inode to have its own + * security context and for the LSM to enforce + * policy on the inode's creation. + * + * @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 + * @context_inode: + * [in] the logical relationship with the new inode (optional) + * + * The LSM may use @context_inode in inode_init_security_anon(), but a + * reference to it is not held. Returns the newly created file* or an error + * pointer. See the anon_inode_getfile() documentation for more information. + */ +struct file *anon_inode_getfile_secure(const char *name, + const struct file_operations *fops, + void *priv, int flags, + const struct inode *context_inode) +{ + return __anon_inode_getfile(name, fops, priv, flags, + context_inode, true); +} + static int __anon_inode_getfd(const char *name, const struct file_operations *fops, void *priv, int flags, diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h index 71881a2b6f78..5deaddbd7927 100644 --- a/include/linux/anon_inodes.h +++ b/include/linux/anon_inodes.h @@ -15,6 +15,10 @@ struct inode; struct file *anon_inode_getfile(const char *name, const struct file_operations *fops, void *priv, int flags); +struct file *anon_inode_getfile_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); int anon_inode_getfd_secure(const char *name,
Extending the secure anonymous inode support to other subsystems requires that we have a secure anon_inode_getfile() variant in addition to the existing secure anon_inode_getfd() variant. Thankfully we can reuse the existing __anon_inode_getfile() function and just wrap it with the proper arguments. Signed-off-by: Paul Moore <paul@paul-moore.com> --- v2: - no change v1: - initial draft --- fs/anon_inodes.c | 29 +++++++++++++++++++++++++++++ include/linux/anon_inodes.h | 4 ++++ 2 files changed, 33 insertions(+)