From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <1496173971.2164.25.camel@tycho.nsa.gov> Subject: Re: [PATCH RFC 2/2] nfs: update labeling behavior on a superblock when submounting From: Stephen Smalley To: "J . Bruce Fields" Cc: Scott Mayhew , linux-nfs@vger.kernel.org, selinux@tycho.nsa.gov, Trond Myklebust Date: Tue, 30 May 2017 15:52:51 -0400 In-Reply-To: <20170530194030.GB9371@fieldses.org> References: <20170404232646.GB24146@parsley.fieldses.org> <20170525210754.24265-1-smayhew@redhat.com> <20170525210754.24265-3-smayhew@redhat.com> <1495808645.12091.10.camel@tycho.nsa.gov> <20170526152821.xgyidg7qebdwbm6f@tonberry.usersys.redhat.com> <1496155125.2164.8.camel@tycho.nsa.gov> <20170530194030.GB9371@fieldses.org> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 List-Id: "Security-Enhanced Linux \(SELinux\) mailing list" List-Post: List-Help: On Tue, 2017-05-30 at 15:40 -0400, J . Bruce Fields wrote: > On Tue, May 30, 2017 at 10:38:45AM -0400, Stephen Smalley wrote: > > On Fri, 2017-05-26 at 11:28 -0400, Scott Mayhew wrote: > > > On Fri, 26 May 2017, Stephen Smalley wrote: > > > > > > > On Thu, 2017-05-25 at 17:07 -0400, Scott Mayhew wrote: > > > > > When the client traverses from filesystem exported without > > > > > the > > > > > "security_label" option to one exported with the > > > > > "security_label" > > > > > option, it needs to pass SECURITY_LSM_NATIVE_LABELS to > > > > > security_sb_set_mnt_opts() so that the new superblock has > > > > > SBLABEL_MNT > > > > > set in its security mount options.  Otherwise, attempts to > > > > > set > > > > > security > > > > > labels via setxattr over NFSv4.2 will fail. > > > > > > > > > > Signed-off-by: Scott Mayhew > > > > > --- > > > > >  fs/nfs/super.c | 23 ++++++++++++++++++++++- > > > > >  1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c > > > > > index 2f3822a..d7a3b89 100644 > > > > > --- a/fs/nfs/super.c > > > > > +++ b/fs/nfs/super.c > > > > > @@ -2544,10 +2544,31 @@ > > > > > EXPORT_SYMBOL_GPL(nfs_set_sb_security); > > > > >  int nfs_clone_sb_security(struct super_block *s, struct > > > > > dentry > > > > > *mntroot, > > > > >     struct nfs_mount_info *mount_info) > > > > >  { > > > > > + int error; > > > > > + unsigned long kflags = 0, kflags_out = 0; > > > > > + struct security_mnt_opts opts; > > > > > + > > > > >   /* clone any lsm security options from the parent to > > > > > the > > > > > new > > > > > sb */ > > > > >   if (d_inode(mntroot)->i_op != NFS_SB(s)->nfs_client- > > > > > > rpc_ops->dir_inode_ops) > > > > > > > > > >   return -ESTALE; > > > > > - return security_sb_clone_mnt_opts(mount_info- > > > > > >cloned- > > > > > > sb, > > > > > > > > > > s); > > > > > + error = security_sb_clone_mnt_opts(mount_info- > > > > > >cloned- > > > > > > sb, > > > > > > > > > > s); > > > > > + if (error) > > > > > + goto err; > > > > > + > > > > > + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL && > > > > > + !(NFS_SB(mount_info->cloned->sb)->caps & > > > > > NFS_CAP_SECURITY_LABEL)) { > > > > > + memset(&opts, 0, sizeof(opts)); > > > > > + kflags |= SECURITY_LSM_NATIVE_LABELS; > > > > > + > > > > > + error = security_sb_set_mnt_opts(s, &opts, > > > > > kflags, > > > > > &kflags_out); > > > > > + if (error) > > > > > + goto err; > > > > > + > > > > > + if (!(kflags_out & > > > > > SECURITY_LSM_NATIVE_LABELS)) > > > > > + NFS_SB(s)->caps &= > > > > > ~NFS_CAP_SECURITY_LABEL; > > > > > + } > > > > > +err: > > > > > + return error; > > > > >  } > > > > >  EXPORT_SYMBOL_GPL(nfs_clone_sb_security); > > > > > > > > Could this clobber a context set via context= mount option? > > > > > > Argh, yes I suppose it could.  In my first attempt to fix this, I > > > added > > > a security_sb_get_mnt_opts() hook to get the original mount > > > options > > > and > > > then passed that along with the SECURITY_LSM_NATIVE_LABELS flag > > > to > > > security_sb_set_mnt_opts().  When I saw that > > > security_sb_set_mnt_opts() > > > wouldn't allow me to change a superblock that had already been > > > initialized, I got rid of the hook and added the check in patch > > > 1... > > > maybe a combination of the two is needed? > > > > > > Testing it again now, I'm not sure the context= mount option is > > > working > > > correctly with the latest kernel. > > > > Looks like you are correct, > > https://github.com/SELinuxProject/selinux-kernel/issues/35 > > Ugh.  So, to make sure I understand: the desired behavior is that in > the > case the client mounts with a context= option, behavior is exactly as > if > the client or server didn't support the new security labeling > protocol. > That would make sense to me. Yes, that's correct. And in theory that is what nfs_set_sb_security() is trying to do by clearing NFS_CAP_SECURITY_LABEL if SECURITY_LSM_NATIVE_LABELS was not set by the security hook.