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@xxxxxxxxxxxxxx>
> > Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> > ---
> > 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)
>
