[v7] vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing

Commit Message

Jeff Layton Aug. 4, 2023, 4:09 p.m. UTC

From: David Howells <[e-mail]>

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 when alloc_fs_context() is creating the fs_context for it.

Signed-off-by: David Howells <[e-mail]>
Signed-off-by: Jeff Layton <[e-mail]>
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 <[e-mail]>
Reviewed-by: Jeff Layton <[e-mail]>
Acked-by: Casey Schaufler <[e-mail]>
Acked-by: "Christian Brauner (Microsoft)" <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]/ # v1
Link: https://lore.kernel.org/r/[e-mail]/ # v2
Link: https://lore.kernel.org/r/[e-mail]/ # v3
Link: https://lore.kernel.org/r/[e-mail]/ # v4
Link: https://lore.kernel.org/r/[e-mail]/ # v5
---
ver #7)
 - Drop lsm_set boolean
 - Link to v6: https://lore.kernel.org/r/[e-mail]

ver #6)
 - Rebase onto v6.5.0-rc4

ver #5)
 - Removed unused variable.
 - Only allocate smack_mnt_opts if we're dealing with a submount.

ver #4)
 - When doing a FOR_SUBMOUNT mount, don't set the root label in SELinux or
   Smack.

ver #3)
 - Made LSM parameter extraction dependent on fc->purpose ==
   FS_CONTEXT_FOR_SUBMOUNT.  Shouldn't happen on FOR_RECONFIGURE.

ver #2)
 - Added Smack support
 - Made LSM parameter extraction dependent on reference != NULL.
---
 fs/fs_context.c               |  4 ++++
 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, 104 insertions(+)


---
base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
change-id: 20230802-master-3082090e8d69

Best regards,

Comments

Christian Brauner Aug. 5, 2023, 12:43 p.m. UTC | #1

On Fri, Aug 04, 2023 at 12:09:34PM -0400, Jeff Layton wrote:
> From: David Howells <[e-mail]>
> 
> 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 when alloc_fs_context() is creating the fs_context for it.
> 
> Signed-off-by: David Howells <[e-mail]>
> Signed-off-by: Jeff Layton <[e-mail]>
> 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 <[e-mail]>
> Reviewed-by: Jeff Layton <[e-mail]>
> Acked-by: Casey Schaufler <[e-mail]>
> Acked-by: "Christian Brauner (Microsoft)" <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]/ # v1
> Link: https://lore.kernel.org/r/[e-mail]/ # v2
> Link: https://lore.kernel.org/r/[e-mail]/ # v3
> Link: https://lore.kernel.org/r/[e-mail]/ # v4
> Link: https://lore.kernel.org/r/[e-mail]/ # v5
> ---
> ver #7)
>  - Drop lsm_set boolean
>  - Link to v6: https://lore.kernel.org/r/[e-mail]
> 
> ver #6)
>  - Rebase onto v6.5.0-rc4
> 
> ver #5)
>  - Removed unused variable.
>  - Only allocate smack_mnt_opts if we're dealing with a submount.
> 
> ver #4)
>  - When doing a FOR_SUBMOUNT mount, don't set the root label in SELinux or
>    Smack.
> 
> ver #3)
>  - Made LSM parameter extraction dependent on fc->purpose ==
>    FS_CONTEXT_FOR_SUBMOUNT.  Shouldn't happen on FOR_RECONFIGURE.
> 
> ver #2)
>  - Added Smack support
>  - Made LSM parameter extraction dependent on reference != NULL.
> ---
>  fs/fs_context.c               |  4 ++++
>  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, 104 insertions(+)
> 
> diff --git a/fs/fs_context.c b/fs/fs_context.c
> index 851214d1d013..a523aea956c4 100644
> --- a/fs/fs_context.c
> +++ b/fs/fs_context.c
> @@ -282,6 +282,10 @@ static struct fs_context *alloc_fs_context(struct file_system_type *fs_type,
>  		break;
>  	}
>  
> +	ret = security_fs_context_init(fc, reference);
> +	if (ret < 0)
> +		goto err_fc;
> +
>  	/* TODO: Make all filesystems support this unconditionally */
>  	init_fs_context = fc->fs_type->init_fs_context;
>  	if (!init_fs_context)
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 7308a1a7599b..7ce3550154b1 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 dentry *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..61fda06fac9d 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 dentry *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 dentry *reference)

I think that's the wrong way of doing this hook. The security hook
really doesn't belong into alloc_fs_context().

