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 <smayhew@xxxxxxxxxx>
> > > > > ---
> > > > > 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.
