| Message ID | 20170525210754.24265-3-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@redhat.com> > --- > 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?
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@redhat.com> > > --- > > 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. -Scott
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@redhat.com> > > > --- > > > 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? Could we instead extend the security_sb_clone_mnt_opts() hook interface to pass flags, and just handle it inside of the selinux_sb_clone_mnt_opts() hook? Then we don't have to change selinux_sb_set_mnt_opts() handling, which may be called for mount(2) from userspace, not just from NFS client code. You might need to refactor it to share common code, but we shouldn't alter its logic when called normally. > Testing it again now, I'm not sure the context= mount option is > working > correctly with the latest kernel. Hmm...if not, then that's another regression that will need to be addressed.
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@redhat.com> > > > --- > > > 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
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@redhat.com> > > > > --- > > > > 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. --b.
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@redhat.com> > > > > > --- > > > > > 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.
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);
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@redhat.com> --- fs/nfs/super.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)