I think what we want is a dedicated helper similar to vfs_dup_context():

// Only pass the superblock. There's no need for the dentry. I would
// avoid even passing fs_context but if that's preferred then sure.
security_fs_context_submount(struct fs_context *fc, const struct super_block *sb)

vfs_submount_fs_context(struct file_system_type *fs_type, struct dentry *reference)
{
        fc = fs_context_for_submount(fs_type, reference);

        security_fs_context_for_submount(fc, reference->d_sb);
}

This automatically ensures it's only called for submounts, the LSM
doesn't need to care about fc->purpose and this isn't called
in a pure allocation function for all allocation calls.

The we should switch all callers over to that new helper and unexport
that fs_context_for_submount() thing completely. Yes, that's more work
but that's the correct thing to do. And we need to audit fuse, cifs,
afs, and nfs anyway that they work fine with the new security hook.*


[1]: If really needed, then any additional fs specific work that needs
     to be done during submount allocation should probably probably be
     done in a new callback.

     struct fs_context_operations {
            void (*free)(struct fs_context *fc);
            int (*dup)(struct fs_context *fc, struct fs_context *src_fc);
    +       int (*submount)(struct fs_context *fc, const struct super_block *sb);

Christian Brauner Aug. 5, 2023, 12:59 p.m. UTC | #2

On Sat, Aug 05, 2023 at 02:44:04PM +0200, Christian Brauner wrote:
> On Fri, Aug 04, 2023 at 12:09:34PM -0400, Jeff Layton wrote:
> > From: David Howells <[e-mail]>
> > 
> > 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 when alloc_fs_context() is creating the fs_context for it.
> > 
> > Signed-off-by: David Howells <[e-mail]>
> > Signed-off-by: Jeff Layton <[e-mail]>
> > 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 <[e-mail]>
> > Reviewed-by: Jeff Layton <[e-mail]>
> > Acked-by: Casey Schaufler <[e-mail]>
> > Acked-by: "Christian Brauner (Microsoft)" <[e-mail]>
> > Link: https://lore.kernel.org/r/[e-mail]/ # v1
> > Link: https://lore.kernel.org/r/[e-mail]/ # v2
> > Link: https://lore.kernel.org/r/[e-mail]/ # v3
> > Link: https://lore.kernel.org/r/[e-mail]/ # v4
> > Link: https://lore.kernel.org/r/[e-mail]/ # v5
> > ---
> > ver #7)
> >  - Drop lsm_set boolean
> >  - Link to v6: https://lore.kernel.org/r/[e-mail]
> > 
> > ver #6)
> >  - Rebase onto v6.5.0-rc4
> > 
> > ver #5)
> >  - Removed unused variable.
> >  - Only allocate smack_mnt_opts if we're dealing with a submount.
> > 
> > ver #4)
> >  - When doing a FOR_SUBMOUNT mount, don't set the root label in SELinux or
> >    Smack.
> > 
> > ver #3)
> >  - Made LSM parameter extraction dependent on fc->purpose ==
> >    FS_CONTEXT_FOR_SUBMOUNT.  Shouldn't happen on FOR_RECONFIGURE.
> > 
> > ver #2)
> >  - Added Smack support
> >  - Made LSM parameter extraction dependent on reference != NULL.
> > ---
> >  fs/fs_context.c               |  4 ++++
> >  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, 104 insertions(+)
> > 
> > diff --git a/fs/fs_context.c b/fs/fs_context.c
> > index 851214d1d013..a523aea956c4 100644
> > --- a/fs/fs_context.c
> > +++ b/fs/fs_context.c
> > @@ -282,6 +282,10 @@ static struct fs_context *alloc_fs_context(struct file_system_type *fs_type,
> >  		break;
> >  	}
> >  
> > +	ret = security_fs_context_init(fc, reference);
> > +	if (ret < 0)
> > +		goto err_fc;
> > +
> >  	/* TODO: Make all filesystems support this unconditionally */
> >  	init_fs_context = fc->fs_type->init_fs_context;
> >  	if (!init_fs_context)
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 7308a1a7599b..7ce3550154b1 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 dentry *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..61fda06fac9d 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 dentry *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 dentry *reference)
> 
> I think that's the wrong way of doing this hook. The security hook
> really doesn't belong into alloc_fs_context().
> 
> I think what we want is a dedicated helper similar to vfs_dup_context():
> 
> // Only pass the superblock. There's no need for the dentry. I would
> // avoid even passing fs_context but if that's preferred then sure.
> security_fs_context_submount(struct fs_context *fc, const struct super_block *sb)
> 
> vfs_submount_fs_context(struct file_system_type *fs_type, struct dentry *reference)
> {
>         fc = fs_context_for_submount(fs_type, reference);
> 
>         security_fs_context_for_submount(fc, reference->d_sb);
> }
> 
> This automatically ensures it's only called for submounts, the LSM
> doesn't need to care about fc->purpose and this isn't called
> in a pure allocation function for all allocation calls.
> 
> The we should switch all callers over to that new helper and unexport
> that fs_context_for_submount() thing completely. Yes, that's more work
> but that's the correct thing to do. And we need to audit fuse, cifs,
> afs, and nfs anyway that they work fine with the new security hook.*
> 
> 
> [1]: If really needed, then any additional fs specific work that needs
>      to be done during submount allocation should probably probably be

