On Thu, 2017-06-01 at 10:46 -0400, Scott Mayhew wrote:
> When an NFSv4 client performs a mount operation, it first mounts the
> NFSv4 root and then does path walk to the exported path and performs
> a
> submount on that, cloning the security mount options from the root's
> superblock to the submount's superblock in the process.
>
> Unless the NFS server has an explicit fsid=0 export with the
> "security_label" option, the NFSv4 root superblock will not have
> SBLABEL_MNT set, and neither will the submount superblock after
> cloning
> the security mount options. As a result, setxattr's of security
> labels
> over NFSv4.2 will fail. In a similar fashion, NFSv4.2 mounts mounted
> with the context= mount option will not show the correct labels
> because
> the nfs_server->caps flags of the cloned superblock will still have
> NFS_CAP_SECURITY_LABEL set.
>
> Allowing the NFSv4 client to enable or disable
> SECURITY_LSM_NATIVE_LABELS
> behavior will ensure that the SBLABEL_MNT flag has the correct value
> when the client traverses from an exported path without the
> "security_label" option to one with the "security_label" option and
> vice versa. Similarly, checking to see if SECURITY_LSM_NATIVE_LABELS
> is
> set upon return from security_sb_clone_mnt_opts() and clearing
> NFS_CAP_SECURITY_LABEL if necessary will allow the correct labels to
> be displayed for NFSv4.2 mounts mounted with the context= mount
> option.
>
> Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> ---
> fs/nfs/super.c | 18 +++++++++++++++++-
> include/linux/lsm_hooks.h | 4 +++-
> include/linux/security.h | 8 ++++++--
> security/security.c | 7 +++++--
> security/selinux/hooks.c | 43
> ++++++++++++++++++++++++++++++++++++++-----
> 5 files changed, 69 insertions(+), 11 deletions(-)
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 2f3822a..6a11535 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -2544,10 +2544,26 @@ 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;
> +
> /* 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);
> +
> + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL)
> + kflags |= SECURITY_LSM_NATIVE_LABELS;
> +
> + error = security_sb_clone_mnt_opts(mount_info->cloned->sb,
> s, kflags, &kflags_out);
> + if (error)
> + goto err;
Not sure this is justified; coding style says to just return directly
if no cleanup is required.
> +
> + if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL &&
> + !(kflags_out & SECURITY_LSM_NATIVE_LABELS))
> + NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL;
> +err:
> + return error;
> +
> }
> EXPORT_SYMBOL_GPL(nfs_clone_sb_security);
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 080f34e..2f54bfb 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1388,7 +1388,9 @@ union security_list_options {
> unsigned long kern_flags,
> unsigned long *set_kern_flags);
> int (*sb_clone_mnt_opts)(const struct super_block *oldsb,
> - struct super_block *newsb);
> + struct super_block *newsb,
> + unsigned long kern_flags,
> + unsigned long
> *set_kern_flags);
> int (*sb_parse_opts_str)(char *options, struct
> security_mnt_opts *opts);
> int (*dentry_init_security)(struct dentry *dentry, int mode,
> const struct qstr *name,
> void **ctx,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index af675b5..a55ae9c 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -240,7 +240,9 @@ int security_sb_set_mnt_opts(struct super_block
> *sb,
> unsigned long kern_flags,
> unsigned long *set_kern_flags);
> int security_sb_clone_mnt_opts(const struct super_block *oldsb,
> - struct super_block *newsb);
> + struct super_block *newsb,
> + unsigned long kern_flags,
> + unsigned long *set_kern_flags);
> int security_sb_parse_opts_str(char *options, struct
> security_mnt_opts *opts);
> int security_dentry_init_security(struct dentry *dentry, int mode,
> const struct qstr *name,
> void **ctx,
> @@ -581,7 +583,9 @@ static inline int security_sb_set_mnt_opts(struct
> super_block *sb,
> }
>
> static inline int security_sb_clone_mnt_opts(const struct
> super_block *oldsb,
> - struct super_block
> *newsb)
> + struct super_block
> *newsb,
> + unsigned long
> kern_flags,
> + unsigned long
> *set_kern_flags)
> {
> return 0;
> }
> diff --git a/security/security.c b/security/security.c
> index b9fea39..7b70ea2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -380,9 +380,12 @@ int security_sb_set_mnt_opts(struct super_block
> *sb,
> EXPORT_SYMBOL(security_sb_set_mnt_opts);
>
> int security_sb_clone_mnt_opts(const struct super_block *oldsb,
> - struct super_block *newsb)
> + struct super_block *newsb,
> + unsigned long kern_flags,
> + unsigned long *set_kern_flags)
> {
> - return call_int_hook(sb_clone_mnt_opts, 0, oldsb, newsb);
> + return call_int_hook(sb_clone_mnt_opts, 0, oldsb, newsb,
> + kern_flags, set_kern_flags);
> }
> EXPORT_SYMBOL(security_sb_clone_mnt_opts);
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e67a526..80d9acf 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -529,8 +529,14 @@ static int sb_finish_set_opts(struct super_block
> *sb)
> sb->s_id, sb->s_type->name);
>
> sbsec->flags |= SE_SBINITIALIZED;
> +
> + /* Explicitly set or clear SBLABEL_MNT. It's not sufficient
> to simply
> + leave the flag untouched because sb_clone_mnt_opts might
> be handing
> + us a superblock that needs the flag to be cleared. */
Coding style (and checkpatch.pl) prefer a different style for multi-
line comments.
> if (selinux_is_sblabel_mnt(sb))
> sbsec->flags |= SBLABEL_MNT;
> + else
> + sbsec->flags &= ~SBLABEL_MNT;
>
> /* Initialize the root inode. */
> rc = inode_doinit_with_dentry(root_inode, root);
> @@ -963,8 +969,11 @@ static int selinux_cmp_sb_context(const struct
> super_block *oldsb,
> }
>
> static int selinux_sb_clone_mnt_opts(const struct super_block
> *oldsb,
> - struct super_block *newsb)
> + struct super_block *newsb,
> + unsigned long kern_flags,
> + unsigned long
> *set_kern_flags)
> {
> + int rc = 0;
> const struct superblock_security_struct *oldsbsec = oldsb-
> >s_security;
> struct superblock_security_struct *newsbsec = newsb-
> >s_security;
>
> @@ -977,14 +986,23 @@ static int selinux_sb_clone_mnt_opts(const
> struct super_block *oldsb,
> * mount options. thus we can safely deal with this
> superblock later
> */
> if (!ss_initialized)
> - return 0;
> + goto out;
Likewise, don't see the point of this since no cleanup is required, and
as per coding style.
> +
> + if (kern_flags && !set_kern_flags) {
> + /* Specifying internal flags without providing a
> place to
> + * place the results is not allowed */
> + rc = -EINVAL;
> + goto out;
Ditto, just return directly.
> + }
>
> /* how can we clone if the old one wasn't set up?? */
> BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
>
> /* if fs is reusing a sb, make sure that the contexts match
> */
> - if (newsbsec->flags & SE_SBINITIALIZED)
> - return selinux_cmp_sb_context(oldsb, newsb);
> + if (newsbsec->flags & SE_SBINITIALIZED) {
> + rc = selinux_cmp_sb_context(oldsb, newsb);
> + goto out;
> + }
And again.
>
> mutex_lock(&newsbsec->lock);
>
> @@ -994,6 +1012,19 @@ static int selinux_sb_clone_mnt_opts(const
> struct super_block *oldsb,
> newsbsec->def_sid = oldsbsec->def_sid;
> newsbsec->behavior = oldsbsec->behavior;
>
> + if (newsbsec->behavior == SECURITY_FS_USE_NATIVE
> + && !(kern_flags &
> SECURITY_LSM_NATIVE_LABELS)
> + && !set_context) {
I prefer ending the prior line with &&, not beginning the next one with
it. Also indentation of the continuation lines seems excessive.
> + rc = security_fs_use(newsb);
> + if (rc)
> + goto out_unlock;
> + }
> +
> + if (kern_flags & SECURITY_LSM_NATIVE_LABELS && !set_context)
> {
> + newsbsec->behavior = SECURITY_FS_USE_NATIVE;
> + *set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
> + }
> +
> if (set_context) {
> u32 sid = oldsbsec->mntpoint_sid;
>
> @@ -1013,8 +1044,10 @@ static int selinux_sb_clone_mnt_opts(const
> struct super_block *oldsb,
> }
>
> sb_finish_set_opts(newsb);
> +out_unlock:
> mutex_unlock(&newsbsec->lock);
> - return 0;
> +out:
> + return rc;
> }
>
> static int selinux_parse_opts_str(char *options,
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
