| Message ID | 20200303225837.1557210-1-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFS: Ensure security label is set for root inode | expand |
On Tue, 2020-03-03 at 17:58 -0500, Scott Mayhew wrote: > When using NFSv4.2, the security label for the root inode should be > set > via a call to nfs_setsecurity() during the mount process, otherwise > the > inode will appear as unlabeled for up to acdirmin seconds. Currently > the label for the root inode is allocated, retrieved, and freed > entirely > witin nfs4_proc_get_root(). > > Add a field for the label to the nfs_fattr struct, and allocate & > free > the label in nfs_get_root(), where we also add a call to > nfs_setsecurity(). Note that for the call to nfs_setsecurity() to > succeed, it's necessary to also move the logic calling > security_sb_{set,clone}_security() from nfs_get_tree_common() down > into > nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in > the > super_block's security flags and nfs_setsecurity() will silently > fail. I built and tested this patch on selinux-next (note that the NFS module is a few patches behind). The unlabeled problem is solved, however using: mount -t nfs -o vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0 localhost:$TESTDIR /mnt/selinux-testsuite I get the message: mount.nfs: an incorrect mount option was specified with a log entry: SELinux: mount invalid. Same superblock, different security settings for (dev 0:42, type nfs) If I use "fscontext=" instead then works okay. Using no context option also works. I guess the rootcontext= option should still work ??? > > Reported-by: Richard Haines <richard_c_haines@btinternet.com> > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > fs/nfs/getroot.c | 39 +++++++++++++++++++++++++++++++++++---- > fs/nfs/nfs4proc.c | 12 +++--------- > fs/nfs/super.c | 25 ------------------------- > include/linux/nfs_xdr.h | 1 + > 4 files changed, 39 insertions(+), 38 deletions(-) > > diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c > index b012c2668a1f..6d0628149390 100644 > --- a/fs/nfs/getroot.c > +++ b/fs/nfs/getroot.c > @@ -73,6 +73,7 @@ int nfs_get_root(struct super_block *s, struct > fs_context *fc) > struct inode *inode; > char *name; > int error = -ENOMEM; > + unsigned long kflags = 0, kflags_out = 0; > > name = kstrdup(fc->source, GFP_KERNEL); > if (!name) > @@ -83,11 +84,14 @@ int nfs_get_root(struct super_block *s, struct > fs_context *fc) > if (fsinfo.fattr == NULL) > goto out_name; > > + fsinfo.fattr->label = nfs4_label_alloc(server, GFP_KERNEL); > + if (IS_ERR(fsinfo.fattr->label)) > + goto out_fattr; > error = server->nfs_client->rpc_ops->getroot(server, ctx- > >mntfh, &fsinfo); > if (error < 0) { > dprintk("nfs_get_root: getattr error = %d\n", -error); > nfs_errorf(fc, "NFS: Couldn't getattr on root"); > - goto out_fattr; > + goto out_label; > } > > inode = nfs_fhget(s, ctx->mntfh, fsinfo.fattr, NULL); > @@ -95,12 +99,12 @@ int nfs_get_root(struct super_block *s, struct > fs_context *fc) > dprintk("nfs_get_root: get root inode failed\n"); > error = PTR_ERR(inode); > nfs_errorf(fc, "NFS: Couldn't get root inode"); > - goto out_fattr; > + goto out_label; > } > > error = nfs_superblock_set_dummy_root(s, inode); > if (error != 0) > - goto out_fattr; > + goto out_label; > > /* root dentries normally start off anonymous and get spliced > in later > * if the dentry tree reaches them; however if the dentry > already > @@ -111,7 +115,7 @@ int nfs_get_root(struct super_block *s, struct > fs_context *fc) > dprintk("nfs_get_root: get root dentry failed\n"); > error = PTR_ERR(root); > nfs_errorf(fc, "NFS: Couldn't get root dentry"); > - goto out_fattr; > + goto out_label; > } > > security_d_instantiate(root, inode); > @@ -123,12 +127,39 @@ int nfs_get_root(struct super_block *s, struct > fs_context *fc) > } > spin_unlock(&root->d_lock); > fc->root = root; > + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL) > + kflags |= SECURITY_LSM_NATIVE_LABELS; > + if (ctx->clone_data.sb) { > + if (d_inode(fc->root)->i_fop != &nfs_dir_operations) { > + error = -ESTALE; > + goto error_splat_root; > + } > + /* clone any lsm security options from the parent to > the new sb */ > + error = security_sb_clone_mnt_opts(ctx->clone_data.sb, > s, kflags, > + &kflags_out); > + } else { > + error = security_sb_set_mnt_opts(s, fc->security, > + kflags, > &kflags_out); > + } > + if (error) > + goto error_splat_root; > + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && > + !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) > + NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL; > + > + nfs_setsecurity(inode, fsinfo.fattr, fsinfo.fattr->label); > error = 0; > > +out_label: > + nfs4_label_free(fsinfo.fattr->label); > out_fattr: > nfs_free_fattr(fsinfo.fattr); > out_name: > kfree(name); > out: > return error; > +error_splat_root: > + dput(fc->root); > + fc->root = NULL; > + goto out_label; > } > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 69b7ab7a5815..cb34e840e4fb 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -4002,7 +4002,7 @@ static int nfs4_proc_get_root(struct nfs_server > *server, struct nfs_fh *mntfh, > { > int error; > struct nfs_fattr *fattr = info->fattr; > - struct nfs4_label *label = NULL; > + struct nfs4_label *label = fattr->label; > > error = nfs4_server_capabilities(server, mntfh); > if (error < 0) { > @@ -4010,23 +4010,17 @@ static int nfs4_proc_get_root(struct > nfs_server *server, struct nfs_fh *mntfh, > return error; > } > > - label = nfs4_label_alloc(server, GFP_KERNEL); > - if (IS_ERR(label)) > - return PTR_ERR(label); > - > error = nfs4_proc_getattr(server, mntfh, fattr, label, NULL); > if (error < 0) { > dprintk("nfs4_get_root: getattr error = %d\n", -error); > - goto err_free_label; > + goto out; > } > > if (fattr->valid & NFS_ATTR_FATTR_FSID && > !nfs_fsid_equal(&server->fsid, &fattr->fsid)) > memcpy(&server->fsid, &fattr->fsid, sizeof(server- > >fsid)); > > -err_free_label: > - nfs4_label_free(label); > - > +out: > return error; > } > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c > index dada09b391c6..bb14bede6da5 100644 > --- a/fs/nfs/super.c > +++ b/fs/nfs/super.c > @@ -1179,7 +1179,6 @@ int nfs_get_tree_common(struct fs_context *fc) > struct super_block *s; > int (*compare_super)(struct super_block *, struct fs_context *) > = nfs_compare_super; > struct nfs_server *server = ctx->server; > - unsigned long kflags = 0, kflags_out = 0; > int error; > > ctx->server = NULL; > @@ -1239,26 +1238,6 @@ int nfs_get_tree_common(struct fs_context *fc) > goto error_splat_super; > } > > - if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL) > - kflags |= SECURITY_LSM_NATIVE_LABELS; > - if (ctx->clone_data.sb) { > - if (d_inode(fc->root)->i_fop != &nfs_dir_operations) { > - error = -ESTALE; > - goto error_splat_root; > - } > - /* clone any lsm security options from the parent to > the new sb */ > - error = security_sb_clone_mnt_opts(ctx->clone_data.sb, > s, kflags, > - &kflags_out); > - } else { > - error = security_sb_set_mnt_opts(s, fc->security, > - kflags, > &kflags_out); > - } > - if (error) > - goto error_splat_root; > - if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && > - !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) > - NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL; > - > s->s_flags |= SB_ACTIVE; > error = 0; > > @@ -1268,10 +1247,6 @@ int nfs_get_tree_common(struct fs_context *fc) > out_err_nosb: > nfs_free_server(server); > goto out; > - > -error_splat_root: > - dput(fc->root); > - fc->root = NULL; > error_splat_super: > deactivate_locked_super(s); > goto out; > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > index 94c77ed55ce1..6838c149f335 100644 > --- a/include/linux/nfs_xdr.h > +++ b/include/linux/nfs_xdr.h > @@ -75,6 +75,7 @@ struct nfs_fattr { > struct nfs4_string *owner_name; > struct nfs4_string *group_name; > struct nfs4_threshold *mdsthreshold; /* pNFS threshold > hints */ > + struct nfs4_label *label; > }; > > #define NFS_ATTR_FATTR_TYPE (1U << 0)
On Wed, 04 Mar 2020, Richard Haines wrote: > On Tue, 2020-03-03 at 17:58 -0500, Scott Mayhew wrote: > > When using NFSv4.2, the security label for the root inode should be > > set > > via a call to nfs_setsecurity() during the mount process, otherwise > > the > > inode will appear as unlabeled for up to acdirmin seconds. Currently > > the label for the root inode is allocated, retrieved, and freed > > entirely > > witin nfs4_proc_get_root(). > > > > Add a field for the label to the nfs_fattr struct, and allocate & > > free > > the label in nfs_get_root(), where we also add a call to > > nfs_setsecurity(). Note that for the call to nfs_setsecurity() to > > succeed, it's necessary to also move the logic calling > > security_sb_{set,clone}_security() from nfs_get_tree_common() down > > into > > nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in > > the > > super_block's security flags and nfs_setsecurity() will silently > > fail. > > I built and tested this patch on selinux-next (note that the NFS module > is a few patches behind). > The unlabeled problem is solved, however using: > > mount -t nfs -o > vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0 > localhost:$TESTDIR /mnt/selinux-testsuite > > I get the message: > mount.nfs: an incorrect mount option was specified > with a log entry: > SELinux: mount invalid. Same superblock, different security > settings for (dev 0:42, type nfs) > > If I use "fscontext=" instead then works okay. Using no context option > also works. I guess the rootcontext= option should still work ??? Thanks for testing. It definitely wasn't my intention to break anything, so I'll look into it. -Scott > > > > > Reported-by: Richard Haines <richard_c_haines@btinternet.com> > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > --- > > fs/nfs/getroot.c | 39 +++++++++++++++++++++++++++++++++++---- > > fs/nfs/nfs4proc.c | 12 +++--------- > > fs/nfs/super.c | 25 ------------------------- > > include/linux/nfs_xdr.h | 1 + > > 4 files changed, 39 insertions(+), 38 deletions(-) > > > > diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c > > index b012c2668a1f..6d0628149390 100644 > > --- a/fs/nfs/getroot.c > > +++ b/fs/nfs/getroot.c > > @@ -73,6 +73,7 @@ int nfs_get_root(struct super_block *s, struct > > fs_context *fc) > > struct inode *inode; > > char *name; > > int error = -ENOMEM; > > + unsigned long kflags = 0, kflags_out = 0; > > > > name = kstrdup(fc->source, GFP_KERNEL); > > if (!name) > > @@ -83,11 +84,14 @@ int nfs_get_root(struct super_block *s, struct > > fs_context *fc) > > if (fsinfo.fattr == NULL) > > goto out_name; > > > > + fsinfo.fattr->label = nfs4_label_alloc(server, GFP_KERNEL); > > + if (IS_ERR(fsinfo.fattr->label)) > > + goto out_fattr; > > error = server->nfs_client->rpc_ops->getroot(server, ctx- > > >mntfh, &fsinfo); > > if (error < 0) { > > dprintk("nfs_get_root: getattr error = %d\n", -error); > > nfs_errorf(fc, "NFS: Couldn't getattr on root"); > > - goto out_fattr; > > + goto out_label; > > } > > > > inode = nfs_fhget(s, ctx->mntfh, fsinfo.fattr, NULL); > > @@ -95,12 +99,12 @@ int nfs_get_root(struct super_block *s, struct > > fs_context *fc) > > dprintk("nfs_get_root: get root inode failed\n"); > > error = PTR_ERR(inode); > > nfs_errorf(fc, "NFS: Couldn't get root inode"); > > - goto out_fattr; > > + goto out_label; > > } > > > > error = nfs_superblock_set_dummy_root(s, inode); > > if (error != 0) > > - goto out_fattr; > > + goto out_label; > > > > /* root dentries normally start off anonymous and get spliced > > in later > > * if the dentry tree reaches them; however if the dentry > > already > > @@ -111,7 +115,7 @@ int nfs_get_root(struct super_block *s, struct > > fs_context *fc) > > dprintk("nfs_get_root: get root dentry failed\n"); > > error = PTR_ERR(root); > > nfs_errorf(fc, "NFS: Couldn't get root dentry"); > > - goto out_fattr; > > + goto out_label; > > } > > > > security_d_instantiate(root, inode); > > @@ -123,12 +127,39 @@ int nfs_get_root(struct super_block *s, struct > > fs_context *fc) > > } > > spin_unlock(&root->d_lock); > > fc->root = root; > > + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL) > > + kflags |= SECURITY_LSM_NATIVE_LABELS; > > + if (ctx->clone_data.sb) { > > + if (d_inode(fc->root)->i_fop != &nfs_dir_operations) { > > + error = -ESTALE; > > + goto error_splat_root; > > + } > > + /* clone any lsm security options from the parent to > > the new sb */ > > + error = security_sb_clone_mnt_opts(ctx->clone_data.sb, > > s, kflags, > > + &kflags_out); > > + } else { > > + error = security_sb_set_mnt_opts(s, fc->security, > > + kflags, > > &kflags_out); > > + } > > + if (error) > > + goto error_splat_root; > > + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && > > + !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) > > + NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL; > > + > > + nfs_setsecurity(inode, fsinfo.fattr, fsinfo.fattr->label); > > error = 0; > > > > +out_label: > > + nfs4_label_free(fsinfo.fattr->label); > > out_fattr: > > nfs_free_fattr(fsinfo.fattr); > > out_name: > > kfree(name); > > out: > > return error; > > +error_splat_root: > > + dput(fc->root); > > + fc->root = NULL; > > + goto out_label; > > } > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > index 69b7ab7a5815..cb34e840e4fb 100644 > > --- a/fs/nfs/nfs4proc.c > > +++ b/fs/nfs/nfs4proc.c > > @@ -4002,7 +4002,7 @@ static int nfs4_proc_get_root(struct nfs_server > > *server, struct nfs_fh *mntfh, > > { > > int error; > > struct nfs_fattr *fattr = info->fattr; > > - struct nfs4_label *label = NULL; > > + struct nfs4_label *label = fattr->label; > > > > error = nfs4_server_capabilities(server, mntfh); > > if (error < 0) { > > @@ -4010,23 +4010,17 @@ static int nfs4_proc_get_root(struct > > nfs_server *server, struct nfs_fh *mntfh, > > return error; > > } > > > > - label = nfs4_label_alloc(server, GFP_KERNEL); > > - if (IS_ERR(label)) > > - return PTR_ERR(label); > > - > > error = nfs4_proc_getattr(server, mntfh, fattr, label, NULL); > > if (error < 0) { > > dprintk("nfs4_get_root: getattr error = %d\n", -error); > > - goto err_free_label; > > + goto out; > > } > > > > if (fattr->valid & NFS_ATTR_FATTR_FSID && > > !nfs_fsid_equal(&server->fsid, &fattr->fsid)) > > memcpy(&server->fsid, &fattr->fsid, sizeof(server- > > >fsid)); > > > > -err_free_label: > > - nfs4_label_free(label); > > - > > +out: > > return error; > > } > > > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c > > index dada09b391c6..bb14bede6da5 100644 > > --- a/fs/nfs/super.c > > +++ b/fs/nfs/super.c > > @@ -1179,7 +1179,6 @@ int nfs_get_tree_common(struct fs_context *fc) > > struct super_block *s; > > int (*compare_super)(struct super_block *, struct fs_context *) > > = nfs_compare_super; > > struct nfs_server *server = ctx->server; > > - unsigned long kflags = 0, kflags_out = 0; > > int error; > > > > ctx->server = NULL; > > @@ -1239,26 +1238,6 @@ int nfs_get_tree_common(struct fs_context *fc) > > goto error_splat_super; > > } > > > > - if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL) > > - kflags |= SECURITY_LSM_NATIVE_LABELS; > > - if (ctx->clone_data.sb) { > > - if (d_inode(fc->root)->i_fop != &nfs_dir_operations) { > > - error = -ESTALE; > > - goto error_splat_root; > > - } > > - /* clone any lsm security options from the parent to > > the new sb */ > > - error = security_sb_clone_mnt_opts(ctx->clone_data.sb, > > s, kflags, > > - &kflags_out); > > - } else { > > - error = security_sb_set_mnt_opts(s, fc->security, > > - kflags, > > &kflags_out); > > - } > > - if (error) > > - goto error_splat_root; > > - if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && > > - !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) > > - NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL; > > - > > s->s_flags |= SB_ACTIVE; > > error = 0; > > > > @@ -1268,10 +1247,6 @@ int nfs_get_tree_common(struct fs_context *fc) > > out_err_nosb: > > nfs_free_server(server); > > goto out; > > - > > -error_splat_root: > > - dput(fc->root); > > - fc->root = NULL; > > error_splat_super: > > deactivate_locked_super(s); > > goto out; > > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > > index 94c77ed55ce1..6838c149f335 100644 > > --- a/include/linux/nfs_xdr.h > > +++ b/include/linux/nfs_xdr.h > > @@ -75,6 +75,7 @@ struct nfs_fattr { > > struct nfs4_string *owner_name; > > struct nfs4_string *group_name; > > struct nfs4_threshold *mdsthreshold; /* pNFS threshold > > hints */ > > + struct nfs4_label *label; > > }; > > > > #define NFS_ATTR_FATTR_TYPE (1U << 0) >
On Wed, Mar 4, 2020 at 9:37 AM Scott Mayhew <smayhew@redhat.com> wrote: > > On Wed, 04 Mar 2020, Richard Haines wrote: > > I built and tested this patch on selinux-next (note that the NFS module > > is a few patches behind). > > The unlabeled problem is solved, however using: > > > > mount -t nfs -o > > vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0 > > localhost:$TESTDIR /mnt/selinux-testsuite > > > > I get the message: > > mount.nfs: an incorrect mount option was specified > > with a log entry: > > SELinux: mount invalid. Same superblock, different security > > settings for (dev 0:42, type nfs) > > > > If I use "fscontext=" instead then works okay. Using no context option > > also works. I guess the rootcontext= option should still work ??? > > Thanks for testing. It definitely wasn't my intention to break > anything, so I'll look into it. I'm not sure that rootcontext= should be supported or is supportable over labeled NFS. It's primary use case is to allow assigning a specific context other than the default policy-defined one to the root directory for filesystems that support labeling but don't have existing labels on their root directories, e.g. tmpfs mounts. Even if we set the rootcontext based on rootcontext= during mount(2), it would likely get overridden by subsequent attribute fetches from the server I would think (e.g. it probably already switches to the context from the server after 30 seconds or so?). As long as the separate context= option continues to work correctly on NFS, I'm not overly concerned about this. I should note that we are getting similar errors though when trying to specify any context-related mount options on NFS via the new fsconfig(2) system call, see https://github.com/SELinuxProject/selinux-kernel/issues/49 I don't know if this change in when security_sb_set_mnt_opts() will alter that situation any. Also, FYI, we have recently made it possible to run the selinux-testsuite without errors within a labeled NFS mount. If you clone https://github.com/SELinuxProject/selinux-testsuite/ and follow the README.md instructions including the NFS section and run ./tools/nfs.sh, it will export and mount the testsuite directory via labeled NFS over loopback and run all tests that can be supported over NFS, and then runs a few specific tests for context= mount options (but not the other mount options at present). It still needs some further enhancements as per https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492 but it at least provides some degree of regression testing.
On Wed, 04 Mar 2020, Stephen Smalley wrote: > On Wed, Mar 4, 2020 at 9:37 AM Scott Mayhew <smayhew@redhat.com> wrote: > > > > On Wed, 04 Mar 2020, Richard Haines wrote: > > > I built and tested this patch on selinux-next (note that the NFS module > > > is a few patches behind). > > > The unlabeled problem is solved, however using: > > > > > > mount -t nfs -o > > > vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0 > > > localhost:$TESTDIR /mnt/selinux-testsuite > > > > > > I get the message: > > > mount.nfs: an incorrect mount option was specified > > > with a log entry: > > > SELinux: mount invalid. Same superblock, different security > > > settings for (dev 0:42, type nfs) > > > > > > If I use "fscontext=" instead then works okay. Using no context option > > > also works. I guess the rootcontext= option should still work ??? > > > > Thanks for testing. It definitely wasn't my intention to break > > anything, so I'll look into it. > > I'm not sure that rootcontext= should be supported or is supportable > over labeled NFS. Should rootcontext= be supported for NFS versions < 4.2? If not then maybe it that option should be rejected for nfs and nfs4 fstypes in selinux_set_mnt_opts(). > It's primary use case is to allow assigning a specific context other > than the default policy-defined one > to the root directory for filesystems that support labeling but don't > have existing labels on their root > directories, e.g. tmpfs mounts. Even if we set the rootcontext based > on rootcontext= during mount(2), > it would likely get overridden by subsequent attribute fetches from > the server I would think (e.g. it probably > already switches to the context from the server after 30 seconds or Yes, that's what happens. If we wanted to retain that behavior moving forward, then we need to avoid calling nfs_setsecurity() for the root inode when the rootcontext= option was used. To do that, I think we'd need to add a flag that could be passed back to NFS via the set_kern_flags parameter of selinux_set_mnt_opts(). > so?). As long as the separate context= option > continues to work correctly on NFS, I'm not overly concerned about this. Yep, the context= option still works. > > I should note that we are getting similar errors though when trying to > specify any context-related > mount options on NFS via the new fsconfig(2) system call, see > https://github.com/SELinuxProject/selinux-kernel/issues/49 > I don't know if this change in when security_sb_set_mnt_opts() will > alter that situation any. > > Also, FYI, we have recently made it possible to run the > selinux-testsuite without errors within a labeled NFS > mount. If you clone > https://github.com/SELinuxProject/selinux-testsuite/ and follow the > README.md > instructions including the NFS section and run ./tools/nfs.sh, it will > export and mount the testsuite directory > via labeled NFS over loopback and run all tests that can be supported > over NFS, and then runs a few specific > tests for context= mount options (but not the other mount options at > present). It still needs some further > enhancements as per > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492 > but it at least provides some degree of regression testing. Thanks. I ran this a few times and aside from an exportfs bug everything passed. I'll be sure to run this in the future too. -Scott
On Fri, 2020-03-06 at 17:01 -0500, Scott Mayhew wrote: > On Wed, 04 Mar 2020, Stephen Smalley wrote: > > > On Wed, Mar 4, 2020 at 9:37 AM Scott Mayhew <smayhew@redhat.com> > > wrote: > > > On Wed, 04 Mar 2020, Richard Haines wrote: > > > > I built and tested this patch on selinux-next (note that the > > > > NFS module > > > > is a few patches behind). > > > > The unlabeled problem is solved, however using: > > > > > > > > mount -t nfs -o > > > > vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s > > > > 0 > > > > localhost:$TESTDIR /mnt/selinux-testsuite > > > > > > > > I get the message: > > > > mount.nfs: an incorrect mount option was specified > > > > with a log entry: > > > > SELinux: mount invalid. Same superblock, different > > > > security > > > > settings for (dev 0:42, type nfs) > > > > > > > > If I use "fscontext=" instead then works okay. Using no context > > > > option > > > > also works. I guess the rootcontext= option should still work > > > > ??? > > > > > > Thanks for testing. It definitely wasn't my intention to break > > > anything, so I'll look into it. > > > > I'm not sure that rootcontext= should be supported or is > > supportable > > over labeled NFS. > > Should rootcontext= be supported for NFS versions < 4.2? If not then > maybe it that option should be rejected for nfs and nfs4 fstypes in > selinux_set_mnt_opts(). > > > It's primary use case is to allow assigning a specific context > > other > > than the default policy-defined one > > to the root directory for filesystems that support labeling but > > don't > > have existing labels on their root > > directories, e.g. tmpfs mounts. Even if we set the rootcontext > > based > > on rootcontext= during mount(2), > > it would likely get overridden by subsequent attribute fetches from > > the server I would think (e.g. it probably > > already switches to the context from the server after 30 seconds or > > Yes, that's what happens. If we wanted to retain that behavior > moving > forward, then we need to avoid calling nfs_setsecurity() for the root > inode when the rootcontext= option was used. To do that, I think > we'd > need to add a flag that could be passed back to NFS via the > set_kern_flags parameter of selinux_set_mnt_opts(). > > > so?). As long as the separate context= option > > continues to work correctly on NFS, I'm not overly concerned about > > this. > > Yep, the context= option still works. > > I should note that we are getting similar errors though when trying > > to > > specify any context-related > > mount options on NFS via the new fsconfig(2) system call, see > > https://github.com/SELinuxProject/selinux-kernel/issues/49 I've done further testing and found that with this patch the fsconfig(2) problem is also resolved for nfs (provided the rootcontext is not specified). > > I don't know if this change in when security_sb_set_mnt_opts() will > > alter that situation any. > > > > Also, FYI, we have recently made it possible to run the > > selinux-testsuite without errors within a labeled NFS > > mount. If you clone > > https://github.com/SELinuxProject/selinux-testsuite/ and follow the > > README.md > > instructions including the NFS section and run ./tools/nfs.sh, it > > will > > export and mount the testsuite directory > > via labeled NFS over loopback and run all tests that can be > > supported > > over NFS, and then runs a few specific > > tests for context= mount options (but not the other mount options > > at > > present). It still needs some further > > enhancements as per > > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492 > > but it at least provides some degree of regression testing. Could you describe how I could test these, also are there any other SELinux tests that may be useful (with howto's). I'm almost ready to post another set of RFC test patches, but can add more. > > Thanks. I ran this a few times and aside from an exportfs bug > everything passed. I'll be sure to run this in the future too. > > -Scott >
On Fri, Mar 6, 2020 at 5:01 PM Scott Mayhew <smayhew@redhat.com> wrote: > > On Wed, 04 Mar 2020, Stephen Smalley wrote: > > I'm not sure that rootcontext= should be supported or is supportable > > over labeled NFS. > > Should rootcontext= be supported for NFS versions < 4.2? If not then > maybe it that option should be rejected for nfs and nfs4 fstypes in > selinux_set_mnt_opts(). Looks like it gets ignored currently? $ sudo exportfs -orw,no_root_squash localhost:/home $ sudo mkdir -p /mnt/selinux-testsuite $ sudo mount -t nfs -o vers=4.0,rootcontext=system_u:object_r:etc_t:s0 localhost:/home/sds/selinux-testsuite /mnt/selinux-testsuite $ ls -Zd /mnt/selinux-testsuite system_u:object_r:nfs_t:s0 /mnt/selinux-testsuite $ mount | grep testsuite localhost:/home/sds/selinux-testsuite on /mnt/selinux-testsuite type nfs4 (rw,relatime,rootcontext=system_u:object_r:etc_t:s0,vers=4.0,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp6,timeo=600,retrans=2,sec=sys,clientaddr=::1,local_lock=none,addr=::1) I don't think we need to support it, but I don't know if we explicitly need to test and reject it for nfs/nfs4. > > It's primary use case is to allow assigning a specific context other > > than the default policy-defined one > > to the root directory for filesystems that support labeling but don't > > have existing labels on their root > > directories, e.g. tmpfs mounts. Even if we set the rootcontext based > > on rootcontext= during mount(2), > > it would likely get overridden by subsequent attribute fetches from > > the server I would think (e.g. it probably > > already switches to the context from the server after 30 seconds or > > Yes, that's what happens. If we wanted to retain that behavior moving > forward, then we need to avoid calling nfs_setsecurity() for the root > inode when the rootcontext= option was used. To do that, I think we'd > need to add a flag that could be passed back to NFS via the > set_kern_flags parameter of selinux_set_mnt_opts(). Doesn't seem justified. > > so?). As long as the separate context= option > > continues to work correctly on NFS, I'm not overly concerned about this. > > Yep, the context= option still works. Great, then I have no objections to this patch.
On Sun, Mar 8, 2020 at 1:47 PM Richard Haines <richard_c_haines@btinternet.com> wrote: > > On Fri, 2020-03-06 at 17:01 -0500, Scott Mayhew wrote: > > On Wed, 04 Mar 2020, Stephen Smalley wrote: > > > I should note that we are getting similar errors though when trying > > > to > > > specify any context-related > > > mount options on NFS via the new fsconfig(2) system call, see > > > https://github.com/SELinuxProject/selinux-kernel/issues/49 > I've done further testing and found that with this patch the > fsconfig(2) problem is also resolved for nfs (provided the rootcontext > is not specified). Excellent, two bugs fixed with one patch! >>> It still needs some further > > > enhancements as per > > > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492 > > > but it at least provides some degree of regression testing. > Could you describe how I could test these, also are there any other > SELinux tests that may be useful (with howto's). I'm almost ready to > post another set of RFC test patches, but can add more. The ones identified in that github issue comment would simply be additional tests in tools/nfs.sh unless they happen to already be covered by your fs/filesystem tests once those are applied to the host/native filesystem instead of just ext4. The test cases are: 0. Test the bug fixed by this patch, i.e. perform mount of a security_label exported filesystem, check the label of the mounted directory to confirm it isn't unlabeled. That's a NFS-specific test, goes in tools/nfs.sh. 1. Mount the same filesystem twice with two different sets of context mount options, check that mount(2) fails with errno EINVAL. Test cases might include a) mount first without any context mount options, then try mounting a 2nd time with context mount options and vice versa, b) mounting with the same set of context options (e.g. both using context=) but different context values, c) mounting with different sets of context options (e.g. one uses context=, the other uses fscontext=). This test could be done in fs/filesystem for any filesystem type, not NFS-specific. 2. Mount a security_label exported NFS filesystem twice, confirm that NFS security labeling support isn't silently disabled by trying to set a label on a file and confirm it is set (fixed by kernel commit 3815a245b50124f0865415dcb606a034e97494d4). This would go in tools/nfs.sh since it is NFS-specific. 3. Perform a context= mount of a security_label exported NFS filesystem, check that pre-existing files within the mount show up with the context= value not the underlying xattr value (fixed by kernel commit 0b4d3452b8b4a5309b4445b900e3cec022cca95a). My original version of tools/nfs.sh actually would have caught this because it was testing the context of the nfs.sh script file itself within the context mount but I dropped it back to only checking the top-level mount directory when I moved tools/nfs.sh to avoid depending on a fixed location for it, so it won't be caught currently. We could just change it back to testing the context of a pre-existing file within the mount; any file will do. This would go in tools/nfs.sh, NFS-specific. 4. Ensuring that all of the tests/filesystem and tests/fs_filesystem tests that make sense for NFS are being run on the labeled NFS mount itself when run via tools/nfs.sh and not just on an ext4 mount created by the test script.
On Mon, 2020-03-09 at 09:35 -0400, Stephen Smalley wrote: > On Sun, Mar 8, 2020 at 1:47 PM Richard Haines > <richard_c_haines@btinternet.com> wrote: > > On Fri, 2020-03-06 at 17:01 -0500, Scott Mayhew wrote: > > > On Wed, 04 Mar 2020, Stephen Smalley wrote: > > > > I should note that we are getting similar errors though when > > > > trying > > > > to > > > > specify any context-related > > > > mount options on NFS via the new fsconfig(2) system call, see > > > > https://github.com/SELinuxProject/selinux-kernel/issues/49 > > I've done further testing and found that with this patch the > > fsconfig(2) problem is also resolved for nfs (provided the > > rootcontext > > is not specified). > > Excellent, two bugs fixed with one patch! > > > > > It still needs some further > > > > enhancements as per > > > > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492 > > > > but it at least provides some degree of regression testing. > > Could you describe how I could test these, also are there any other > > SELinux tests that may be useful (with howto's). I'm almost ready > > to > > post another set of RFC test patches, but can add more. > > The ones identified in that github issue comment would simply be > additional tests in tools/nfs.sh > unless they happen to already be covered by your fs/filesystem tests > once those are applied to the > host/native filesystem instead of just ext4. The test cases are: > > 0. Test the bug fixed by this patch, i.e. perform mount of a > security_label exported filesystem, check the label of the mounted > directory to confirm it isn't unlabeled. > That's a NFS-specific test, goes in tools/nfs.sh. > > 1. Mount the same filesystem twice with two different sets of context > mount options, check that mount(2) fails with errno EINVAL. I've tests for the first part already, however with NFS it returns EBUSY (using mount(2) or the fixed fsconfig(2)). On ext4, xfs & vfat it does return EINVAL. I guess another NFS bug. Also mount(8) ignores the error and just carries on. Here is a test using the testsuite mount(2): tools/nfs-mount2.sh #!/bin/sh -e MOUNT=`stat --print %m .` TESTDIR=`pwd` NET="nfsvers=4.2,proto=tcp,clientaddr=127.0.0.1,addr=127.0.0.1" function err_exit() { echo "Error on line: $1 - Closing down NFS" umount /mnt/selinux-testsuite exportfs -u localhost:$MOUNT rmdir /mnt/selinux-testsuite systemctl stop nfs-server exit 1 } trap 'err_exit $LINENO' ERR systemctl start nfs-server exportfs -orw,no_root_squash,security_label localhost:$MOUNT mkdir -p /mnt/selinux-testsuite tests/filesystem/mount -v -f nfs -o vers=4.2,$NET -s localhost:$TESTDIR -t /mnt/selinux-testsuite tests/filesystem/mount -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s localhost:$TESTDIR -t /mnt/selinux-testsuite echo "Testing context mount of a security_label export." fctx=`secon -t -f /mnt/selinux-testsuite` if [ "$fctx" != "etc_t" ]; then echo "Context mount failed: got $fctx instead of etc_t." err_exit $LINENO fi umount /mnt/selinux-testsuite echo "Done" exportfs -u localhost:$MOUNT rmdir /mnt/selinux-testsuite systemctl stop nfs-server > Test cases might include a) mount first without any context mount > options, then try mounting a 2nd time with context mount options and > vice versa, b) mounting > with the same set of context options (e.g. both using context=) but > different context values, c) mounting with different sets of context > options (e.g. one uses > context=, the other uses fscontext=). This test could be done in > fs/filesystem for any filesystem type, not NFS-specific. > > 2. Mount a security_label exported NFS filesystem twice, confirm that > NFS security labeling support isn't silently disabled by trying to > set a label on a file and confirm it is set (fixed by kernel commit > 3815a245b50124f0865415dcb606a034e97494d4). This would go in > tools/nfs.sh > since it is NFS-specific. > > 3. Perform a context= mount of a security_label exported NFS > filesystem, check that pre-existing files within the mount show up > with the context= value > not the underlying xattr value (fixed by kernel commit > 0b4d3452b8b4a5309b4445b900e3cec022cca95a). My original version of > tools/nfs.sh actually would have caught this because it was testing > the context of the nfs.sh script file itself within the context mount > but I dropped it back to only checking the top-level mount directory > when I moved tools/nfs.sh to avoid depending on a fixed location for > it, so it won't be caught currently. We could just change it back to > testing the context of a pre-existing file within the mount; any file > will do. This would go in tools/nfs.sh, NFS-specific. > > 4. Ensuring that all of the tests/filesystem and tests/fs_filesystem > tests that make sense for NFS are being run on the labeled NFS mount > itself when run via tools/nfs.sh and not just on an ext4 mount > created > by the test script.
On Mon, Mar 9, 2020 at 12:41 PM Richard Haines <richard_c_haines@btinternet.com> wrote: > > On Mon, 2020-03-09 at 09:35 -0400, Stephen Smalley wrote: > > 1. Mount the same filesystem twice with two different sets of context > > mount options, check that mount(2) fails with errno EINVAL. > > I've tests for the first part already, however with NFS it returns > EBUSY (using mount(2) or the fixed fsconfig(2)). On ext4, xfs & vfat it > does return EINVAL. I guess another NFS bug. Also mount(8) ignores the > error and just carries on. Here is a test using the testsuite mount(2): Looks like selinux_cmp_sb_context() returns -EBUSY on error instead of -EINVAL. This goes back to 094f7b69ea738d7d619cba449d2af97159949459 ("selinux: make security_sb_clone_mnt_opts return an error on context mismatch"). I guess you can just make the test accept either -EINVAL or -EBUSY for the time being and we'll have to consider whether we want to change it and what would break if we did.
On Mon, 2020-03-09 at 09:35 -0400, Stephen Smalley wrote: > On Sun, Mar 8, 2020 at 1:47 PM Richard Haines > <richard_c_haines@btinternet.com> wrote: > > On Fri, 2020-03-06 at 17:01 -0500, Scott Mayhew wrote: > > > On Wed, 04 Mar 2020, Stephen Smalley wrote: > > > > I should note that we are getting similar errors though when > > > > trying > > > > to > > > > specify any context-related > > > > mount options on NFS via the new fsconfig(2) system call, see > > > > https://github.com/SELinuxProject/selinux-kernel/issues/49 > > I've done further testing and found that with this patch the > > fsconfig(2) problem is also resolved for nfs (provided the > > rootcontext > > is not specified). > > Excellent, two bugs fixed with one patch! > > > > > It still needs some further > > > > enhancements as per > > > > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492 > > > > but it at least provides some degree of regression testing. > > Could you describe how I could test these, also are there any other > > SELinux tests that may be useful (with howto's). I'm almost ready > > to > > post another set of RFC test patches, but can add more. > > The ones identified in that github issue comment would simply be > additional tests in tools/nfs.sh > unless they happen to already be covered by your fs/filesystem tests > once those are applied to the > host/native filesystem instead of just ext4. The test cases are: > > 0. Test the bug fixed by this patch, i.e. perform mount of a > security_label exported filesystem, check the label of the mounted > directory to confirm it isn't unlabeled. > That's a NFS-specific test, goes in tools/nfs.sh. > > 1. Mount the same filesystem twice with two different sets of context > mount options, check that mount(2) fails with errno EINVAL. > Test cases might include a) mount first without any context mount > options, then try mounting a 2nd time with context mount options and > vice versa, b) mounting > with the same set of context options (e.g. both using context=) but > different context values, c) mounting with different sets of context > options (e.g. one uses > context=, the other uses fscontext=). This test could be done in > fs/filesystem for any filesystem type, not NFS-specific. > > 2. Mount a security_label exported NFS filesystem twice, confirm that > NFS security labeling support isn't silently disabled by trying to > set a label on a file and confirm it is set (fixed by kernel commit > 3815a245b50124f0865415dcb606a034e97494d4). This would go in > tools/nfs.sh > since it is NFS-specific. And another one. If you run the same mount twice using mount(2) you get EBUSY. If you run with fsmount(2) it works. A simple test below, just set $1 to fs for fsmount(2) Otherwise I've completed the remaining tests with no problems. #!/bin/sh -e MOUNT=`stat --print %m .` TESTDIR=`pwd` NET="nfsvers=4.2,proto=tcp,clientaddr=127.0.0.1,addr=127.0.0.1" function err_exit() { echo "Error on line: $1 - Closing down NFS" umount /mnt/selinux-testsuite exportfs -u localhost:$MOUNT rmdir /mnt/selinux-testsuite systemctl stop nfs-server exit 1 } trap 'err_exit $LINENO' ERR systemctl start nfs-server exportfs -orw,no_root_squash,security_label localhost:$MOUNT mkdir -p /mnt/selinux-testsuite if [ $1 ] && [ $1 = 'fs' ]; then RUN="tests/fs_filesystem/fsmount" else RUN="tests/filesystem/mount" fi $RUN -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s localhost:$TESTDIR -t /mnt/selinux-testsuite $RUN -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s localhost:$TESTDIR -t /mnt/selinux-testsuite echo "Testing context mount of a security_label export." fctx=`secon -t -f /mnt/selinux-testsuite` if [ "$fctx" != "etc_t" ]; then echo "Context mount failed: got $fctx instead of etc_t." err_exit $LINENO fi umount /mnt/selinux-testsuite umount /mnt/selinux-testsuite echo "Done" exportfs -u localhost:$MOUNT rmdir /mnt/selinux-testsuite systemctl stop nfs-server > > 3. Perform a context= mount of a security_label exported NFS > filesystem, check that pre-existing files within the mount show up > with the context= value > not the underlying xattr value (fixed by kernel commit > 0b4d3452b8b4a5309b4445b900e3cec022cca95a). My original version of > tools/nfs.sh actually would have caught this because it was testing > the context of the nfs.sh script file itself within the context mount > but I dropped it back to only checking the top-level mount directory > when I moved tools/nfs.sh to avoid depending on a fixed location for > it, so it won't be caught currently. We could just change it back to > testing the context of a pre-existing file within the mount; any file > will do. This would go in tools/nfs.sh, NFS-specific. > > 4. Ensuring that all of the tests/filesystem and tests/fs_filesystem > tests that make sense for NFS are being run on the labeled NFS mount > itself when run via tools/nfs.sh and not just on an ext4 mount > created > by the test script.
On Tue, Mar 10, 2020 at 9:27 AM Richard Haines <richard_c_haines@btinternet.com> wrote: > > On Mon, 2020-03-09 at 09:35 -0400, Stephen Smalley wrote: > > 2. Mount a security_label exported NFS filesystem twice, confirm that > > NFS security labeling support isn't silently disabled by trying to > > set a label on a file and confirm it is set (fixed by kernel commit > > 3815a245b50124f0865415dcb606a034e97494d4). This would go in > > tools/nfs.sh > > since it is NFS-specific. > > And another one. If you run the same mount twice using mount(2) you get > EBUSY. If you run with fsmount(2) it works. A simple test below, just > set $1 to fs for fsmount(2) I don't know if that's a bug or just an inconsistency between mount(2) and fsmount(2). Question for David, Al, and/or fsdevel (cc'd). > > Otherwise I've completed the remaining tests with no problems. > > #!/bin/sh -e > MOUNT=`stat --print %m .` > TESTDIR=`pwd` > NET="nfsvers=4.2,proto=tcp,clientaddr=127.0.0.1,addr=127.0.0.1" > > function err_exit() { > echo "Error on line: $1 - Closing down NFS" > umount /mnt/selinux-testsuite > exportfs -u localhost:$MOUNT > rmdir /mnt/selinux-testsuite > systemctl stop nfs-server > exit 1 > } > > trap 'err_exit $LINENO' ERR > > systemctl start nfs-server > exportfs -orw,no_root_squash,security_label localhost:$MOUNT > mkdir -p /mnt/selinux-testsuite > > if [ $1 ] && [ $1 = 'fs' ]; then > RUN="tests/fs_filesystem/fsmount" > else > RUN="tests/filesystem/mount" > fi > > $RUN -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s > localhost:$TESTDIR -t /mnt/selinux-testsuite > $RUN -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s > localhost:$TESTDIR -t /mnt/selinux-testsuite > echo "Testing context mount of a security_label export." > fctx=`secon -t -f /mnt/selinux-testsuite` > if [ "$fctx" != "etc_t" ]; then > echo "Context mount failed: got $fctx instead of etc_t." > err_exit $LINENO > fi > umount /mnt/selinux-testsuite > umount /mnt/selinux-testsuite > > echo "Done" > exportfs -u localhost:$MOUNT > rmdir /mnt/selinux-testsuite > systemctl stop nfs-server
On Tue, Mar 3, 2020 at 5:59 PM Scott Mayhew <smayhew@redhat.com> wrote: > > When using NFSv4.2, the security label for the root inode should be set > via a call to nfs_setsecurity() during the mount process, otherwise the > inode will appear as unlabeled for up to acdirmin seconds. Currently > the label for the root inode is allocated, retrieved, and freed entirely > witin nfs4_proc_get_root(). > > Add a field for the label to the nfs_fattr struct, and allocate & free > the label in nfs_get_root(), where we also add a call to > nfs_setsecurity(). Note that for the call to nfs_setsecurity() to > succeed, it's necessary to also move the logic calling > security_sb_{set,clone}_security() from nfs_get_tree_common() down into > nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in the > super_block's security flags and nfs_setsecurity() will silently fail. > > Reported-by: Richard Haines <richard_c_haines@btinternet.com> > Signed-off-by: Scott Mayhew <smayhew@redhat.com> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> Tested-by: Stephen Smalley <sds@tycho.nsa.gov>
On Tue, Mar 10, 2020 at 11:53 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Tue, Mar 3, 2020 at 5:59 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > When using NFSv4.2, the security label for the root inode should be set > > via a call to nfs_setsecurity() during the mount process, otherwise the > > inode will appear as unlabeled for up to acdirmin seconds. Currently > > the label for the root inode is allocated, retrieved, and freed entirely > > witin nfs4_proc_get_root(). > > > > Add a field for the label to the nfs_fattr struct, and allocate & free > > the label in nfs_get_root(), where we also add a call to > > nfs_setsecurity(). Note that for the call to nfs_setsecurity() to > > succeed, it's necessary to also move the logic calling > > security_sb_{set,clone}_security() from nfs_get_tree_common() down into > > nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in the > > super_block's security flags and nfs_setsecurity() will silently fail. > > > > Reported-by: Richard Haines <richard_c_haines@btinternet.com> > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > Tested-by: Stephen Smalley <sds@tycho.nsa.gov> This all looks reasonable to me so I've merged it into selinux/next (with some minor line width fixes); I was hoping some of the NFS guys would lend an ACK but it has been well over a week with no objections so I figure it is fair game. Thanks for the patch, testing, and review everyone!
diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c index b012c2668a1f..6d0628149390 100644 --- a/fs/nfs/getroot.c +++ b/fs/nfs/getroot.c @@ -73,6 +73,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) struct inode *inode; char *name; int error = -ENOMEM; + unsigned long kflags = 0, kflags_out = 0; name = kstrdup(fc->source, GFP_KERNEL); if (!name) @@ -83,11 +84,14 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) if (fsinfo.fattr == NULL) goto out_name; + fsinfo.fattr->label = nfs4_label_alloc(server, GFP_KERNEL); + if (IS_ERR(fsinfo.fattr->label)) + goto out_fattr; error = server->nfs_client->rpc_ops->getroot(server, ctx->mntfh, &fsinfo); if (error < 0) { dprintk("nfs_get_root: getattr error = %d\n", -error); nfs_errorf(fc, "NFS: Couldn't getattr on root"); - goto out_fattr; + goto out_label; } inode = nfs_fhget(s, ctx->mntfh, fsinfo.fattr, NULL); @@ -95,12 +99,12 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) dprintk("nfs_get_root: get root inode failed\n"); error = PTR_ERR(inode); nfs_errorf(fc, "NFS: Couldn't get root inode"); - goto out_fattr; + goto out_label; } error = nfs_superblock_set_dummy_root(s, inode); if (error != 0) - goto out_fattr; + goto out_label; /* root dentries normally start off anonymous and get spliced in later * if the dentry tree reaches them; however if the dentry already @@ -111,7 +115,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) dprintk("nfs_get_root: get root dentry failed\n"); error = PTR_ERR(root); nfs_errorf(fc, "NFS: Couldn't get root dentry"); - goto out_fattr; + goto out_label; } security_d_instantiate(root, inode); @@ -123,12 +127,39 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc) } spin_unlock(&root->d_lock); fc->root = root; + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL) + kflags |= SECURITY_LSM_NATIVE_LABELS; + if (ctx->clone_data.sb) { + if (d_inode(fc->root)->i_fop != &nfs_dir_operations) { + error = -ESTALE; + goto error_splat_root; + } + /* clone any lsm security options from the parent to the new sb */ + error = security_sb_clone_mnt_opts(ctx->clone_data.sb, s, kflags, + &kflags_out); + } else { + error = security_sb_set_mnt_opts(s, fc->security, + kflags, &kflags_out); + } + if (error) + goto error_splat_root; + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && + !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) + NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL; + + nfs_setsecurity(inode, fsinfo.fattr, fsinfo.fattr->label); error = 0; +out_label: + nfs4_label_free(fsinfo.fattr->label); out_fattr: nfs_free_fattr(fsinfo.fattr); out_name: kfree(name); out: return error; +error_splat_root: + dput(fc->root); + fc->root = NULL; + goto out_label; } diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 69b7ab7a5815..cb34e840e4fb 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -4002,7 +4002,7 @@ static int nfs4_proc_get_root(struct nfs_server *server, struct nfs_fh *mntfh, { int error; struct nfs_fattr *fattr = info->fattr; - struct nfs4_label *label = NULL; + struct nfs4_label *label = fattr->label; error = nfs4_server_capabilities(server, mntfh); if (error < 0) { @@ -4010,23 +4010,17 @@ static int nfs4_proc_get_root(struct nfs_server *server, struct nfs_fh *mntfh, return error; } - label = nfs4_label_alloc(server, GFP_KERNEL); - if (IS_ERR(label)) - return PTR_ERR(label); - error = nfs4_proc_getattr(server, mntfh, fattr, label, NULL); if (error < 0) { dprintk("nfs4_get_root: getattr error = %d\n", -error); - goto err_free_label; + goto out; } if (fattr->valid & NFS_ATTR_FATTR_FSID && !nfs_fsid_equal(&server->fsid, &fattr->fsid)) memcpy(&server->fsid, &fattr->fsid, sizeof(server->fsid)); -err_free_label: - nfs4_label_free(label); - +out: return error; } diff --git a/fs/nfs/super.c b/fs/nfs/super.c index dada09b391c6..bb14bede6da5 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -1179,7 +1179,6 @@ int nfs_get_tree_common(struct fs_context *fc) struct super_block *s; int (*compare_super)(struct super_block *, struct fs_context *) = nfs_compare_super; struct nfs_server *server = ctx->server; - unsigned long kflags = 0, kflags_out = 0; int error; ctx->server = NULL; @@ -1239,26 +1238,6 @@ int nfs_get_tree_common(struct fs_context *fc) goto error_splat_super; } - if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL) - kflags |= SECURITY_LSM_NATIVE_LABELS; - if (ctx->clone_data.sb) { - if (d_inode(fc->root)->i_fop != &nfs_dir_operations) { - error = -ESTALE; - goto error_splat_root; - } - /* clone any lsm security options from the parent to the new sb */ - error = security_sb_clone_mnt_opts(ctx->clone_data.sb, s, kflags, - &kflags_out); - } else { - error = security_sb_set_mnt_opts(s, fc->security, - kflags, &kflags_out); - } - if (error) - goto error_splat_root; - if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && - !(kflags_out & SECURITY_LSM_NATIVE_LABELS)) - NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL; - s->s_flags |= SB_ACTIVE; error = 0; @@ -1268,10 +1247,6 @@ int nfs_get_tree_common(struct fs_context *fc) out_err_nosb: nfs_free_server(server); goto out; - -error_splat_root: - dput(fc->root); - fc->root = NULL; error_splat_super: deactivate_locked_super(s); goto out; diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h index 94c77ed55ce1..6838c149f335 100644 --- a/include/linux/nfs_xdr.h +++ b/include/linux/nfs_xdr.h @@ -75,6 +75,7 @@ struct nfs_fattr { struct nfs4_string *owner_name; struct nfs4_string *group_name; struct nfs4_threshold *mdsthreshold; /* pNFS threshold hints */ + struct nfs4_label *label; }; #define NFS_ATTR_FATTR_TYPE (1U << 0)
When using NFSv4.2, the security label for the root inode should be set via a call to nfs_setsecurity() during the mount process, otherwise the inode will appear as unlabeled for up to acdirmin seconds. Currently the label for the root inode is allocated, retrieved, and freed entirely witin nfs4_proc_get_root(). Add a field for the label to the nfs_fattr struct, and allocate & free the label in nfs_get_root(), where we also add a call to nfs_setsecurity(). Note that for the call to nfs_setsecurity() to succeed, it's necessary to also move the logic calling security_sb_{set,clone}_security() from nfs_get_tree_common() down into nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in the super_block's security flags and nfs_setsecurity() will silently fail. Reported-by: Richard Haines <richard_c_haines@btinternet.com> Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- fs/nfs/getroot.c | 39 +++++++++++++++++++++++++++++++++++---- fs/nfs/nfs4proc.c | 12 +++--------- fs/nfs/super.c | 25 ------------------------- include/linux/nfs_xdr.h | 1 + 4 files changed, 39 insertions(+), 38 deletions(-)