s/allocation/creation/

>      done in a new callback.
> 
>      struct fs_context_operations {
>             void (*free)(struct fs_context *fc);
>             int (*dup)(struct fs_context *fc, struct fs_context *src_fc);
>     +       int (*submount)(struct fs_context *fc, const struct super_block *sb);

in vfs_submount_fs_context() mirroring the ->dup() call in
vfs_dup_context().

Jeff Layton Aug. 7, 2023, 12:57 p.m. UTC | #3

On Sat, 2023-08-05 at 14:43 +0200, Christian Brauner wrote:
> On Fri, Aug 04, 2023 at 12:09:34PM -0400, Jeff Layton wrote:
> > From: David Howells <[e-mail]>
> > 
> > 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 when alloc_fs_context() is creating the fs_context for it.
> > 
> > Signed-off-by: David Howells <[e-mail]>
> > Signed-off-by: Jeff Layton <[e-mail]>
> > 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 <[e-mail]>
> > Reviewed-by: Jeff Layton <[e-mail]>
> > Acked-by: Casey Schaufler <[e-mail]>
> > Acked-by: "Christian Brauner (Microsoft)" <[e-mail]>
> > Link: https://lore.kernel.org/r/[e-mail]/ # v1
> > Link: https://lore.kernel.org/r/[e-mail]/ # v2
> > Link: https://lore.kernel.org/r/[e-mail]/ # v3
> > Link: https://lore.kernel.org/r/[e-mail]/ # v4
> > Link: https://lore.kernel.org/r/[e-mail]/ # v5
> > ---
> > ver #7)
> >  - Drop lsm_set boolean
> >  - Link to v6: https://lore.kernel.org/r/[e-mail]
> > 
> > ver #6)
> >  - Rebase onto v6.5.0-rc4
> > 
> > ver #5)
> >  - Removed unused variable.
> >  - Only allocate smack_mnt_opts if we're dealing with a submount.
> > 
> > ver #4)
> >  - When doing a FOR_SUBMOUNT mount, don't set the root label in SELinux or
> >    Smack.
> > 
> > ver #3)
> >  - Made LSM parameter extraction dependent on fc->purpose ==
> >    FS_CONTEXT_FOR_SUBMOUNT.  Shouldn't happen on FOR_RECONFIGURE.
> > 
> > ver #2)
> >  - Added Smack support
> >  - Made LSM parameter extraction dependent on reference != NULL.
> > ---
> >  fs/fs_context.c               |  4 ++++
> >  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, 104 insertions(+)
> > 
> > diff --git a/fs/fs_context.c b/fs/fs_context.c
> > index 851214d1d013..a523aea956c4 100644
> > --- a/fs/fs_context.c
> > +++ b/fs/fs_context.c
> > @@ -282,6 +282,10 @@ static struct fs_context *alloc_fs_context(struct file_system_type *fs_type,
> >  		break;
> >  	}
> >  
> > +	ret = security_fs_context_init(fc, reference);
> > +	if (ret < 0)
> > +		goto err_fc;
> > +
> >  	/* TODO: Make all filesystems support this unconditionally */
> >  	init_fs_context = fc->fs_type->init_fs_context;
> >  	if (!init_fs_context)
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 7308a1a7599b..7ce3550154b1 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 dentry *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..61fda06fac9d 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 dentry *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 dentry *reference)
> 
> I think that's the wrong way of doing this hook. The security hook
> really doesn't belong into alloc_fs_context().
> 
> I think what we want is a dedicated helper similar to vfs_dup_context():
> 
> // Only pass the superblock. There's no need for the dentry. I would
> // avoid even passing fs_context but if that's preferred then sure.
> security_fs_context_submount(struct fs_context *fc, const struct super_block *sb)
> 
> vfs_submount_fs_context(struct file_system_type *fs_type, struct dentry *reference)
> {
>         fc = fs_context_for_submount(fs_type, reference);
> 
>         security_fs_context_for_submount(fc, reference->d_sb);
> }
> 
> This automatically ensures it's only called for submounts, the LSM
> doesn't need to care about fc->purpose and this isn't called
> in a pure allocation function for all allocation calls.
> 
> The we should switch all callers over to that new helper and unexport
> that fs_context_for_submount() thing completely. Yes, that's more work
> but that's the correct thing to do. And we need to audit fuse, cifs,
> afs, and nfs anyway that they work fine with the new security hook.*
> 

