Message ID | 166807856758.2972602.14175912201162072721.stgit@warthog.procyon.org.uk (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | [v5] vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing | expand |
On Thu, Nov 10, 2022 at 6:09 AM David Howells <dhowells@redhat.com> wrote: > > When NFS superblocks are created by automounting, their LSM parameters > aren't set in the fs_context struct prior to sget_fc() being called, > leading to failure to match existing superblocks. > > Fix this by adding a new LSM hook to load fc->security for submount > creation when alloc_fs_context() is creating the fs_context for it. > > However, this uncovers a further bug: nfs_get_root() initialises the > superblock security manually by calling security_sb_set_mnt_opts() or > security_sb_clone_mnt_opts() - but then vfs_get_tree() calls > security_sb_set_mnt_opts(), which can lead to SELinux, at least, > complaining. > > Fix that by adding a flag to the fs_context that suppresses the > security_sb_set_mnt_opts() call in vfs_get_tree(). This can be set by NFS > when it sets the LSM context on the new superblock. > > The first bug leads to messages like the following appearing in dmesg: > > NFS: Cache volume key already in use (nfs,4.2,2,108,106a8c0,1,,,,100000,100000,2ee,3a98,1d4c,3a98,1) > > Changes > ======= > ver #5) > - Removed unused variable. > - Only allocate smack_mnt_opts if we're dealing with a submount. > > ver #4) > - When doing a FOR_SUBMOUNT mount, don't set the root label in SELinux or > Smack. > > ver #3) > - Made LSM parameter extraction dependent on fc->purpose == > FS_CONTEXT_FOR_SUBMOUNT. Shouldn't happen on FOR_RECONFIGURE. > > ver #2) > - Added Smack support > - Made LSM parameter extraction dependent on reference != NULL. > > Signed-off-by: David Howells <dhowells@redhat.com> > Fixes: 9bc61ab18b1d ("vfs: Introduce fs_context, switch vfs_kern_mount() to it.") > Fixes: 779df6a5480f ("NFS: Ensure security label is set for root inode) > Tested-by: Jeff Layton <jlayton@kernel.org> > Reviewed-by: Jeff Layton <jlayton@kernel.org> > Acked-by: Casey Schaufler <casey@schaufler-ca.com> > Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org> > cc: Trond Myklebust <trond.myklebust@hammerspace.com> > cc: Anna Schumaker <anna@kernel.org> > cc: Alexander Viro <viro@zeniv.linux.org.uk> > cc: Scott Mayhew <smayhew@redhat.com> > cc: Jeff Layton <jlayton@kernel.org> > cc: Paul Moore <paul@paul-moore.com> > cc: linux-nfs@vger.kernel.org > cc: selinux@vger.kernel.org > cc: linux-security-module@vger.kernel.org > cc: linux-fsdevel@vger.kernel.org > Link: https://lore.kernel.org/r/165962680944.3334508.6610023900349142034.stgit@warthog.procyon.org.uk/ # v1 > Link: https://lore.kernel.org/r/165962729225.3357250.14350728846471527137.stgit@warthog.procyon.org.uk/ # v2 > Link: https://lore.kernel.org/r/165970659095.2812394.6868894171102318796.stgit@warthog.procyon.org.uk/ # v3 > Link: https://lore.kernel.org/r/166133579016.3678898.6283195019480567275.stgit@warthog.procyon.org.uk/ # v4 > Link: https://lore.kernel.org/r/217595.1662033775@warthog.procyon.org.uk/ # v5 > --- > > fs/fs_context.c | 4 +++ > fs/nfs/getroot.c | 1 + > fs/super.c | 10 +++++--- > include/linux/fs_context.h | 1 + > include/linux/lsm_hook_defs.h | 1 + > include/linux/lsm_hooks.h | 6 ++++- > include/linux/security.h | 6 +++++ > security/security.c | 5 ++++ > security/selinux/hooks.c | 25 +++++++++++++++++++ > security/smack/smack_lsm.c | 54 +++++++++++++++++++++++++++++++++++++++++ > 10 files changed, 108 insertions(+), 5 deletions(-) > > diff --git a/fs/fs_context.c b/fs/fs_context.c > index 24ce12f0db32..22248b8a88a8 100644 > --- a/fs/fs_context.c > +++ b/fs/fs_context.c > @@ -282,6 +282,10 @@ static struct fs_context *alloc_fs_context(struct file_system_type *fs_type, > break; > } > > + ret = security_fs_context_init(fc, reference); > + if (ret < 0) > + goto err_fc; > + > /* TODO: Make all filesystems support this unconditionally */ > init_fs_context = fc->fs_type->init_fs_context; > if (!init_fs_context) > diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c > index 11ff2b2e060f..651bffb0067e 100644 > --- a/fs/nfs/getroot.c > +++ b/fs/nfs/getroot.c > @@ -144,6 +144,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) > } > if (error) > goto error_splat_root; > + fc->lsm_set = true; > if (server->caps & NFS_CAP_SECURITY_LABEL && > !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) > server->caps &= ~NFS_CAP_SECURITY_LABEL; > diff --git a/fs/super.c b/fs/super.c > index 8d39e4f11cfa..f200ae0549ca 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -1553,10 +1553,12 @@ int vfs_get_tree(struct fs_context *fc) > smp_wmb(); > sb->s_flags |= SB_BORN; > > - error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL); > - if (unlikely(error)) { > - fc_drop_locked(fc); > - return error; > + if (!(fc->lsm_set)) { > + error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL); > + if (unlikely(error)) { > + fc_drop_locked(fc); > + return error; > + } > } Thinking about all the different things that an LSM could do, would it ever be possible that a LSM would want the security_sb_set_mnt_opts() call to happen here? I'm wondering if we are better off leaving it up to the LSM by passing the fs_context in the security_sb_set_mnt_opts() hook; those that want to effectively skip this call due to a submount setup already done in security_fs_context_init() can check the fs_context::purpose value in the security_sb_set_mnt_opts() hook. Thoughts?
On Fri, Nov 11, 2022 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Nov 10, 2022 at 6:09 AM David Howells <dhowells@redhat.com> wrote: > > > > When NFS superblocks are created by automounting, their LSM parameters > > aren't set in the fs_context struct prior to sget_fc() being called, > > leading to failure to match existing superblocks. > > > > Fix this by adding a new LSM hook to load fc->security for submount > > creation when alloc_fs_context() is creating the fs_context for it. > > > > However, this uncovers a further bug: nfs_get_root() initialises the > > superblock security manually by calling security_sb_set_mnt_opts() or > > security_sb_clone_mnt_opts() - but then vfs_get_tree() calls > > security_sb_set_mnt_opts(), which can lead to SELinux, at least, > > complaining. > > > > Fix that by adding a flag to the fs_context that suppresses the > > security_sb_set_mnt_opts() call in vfs_get_tree(). This can be set by NFS > > when it sets the LSM context on the new superblock. > > > > The first bug leads to messages like the following appearing in dmesg: > > > > NFS: Cache volume key already in use (nfs,4.2,2,108,106a8c0,1,,,,100000,100000,2ee,3a98,1d4c,3a98,1) > > > > Changes > > ======= > > ver #5) > > - Removed unused variable. > > - Only allocate smack_mnt_opts if we're dealing with a submount. > > > > ver #4) > > - When doing a FOR_SUBMOUNT mount, don't set the root label in SELinux or > > Smack. > > > > ver #3) > > - Made LSM parameter extraction dependent on fc->purpose == > > FS_CONTEXT_FOR_SUBMOUNT. Shouldn't happen on FOR_RECONFIGURE. > > > > ver #2) > > - Added Smack support > > - Made LSM parameter extraction dependent on reference != NULL. > > > > Signed-off-by: David Howells <dhowells@redhat.com> > > Fixes: 9bc61ab18b1d ("vfs: Introduce fs_context, switch vfs_kern_mount() to it.") > > Fixes: 779df6a5480f ("NFS: Ensure security label is set for root inode) > > Tested-by: Jeff Layton <jlayton@kernel.org> > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > Acked-by: Casey Schaufler <casey@schaufler-ca.com> > > Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > cc: Trond Myklebust <trond.myklebust@hammerspace.com> > > cc: Anna Schumaker <anna@kernel.org> > > cc: Alexander Viro <viro@zeniv.linux.org.uk> > > cc: Scott Mayhew <smayhew@redhat.com> > > cc: Jeff Layton <jlayton@kernel.org> > > cc: Paul Moore <paul@paul-moore.com> > > cc: linux-nfs@vger.kernel.org > > cc: selinux@vger.kernel.org > > cc: linux-security-module@vger.kernel.org > > cc: linux-fsdevel@vger.kernel.org > > Link: https://lore.kernel.org/r/165962680944.3334508.6610023900349142034.stgit@warthog.procyon.org.uk/ # v1 > > Link: https://lore.kernel.org/r/165962729225.3357250.14350728846471527137.stgit@warthog.procyon.org.uk/ # v2 > > Link: https://lore.kernel.org/r/165970659095.2812394.6868894171102318796.stgit@warthog.procyon.org.uk/ # v3 > > Link: https://lore.kernel.org/r/166133579016.3678898.6283195019480567275.stgit@warthog.procyon.org.uk/ # v4 > > Link: https://lore.kernel.org/r/217595.1662033775@warthog.procyon.org.uk/ # v5 > > --- > > > > fs/fs_context.c | 4 +++ > > fs/nfs/getroot.c | 1 + > > fs/super.c | 10 +++++--- > > include/linux/fs_context.h | 1 + > > include/linux/lsm_hook_defs.h | 1 + > > include/linux/lsm_hooks.h | 6 ++++- > > include/linux/security.h | 6 +++++ > > security/security.c | 5 ++++ > > security/selinux/hooks.c | 25 +++++++++++++++++++ > > security/smack/smack_lsm.c | 54 +++++++++++++++++++++++++++++++++++++++++ > > 10 files changed, 108 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fs_context.c b/fs/fs_context.c > > index 24ce12f0db32..22248b8a88a8 100644 > > --- a/fs/fs_context.c > > +++ b/fs/fs_context.c > > @@ -282,6 +282,10 @@ static struct fs_context *alloc_fs_context(struct file_system_type *fs_type, > > break; > > } > > > > + ret = security_fs_context_init(fc, reference); > > + if (ret < 0) > > + goto err_fc; > > + > > /* TODO: Make all filesystems support this unconditionally */ > > init_fs_context = fc->fs_type->init_fs_context; > > if (!init_fs_context) > > diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c > > index 11ff2b2e060f..651bffb0067e 100644 > > --- a/fs/nfs/getroot.c > > +++ b/fs/nfs/getroot.c > > @@ -144,6 +144,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) > > } > > if (error) > > goto error_splat_root; > > + fc->lsm_set = true; > > if (server->caps & NFS_CAP_SECURITY_LABEL && > > !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) > > server->caps &= ~NFS_CAP_SECURITY_LABEL; > > diff --git a/fs/super.c b/fs/super.c > > index 8d39e4f11cfa..f200ae0549ca 100644 > > --- a/fs/super.c > > +++ b/fs/super.c > > @@ -1553,10 +1553,12 @@ int vfs_get_tree(struct fs_context *fc) > > smp_wmb(); > > sb->s_flags |= SB_BORN; > > > > - error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL); > > - if (unlikely(error)) { > > - fc_drop_locked(fc); > > - return error; > > + if (!(fc->lsm_set)) { > > + error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL); > > + if (unlikely(error)) { > > + fc_drop_locked(fc); > > + return error; > > + } > > } > > Thinking about all the different things that an LSM could do, would it > ever be possible that a LSM would want the security_sb_set_mnt_opts() > call to happen here? I'm wondering if we are better off leaving it up > to the LSM by passing the fs_context in the security_sb_set_mnt_opts() > hook; those that want to effectively skip this call due to a submount > setup already done in security_fs_context_init() can check the > fs_context::purpose value in the security_sb_set_mnt_opts() hook. Actually, we could probably also create a LSM specific flag in fs_context::security to indicate that the setup has already been done. That's probably a little safer than relying on fs_context::purpose in the security_sb_set_mnt_opts() hook. > Thoughts?
On Thu, Nov 10, 2022 at 11:09:27AM +0000, David Howells wrote: > When NFS superblocks are created by automounting, their LSM parameters > aren't set in the fs_context struct prior to sget_fc() being called, > leading to failure to match existing superblocks. > > Fix this by adding a new LSM hook to load fc->security for submount > creation when alloc_fs_context() is creating the fs_context for it. FWIW, it feels like security_sb_mnt_opts_compat() would be a saner place for that. It would need to get struct dentry *reference passed to it, but that should be it...
diff --git a/fs/fs_context.c b/fs/fs_context.c index 24ce12f0db32..22248b8a88a8 100644 --- a/fs/fs_context.c +++ b/fs/fs_context.c @@ -282,6 +282,10 @@ static struct fs_context *alloc_fs_context(struct file_system_type *fs_type, break; } + ret = security_fs_context_init(fc, reference); + if (ret < 0) + goto err_fc; + /* TODO: Make all filesystems support this unconditionally */ init_fs_context = fc->fs_type->init_fs_context; if (!init_fs_context) diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c index 11ff2b2e060f..651bffb0067e 100644 --- a/fs/nfs/getroot.c +++ b/fs/nfs/getroot.c @@ -144,6 +144,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) } if (error) goto error_splat_root; + fc->lsm_set = true; if (server->caps & NFS_CAP_SECURITY_LABEL && !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) server->caps &= ~NFS_CAP_SECURITY_LABEL; diff --git a/fs/super.c b/fs/super.c index 8d39e4f11cfa..f200ae0549ca 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1553,10 +1553,12 @@ int vfs_get_tree(struct fs_context *fc) smp_wmb(); sb->s_flags |= SB_BORN; - error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL); - if (unlikely(error)) { - fc_drop_locked(fc); - return error; + if (!(fc->lsm_set)) { + error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL); + if (unlikely(error)) { + fc_drop_locked(fc); + return error; + } } /* diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index 13fa6f3df8e4..3876dd96bb20 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -110,6 +110,7 @@ struct fs_context { bool need_free:1; /* Need to call ops->free() */ bool global:1; /* Goes into &init_user_ns */ bool oldapi:1; /* Coming from mount(2) */ + bool lsm_set:1; /* security_sb_set/clone_mnt_opts() already done */ }; struct fs_context_operations { diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index ec119da1d89b..256b6b0c0e6e 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -54,6 +54,7 @@ LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, struct file *f LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, struct linux_binprm *bprm) +LSM_HOOK(int, 0, fs_context_init, struct fs_context *fc, struct dentry *reference) LSM_HOOK(int, 0, fs_context_dup, struct fs_context *fc, struct fs_context *src_sc) LSM_HOOK(int, -ENOPARAM, fs_context_parse_param, struct fs_context *fc, diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 4ec80b96c22e..a4885eb620e7 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -87,8 +87,12 @@ * Security hooks for mount using fs_context. * [See also Documentation/filesystems/mount_api.rst] * + * @fs_context_init: + * Initialise fc->security. This is initialised to NULL by the caller. + * @fc indicates the new filesystem context. + * @dentry indicates a reference for submount/remount * @fs_context_dup: - * Allocate and attach a security structure to sc->security. This pointer + * Allocate and attach a security structure to fc->security. This pointer * is initialised to NULL by the caller. * @fc indicates the new filesystem context. * @src_fc indicates the original filesystem context. diff --git a/include/linux/security.h b/include/linux/security.h index ca1b7109c0db..6c626ae3bb33 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -293,6 +293,7 @@ int security_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file); int security_bprm_check(struct linux_binprm *bprm); void security_bprm_committing_creds(struct linux_binprm *bprm); void security_bprm_committed_creds(struct linux_binprm *bprm); +int security_fs_context_init(struct fs_context *fc, struct dentry *reference); int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc); int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param); int security_sb_alloc(struct super_block *sb); @@ -625,6 +626,11 @@ static inline void security_bprm_committed_creds(struct linux_binprm *bprm) { } +static inline int security_fs_context_init(struct fs_context *fc, + struct dentry *reference) +{ + return 0; +} static inline int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc) { diff --git a/security/security.c b/security/security.c index 79d82cb6e469..0732d873740a 100644 --- a/security/security.c +++ b/security/security.c @@ -882,6 +882,11 @@ void security_bprm_committed_creds(struct linux_binprm *bprm) call_void_hook(bprm_committed_creds, bprm); } +int security_fs_context_init(struct fs_context *fc, struct dentry *reference) +{ + return call_int_hook(fs_context_init, 0, fc, reference); +} + int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc) { return call_int_hook(fs_context_dup, 0, fc, src_fc); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f553c370397e..426acfc87a0a 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2766,6 +2766,30 @@ static int selinux_umount(struct vfsmount *mnt, int flags) FILESYSTEM__UNMOUNT, NULL); } +static int selinux_fs_context_init(struct fs_context *fc, + struct dentry *reference) +{ + const struct superblock_security_struct *sbsec; + struct selinux_mnt_opts *opts; + + if (fc->purpose == FS_CONTEXT_FOR_SUBMOUNT) { + opts = kzalloc(sizeof(*opts), GFP_KERNEL); + if (!opts) + return -ENOMEM; + + sbsec = selinux_superblock(reference->d_sb); + if (sbsec->flags & FSCONTEXT_MNT) + opts->fscontext_sid = sbsec->sid; + if (sbsec->flags & CONTEXT_MNT) + opts->context_sid = sbsec->mntpoint_sid; + if (sbsec->flags & DEFCONTEXT_MNT) + opts->defcontext_sid = sbsec->def_sid; + fc->security = opts; + } + + return 0; +} + static int selinux_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc) { @@ -7263,6 +7287,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { /* * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE */ + LSM_HOOK_INIT(fs_context_init, selinux_fs_context_init), LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup), LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param), LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts), diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index b6306d71c908..bef2e10320d4 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -611,6 +611,59 @@ static int smack_add_opt(int token, const char *s, void **mnt_opts) return -EINVAL; } +/** + * smack_fs_context_init - Initialise security data for a filesystem context + * @fc: The filesystem context. + * @reference: Reference dentry (automount/reconfigure) or NULL + * + * Returns 0 on success or -ENOMEM on error. + */ +static int smack_fs_context_init(struct fs_context *fc, + struct dentry *reference) +{ + struct superblock_smack *sbsp; + struct smack_mnt_opts *ctx; + struct inode_smack *isp; + + if (fc->purpose == FS_CONTEXT_FOR_SUBMOUNT) { + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + fc->security = ctx; + + sbsp = smack_superblock(reference->d_sb); + isp = smack_inode(reference->d_sb->s_root->d_inode); + + if (sbsp->smk_default) { + ctx->fsdefault = kstrdup(sbsp->smk_default->smk_known, GFP_KERNEL); + if (!ctx->fsdefault) + return -ENOMEM; + } + + if (sbsp->smk_floor) { + ctx->fsfloor = kstrdup(sbsp->smk_floor->smk_known, GFP_KERNEL); + if (!ctx->fsfloor) + return -ENOMEM; + } + + if (sbsp->smk_hat) { + ctx->fshat = kstrdup(sbsp->smk_hat->smk_known, GFP_KERNEL); + if (!ctx->fshat) + return -ENOMEM; + } + + if (isp->smk_flags & SMK_INODE_TRANSMUTE) { + if (sbsp->smk_root) { + ctx->fstransmute = kstrdup(sbsp->smk_root->smk_known, GFP_KERNEL); + if (!ctx->fstransmute) + return -ENOMEM; + } + } + } + + return 0; +} + /** * smack_fs_context_dup - Duplicate the security data on fs_context duplication * @fc: The new filesystem context. @@ -4792,6 +4845,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme), LSM_HOOK_INIT(syslog, smack_syslog), + LSM_HOOK_INIT(fs_context_init, smack_fs_context_init), LSM_HOOK_INIT(fs_context_dup, smack_fs_context_dup), LSM_HOOK_INIT(fs_context_parse_param, smack_fs_context_parse_param),