| Message ID | 20230808-master-v9-1-e0ecde888221@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v9] vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing | expand |
On Tue, 08 Aug 2023 07:34:20 -0400, Jeff Layton wrote: > 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: > > [...] I'm stuffing this on vfs.misc because this should be in -next for some time. If there's objections let me know. --- Applied to the vfs.misc branch of the vfs/vfs.git tree. Patches in the vfs.misc branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.misc [1/1] vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing https://git.kernel.org/vfs/vfs/c/4b4fb74b1aa1
On Tue, 2023-08-08 at 15:31 +0200, Christian Brauner wrote: > On Tue, Aug 08, 2023 at 07:34:20AM -0400, Jeff Layton wrote: > > 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> I've made a significant number of changes since Casey acked this. It might be a good idea to drop his Acked-by (unless he wants to chime in and ask us to keep it). Thanks, Jeff > > Acked-by: "Christian Brauner (Microsoft)" <brauner@kernel.org> > > Link: https://lore.kernel.org/r/165962680944.3334508.6610023900349142034.stgit@warthog.procyon.org.uk/ # v1 > > Link: https://lore.kernel.org/r/165962729225.3357250.14350728846471527137.stgit@warthog.procyon.org.uk/ # v2 > > Link: https://lore.kernel.org/r/165970659095.2812394.6868894171102318796.stgit@warthog.procyon.org.uk/ # v3 > > Link: https://lore.kernel.org/r/166133579016.3678898.6283195019480567275.stgit@warthog.procyon.org.uk/ # v4 > > Link: https://lore.kernel.org/r/217595.1662033775@warthog.procyon.org.uk/ # 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 > > > > ver #7) > > - Drop lsm_set boolean > > - Link to v7: https://lore.kernel.org/r/20230804-master-v7-1-5d4e48407298@kernel.org > > > > 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 > > - Link to v8: https://lore.kernel.org/r/20230807-master-v8-1-54e249595f10@kernel.org > > > > ver #9) > > - rename *_fs_context_init to *_fs_context_submount > > - remove checks for FS_CONTEXT_FOR_SUBMOUNT and NULL reference pointers > > - fix prototype on smack_fs_context_submount > > Thanks, this looks good from my perspective. If it looks fine to LSM > folks as well I can put it with the rest of the super work for this > cycle or it can go through the LSM tree.
On 8/10/2023 6:57 AM, Jeff Layton wrote: > On Tue, 2023-08-08 at 15:31 +0200, Christian Brauner wrote: >> On Tue, Aug 08, 2023 at 07:34:20AM -0400, Jeff Layton wrote: >>> 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> > I've made a significant number of changes since Casey acked this. It > might be a good idea to drop his Acked-by (unless he wants to chime in > and ask us to keep it). You can keep the Acked-by. > > Thanks, > Jeff > >>> Acked-by: "Christian Brauner (Microsoft)" <brauner@kernel.org> >>> Link: https://lore.kernel.org/r/165962680944.3334508.6610023900349142034.stgit@warthog.procyon.org.uk/ # v1 >>> Link: https://lore.kernel.org/r/165962729225.3357250.14350728846471527137.stgit@warthog.procyon.org.uk/ # v2 >>> Link: https://lore.kernel.org/r/165970659095.2812394.6868894171102318796.stgit@warthog.procyon.org.uk/ # v3 >>> Link: https://lore.kernel.org/r/166133579016.3678898.6283195019480567275.stgit@warthog.procyon.org.uk/ # v4 >>> Link: https://lore.kernel.org/r/217595.1662033775@warthog.procyon.org.uk/ # 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 >>> >>> ver #7) >>> - Drop lsm_set boolean >>> - Link to v7: https://lore.kernel.org/r/20230804-master-v7-1-5d4e48407298@kernel.org >>> >>> 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 >>> - Link to v8: https://lore.kernel.org/r/20230807-master-v8-1-54e249595f10@kernel.org >>> >>> ver #9) >>> - rename *_fs_context_init to *_fs_context_submount >>> - remove checks for FS_CONTEXT_FOR_SUBMOUNT and NULL reference pointers >>> - fix prototype on smack_fs_context_submount >> Thanks, this looks good from my perspective. If it looks fine to LSM >> folks as well I can put it with the rest of the super work for this >> cycle or it can go through the LSM tree.
On Thu, Aug 10, 2023 at 9:57 AM Jeff Layton <jlayton@kernel.org> wrote: > On Tue, 2023-08-08 at 15:31 +0200, Christian Brauner wrote: > > On Tue, Aug 08, 2023 at 07:34:20AM -0400, Jeff Layton wrote: > > > 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> > > I've made a significant number of changes since Casey acked this. It > might be a good idea to drop his Acked-by (unless he wants to chime in > and ask us to keep it). My apologies in that it took me some time to be able to come back to this, but v9 looks fine to me, and I have no problems with Christian sending this up via the VFS tree. Acked-by: Paul Moore <paul@paul-moore.com>
diff --git a/fs/fs_context.c b/fs/fs_context.c index 851214d1d013..375023e40161 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_submount(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..af796986baee 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_submount, 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..bac98ea18f78 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_submount(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_submount(struct fs_context *fc, + struct super_block *reference) +{ + return 0; +} 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..549104a447e3 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_submount() - 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_submount(struct fs_context *fc, struct super_block *reference) +{ + return call_int_hook(fs_context_submount, 0, fc, reference); +} + /** * security_fs_context_dup() - Duplicate a fs_context LSM blob * @fc: destination filesystem context diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d06e350fedee..afd663744041 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2745,6 +2745,27 @@ static int selinux_umount(struct vfsmount *mnt, int flags) FILESYSTEM__UNMOUNT, NULL); } +static int selinux_fs_context_submount(struct fs_context *fc, + struct super_block *reference) +{ + const struct superblock_security_struct *sbsec; + struct selinux_mnt_opts *opts; + + opts = kzalloc(sizeof(*opts), GFP_KERNEL); + if (!opts) + return -ENOMEM; + + sbsec = selinux_superblock(reference); + if (sbsec->flags & FSCONTEXT_MNT) + opts->fscontext_sid = sbsec->sid; + if (sbsec->flags & CONTEXT_MNT) + opts->context_sid = sbsec->mntpoint_sid; + if (sbsec->flags & DEFCONTEXT_MNT) + opts->defcontext_sid = sbsec->def_sid; + fc->security = opts; + return 0; +} + static int selinux_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc) { @@ -7182,6 +7203,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = { /* * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE */ + LSM_HOOK_INIT(fs_context_submount, selinux_fs_context_submount), LSM_HOOK_INIT(fs_context_dup, selinux_fs_context_dup), LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param), LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts), diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 6e270cf3fd30..a8201cf22f20 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -614,6 +614,56 @@ static int smack_add_opt(int token, const char *s, void **mnt_opts) return -EINVAL; } +/** + * smack_fs_context_submount - Initialise security data for a filesystem context + * @fc: The filesystem context. + * @reference: reference superblock + * + * Returns 0 on success or -ENOMEM on error. + */ +static int smack_fs_context_submount(struct fs_context *fc, + struct super_block *reference) +{ + struct superblock_smack *sbsp; + struct smack_mnt_opts *ctx; + struct inode_smack *isp; + + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + fc->security = ctx; + + sbsp = smack_superblock(reference); + isp = smack_inode(reference->s_root->d_inode); + + if (sbsp->smk_default) { + ctx->fsdefault = kstrdup(sbsp->smk_default->smk_known, GFP_KERNEL); + if (!ctx->fsdefault) + return -ENOMEM; + } + + if (sbsp->smk_floor) { + ctx->fsfloor = kstrdup(sbsp->smk_floor->smk_known, GFP_KERNEL); + if (!ctx->fsfloor) + return -ENOMEM; + } + + if (sbsp->smk_hat) { + ctx->fshat = kstrdup(sbsp->smk_hat->smk_known, GFP_KERNEL); + if (!ctx->fshat) + return -ENOMEM; + } + + if (isp->smk_flags & SMK_INODE_TRANSMUTE) { + if (sbsp->smk_root) { + ctx->fstransmute = kstrdup(sbsp->smk_root->smk_known, GFP_KERNEL); + if (!ctx->fstransmute) + return -ENOMEM; + } + } + return 0; +} + /** * smack_fs_context_dup - Duplicate the security data on fs_context duplication * @fc: The new filesystem context. @@ -4876,6 +4926,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = { LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme), LSM_HOOK_INIT(syslog, smack_syslog), + LSM_HOOK_INIT(fs_context_submount, smack_fs_context_submount), LSM_HOOK_INIT(fs_context_dup, smack_fs_context_dup), LSM_HOOK_INIT(fs_context_parse_param, smack_fs_context_parse_param),