It's the same prototype. We could just move the hook call to the end of
fs_context_for_submount, and that would be less churn for its callers.
Or were you wanting to do that to make this a more gradual changeover
for some reason?

I will rework the security hook to take a sb pointer instead though.

> 
> [1]: If really needed, then any additional fs specific work that needs
>      to be done during submount allocation should probably probably be
>      done in a new callback.
> 
>      struct fs_context_operations {
>             void (*free)(struct fs_context *fc);
>             int (*dup)(struct fs_context *fc, struct fs_context *src_fc);
>     +       int (*submount)(struct fs_context *fc, const struct super_block *sb);

Christian Brauner Aug. 7, 2023, 1:08 p.m. UTC | #4

On Mon, Aug 07, 2023 at 08:57:03AM -0400, Jeff Layton wrote:
> On Sat, 2023-08-05 at 14:43 +0200, Christian Brauner wrote:
> > On Fri, Aug 04, 2023 at 12:09:34PM -0400, Jeff Layton wrote:
> > > From: David Howells <[e-mail]>
> > > 
> > > 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 when alloc_fs_context() is creating the fs_context for it.
> > > 
> > > Signed-off-by: David Howells <[e-mail]>
> > > Signed-off-by: Jeff Layton <[e-mail]>
> > > 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 <[e-mail]>
> > > Reviewed-by: Jeff Layton <[e-mail]>
> > > Acked-by: Casey Schaufler <[e-mail]>
> > > Acked-by: "Christian Brauner (Microsoft)" <[e-mail]>
> > > Link: https://lore.kernel.org/r/[e-mail]/ # v1
> > > Link: https://lore.kernel.org/r/[e-mail]/ # v2
> > > Link: https://lore.kernel.org/r/[e-mail]/ # v3
> > > Link: https://lore.kernel.org/r/[e-mail]/ # v4
> > > Link: https://lore.kernel.org/r/[e-mail]/ # v5
> > > ---
> > > ver #7)
> > >  - Drop lsm_set boolean
> > >  - Link to v6: https://lore.kernel.org/r/[e-mail]
> > > 
> > > ver #6)
> > >  - Rebase onto v6.5.0-rc4
> > > 
> > > ver #5)
> > >  - Removed unused variable.
> > >  - Only allocate smack_mnt_opts if we're dealing with a submount.
> > > 
> > > ver #4)
> > >  - When doing a FOR_SUBMOUNT mount, don't set the root label in SELinux or
> > >    Smack.
> > > 
> > > ver #3)
> > >  - Made LSM parameter extraction dependent on fc->purpose ==
> > >    FS_CONTEXT_FOR_SUBMOUNT.  Shouldn't happen on FOR_RECONFIGURE.
> > > 
> > > ver #2)
> > >  - Added Smack support
> > >  - Made LSM parameter extraction dependent on reference != NULL.
> > > ---
> > >  fs/fs_context.c               |  4 ++++
> > >  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, 104 insertions(+)
> > > 
> > > diff --git a/fs/fs_context.c b/fs/fs_context.c
> > > index 851214d1d013..a523aea956c4 100644
> > > --- a/fs/fs_context.c
> > > +++ b/fs/fs_context.c
> > > @@ -282,6 +282,10 @@ static struct fs_context *alloc_fs_context(struct file_system_type *fs_type,
> > >  		break;
> > >  	}
> > >  
> > > +	ret = security_fs_context_init(fc, reference);
> > > +	if (ret < 0)
> > > +		goto err_fc;
> > > +
> > >  	/* TODO: Make all filesystems support this unconditionally */
> > >  	init_fs_context = fc->fs_type->init_fs_context;
> > >  	if (!init_fs_context)
> > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > > index 7308a1a7599b..7ce3550154b1 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 dentry *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..61fda06fac9d 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 dentry *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 dentry *reference)
> > 
> > I think that's the wrong way of doing this hook. The security hook
> > really doesn't belong into alloc_fs_context().
> > 
> > I think what we want is a dedicated helper similar to vfs_dup_context():
> > 
> > // Only pass the superblock. There's no need for the dentry. I would
> > // avoid even passing fs_context but if that's preferred then sure.
> > security_fs_context_submount(struct fs_context *fc, const struct super_block *sb)
> > 
> > vfs_submount_fs_context(struct file_system_type *fs_type, struct dentry *reference)
> > {
> >         fc = fs_context_for_submount(fs_type, reference);
> > 
> >         security_fs_context_for_submount(fc, reference->d_sb);
> > }
> > 
> > This automatically ensures it's only called for submounts, the LSM
> > doesn't need to care about fc->purpose and this isn't called
> > in a pure allocation function for all allocation calls.
> > 
> > The we should switch all callers over to that new helper and unexport
> > that fs_context_for_submount() thing completely. Yes, that's more work
> > but that's the correct thing to do. And we need to audit fuse, cifs,
> > afs, and nfs anyway that they work fine with the new security hook.*
> > 
> 
> It's the same prototype. We could just move the hook call to the end of
> fs_context_for_submount, and that would be less churn for its callers.

