Re: [PATCH v8] vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing
From: Christian Brauner <brauner@kernel.org>
Date: 2023-08-07 13:38:41
Also in:
linux-fsdevel, linux-nfs, lkml, selinux
On Mon, Aug 07, 2023 at 09:18:01AM -0400, Jeff Layton wrote:
quoted hunk ↗ jump to hunk
From: David Howells <dhowells@redhat.com> When NFS superblocks are created by automounting, their LSM parameters aren't set in the fs_context struct prior to sget_fc() being called, leading to failure to match existing superblocks. This bug leads to messages like the following appearing in dmesg when fscache is enabled: NFS: Cache volume key already in use (nfs,4.2,2,108,106a8c0,1,,,,100000,100000,2ee,3a98,1d4c,3a98,1) Fix this by adding a new LSM hook to load fc->security for submount creation. Signed-off-by: David Howells <dhowells@redhat.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> Fixes: 9bc61ab18b1d ("vfs: Introduce fs_context, switch vfs_kern_mount() to it.") Fixes: 779df6a5480f ("NFS: Ensure security label is set for root inode) Tested-by: Jeff Layton <jlayton@kernel.org> Reviewed-by: Jeff Layton <jlayton@kernel.org> Acked-by: Casey Schaufler <casey@schaufler-ca.com> Acked-by: "Christian Brauner (Microsoft)" <brauner@kernel.org> Link: https://lore.kernel.org/r/165962680944.3334508.6610023900349142034.stgit@warthog.procyon.org.uk/ (local) # v1 Link: https://lore.kernel.org/r/165962729225.3357250.14350728846471527137.stgit@warthog.procyon.org.uk/ (local) # v2 Link: https://lore.kernel.org/r/165970659095.2812394.6868894171102318796.stgit@warthog.procyon.org.uk/ (local) # v3 Link: https://lore.kernel.org/r/166133579016.3678898.6283195019480567275.stgit@warthog.procyon.org.uk/ (local) # v4 Link: https://lore.kernel.org/r/217595.1662033775@warthog.procyon.org.uk/ (local) # v5 --- ver #2) - Added Smack support - Made LSM parameter extraction dependent on reference != NULL. ver #3) - Made LSM parameter extraction dependent on fc->purpose == FS_CONTEXT_FOR_SUBMOUNT. Shouldn't happen on FOR_RECONFIGURE. ver #4) - When doing a FOR_SUBMOUNT mount, don't set the root label in SELinux or Smack. ver #5) - Removed unused variable. - Only allocate smack_mnt_opts if we're dealing with a submount. ver #6) - Rebase onto v6.5.0-rc4 - Link to v6: https://lore.kernel.org/r/20230802-master-v6-1-45d48299168b@kernel.org (local) ver #7) - Drop lsm_set boolean - Link to v7: https://lore.kernel.org/r/20230804-master-v7-1-5d4e48407298@kernel.org (local) ver #8) - Remove spurious semicolon in smack_fs_context_init - Make fs_context_init take a superblock as reference instead of dentry - WARN_ON_ONCE's when fc->purpose != FS_CONTEXT_FOR_SUBMOUNT - Call the security hook from fs_context_for_submount instead of alloc_fs_context --- fs/fs_context.c | 23 +++++++++++++++++- include/linux/lsm_hook_defs.h | 1 + include/linux/security.h | 6 +++++ security/security.c | 14 +++++++++++ security/selinux/hooks.c | 25 ++++++++++++++++++++ security/smack/smack_lsm.c | 54 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 122 insertions(+), 1 deletion(-)diff --git a/fs/fs_context.c b/fs/fs_context.c index 851214d1d013..a76d7c82e091 100644 --- a/fs/fs_context.c +++ b/fs/fs_context.c@@ -315,10 +315,31 @@ struct fs_context *fs_context_for_reconfigure(struct dentry *dentry, } EXPORT_SYMBOL(fs_context_for_reconfigure); +/** + * fs_context_for_submount: allocate a new fs_context for a submount + * @type: file_system_type of the new context + * @reference: reference dentry from which to copy relevant info + * + * Allocate a new fs_context suitable for a submount. This also ensures that + * the fc->security object is inherited from @reference (if needed). + */ struct fs_context *fs_context_for_submount(struct file_system_type *type, struct dentry *reference) { - return alloc_fs_context(type, reference, 0, 0, FS_CONTEXT_FOR_SUBMOUNT); + struct fs_context *fc; + int ret; + + fc = alloc_fs_context(type, reference, 0, 0, FS_CONTEXT_FOR_SUBMOUNT); + if (IS_ERR(fc)) + return fc; + + ret = security_fs_context_init(fc, reference->d_sb); + if (ret) { + put_fs_context(fc); + return ERR_PTR(ret); + } + + return fc; } EXPORT_SYMBOL(fs_context_for_submount);diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 7308a1a7599b..2876dd6114c0 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h@@ -54,6 +54,7 @@ LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, struct file *f LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, struct linux_binprm *bprm) +LSM_HOOK(int, 0, fs_context_init, struct fs_context *fc, struct super_block *reference) LSM_HOOK(int, 0, fs_context_dup, struct fs_context *fc, struct fs_context *src_sc) LSM_HOOK(int, -ENOPARAM, fs_context_parse_param, struct fs_context *fc,diff --git a/include/linux/security.h b/include/linux/security.h index 32828502f09e..fe9bf5e805ee 100644 --- a/include/linux/security.h +++ b/include/linux/security.h@@ -293,6 +293,7 @@ int security_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file); int security_bprm_check(struct linux_binprm *bprm); void security_bprm_committing_creds(struct linux_binprm *bprm); void security_bprm_committed_creds(struct linux_binprm *bprm); +int security_fs_context_init(struct fs_context *fc, struct super_block *reference); int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc); int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param); int security_sb_alloc(struct super_block *sb);@@ -629,6 +630,11 @@ static inline void security_bprm_committed_creds(struct linux_binprm *bprm) { } +static inline int security_fs_context_init(struct fs_context *fc, + struct super_block *reference) +{ + return 0; +}
Sorry, my point is we shouldn't be adding a generic security_fs_context_init() hook at all. Pre superblock creation we have a hook during parameter parsing for LSMs and another one during actual superblock creation in vfs_get_tree() and yet another one for fs_context duplicaton. We don't need another generic one during fs_context allocation. Yes, we may need a hook for submount allocation but then we'll add one exactly for that. And then for fs_context_for_submount @sb can't be empty so there's also no point in checking whether it is empty because you've already crashed in fs_context_for_submount(). All the checks below for !reference and fc->purpose != FS_CONTEXT_FOR_SUBMOUNT can go away then as well. So we end up with something easier and stricter.
quoted hunk ↗ jump to hunk
static inline int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc) {diff --git a/security/security.c b/security/security.c index b720424ca37d..f8a666d089f9 100644 --- a/security/security.c +++ b/security/security.c@@ -1138,6 +1138,20 @@ void security_bprm_committed_creds(struct linux_binprm *bprm) call_void_hook(bprm_committed_creds, bprm); } +/** + * security_fs_context_init() - Initialise fc->security + * @fc: new filesystem context + * @reference: dentry reference for submount/remount + * + * Fill out the ->security field for a new fs_context. + * + * Return: Returns 0 on success or negative error code on failure. + */ +int security_fs_context_init(struct fs_context *fc, struct super_block *reference) +{ + return call_int_hook(fs_context_init, 0, fc, reference); +} + /** * security_fs_context_dup() - Duplicate a fs_context LSM blob * @fc: destination filesystem contextdiff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d06e350fedee..c8fb0d77104f 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c@@ -2745,6 +2745,30 @@ static int selinux_umount(struct vfsmount *mnt, int flags) FILESYSTEM__UNMOUNT, NULL); } +static int selinux_fs_context_init(struct fs_context *fc, + struct super_block *reference) +{ + const struct superblock_security_struct *sbsec; + struct selinux_mnt_opts *opts; + + if (!reference || WARN_ON_ONCE(fc->purpose != FS_CONTEXT_FOR_SUBMOUNT)) + return 0;