If you just add it into fs_context_for_submount() after the allocation
and then leave that as first class citizen it's fine ofc. It's the same
result as what I mentioned earlier. We just shouldn't put generic hooks
in an allocation function that allocates different types of filesystem
contexts. I prefer to have them as targeted as possible and also avoid
unnecessary trips into the LSM layer.

> Or were you wanting to do that to make this a more gradual changeover
> for some reason?

No, I think it's fine if you switch it all in one go. I just don't know
if fuse/virtiofs submounts expect an selinux context to be bestowed upon
them.

Patch

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 851214d1d013..a523aea956c4 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -282,6 +282,10 @@  static struct fs_context *alloc_fs_context(struct file_system_type *fs_type,
 		break;
 	}
 
+	ret = security_fs_context_init(fc, reference);
+	if (ret < 0)
+		goto err_fc;
+
 	/* TODO: Make all filesystems support this unconditionally */
 	init_fs_context = fc->fs_type->init_fs_context;
 	if (!init_fs_context)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 7308a1a7599b..7ce3550154b1 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 dentry *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..61fda06fac9d 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 dentry *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 dentry *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..8a6dc6f7cda0 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
+ * @dentry: 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 dentry *reference)
+{
+	return call_int_hook(fs_context_init, 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..97fe5aab82dc 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 dentry *reference)
+{
+	const struct superblock_security_struct *sbsec;
+	struct selinux_mnt_opts *opts;
+
+	if (!reference || fc->purpose != FS_CONTEXT_FOR_SUBMOUNT)
+		return 0;
+
+	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
+	if (!opts)
+		return -ENOMEM;
+
+	sbsec = selinux_superblock(reference->d_sb);
+	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 +7206,7 @@  static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	/*
 	 * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
 	 */
+	LSM_HOOK_INIT(fs_context_init, selinux_fs_context_init),
 	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..ca5f2dbc9116 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -614,6 +614,59 @@  static int smack_add_opt(int token, const char *s, void **mnt_opts)
 	return -EINVAL;
 }
 
+/**
+ * smack_fs_context_init - Initialise security data for a filesystem context
+ * @fc: The filesystem context.
+ * @reference: Reference dentry (automount/reconfigure) or NULL
+ *
+ * Returns 0 on success or -ENOMEM on error.
+ */
+static int smack_fs_context_init(struct fs_context *fc,
+				 struct dentry *reference)
+{
+	struct superblock_smack *sbsp;
+	struct smack_mnt_opts *ctx;
+	struct inode_smack *isp;
+
+	if (!reference || fc->purpose != FS_CONTEXT_FOR_SUBMOUNT);
+		return 0;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	fc->security = ctx;
+
+	sbsp = smack_superblock(reference->d_sb);
+	isp = smack_inode(reference->d_sb->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 +4929,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_init, smack_fs_context_init),
 	LSM_HOOK_INIT(fs_context_dup, smack_fs_context_dup),
 	LSM_HOOK_INIT(fs_context_parse_param, smack_fs_context_parse_param),