[PATCH] selinux: fix handling of empty opts in selinux_fs_context_submount()

[PATCH] selinux: fix handling of empty opts in selinux_fs_context_submount()

[Ondrej Mosnacek]

selinux_set_mnt_opts() relies on the fact that the mount options pointer
is always NULL when all options are unset (specifically in its
!selinux_initialized() branch. However, the new
selinux_fs_context_submount() hook breaks this rule by allocating a new
structure even if no options are set. That causes any submount created
before a SELinux policy is loaded to be rejected in
selinux_set_mnt_opts().

Fix this by making selinux_fs_context_submount() leave fc->security
set to NULL when there are no options to be copied from the reference
superblock.

Reported-by: Adam Williamson <awilliam@redhat.com>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2236345
Fixes: d80a8f1b58c2 ("vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 10350534de6d6..2aa0e219d7217 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2775,14 +2775,20 @@ static int selinux_umount(struct vfsmount *mnt, int flags)
 static int selinux_fs_context_submount(struct fs_context *fc,
 				   struct super_block *reference)
 {
-	const struct superblock_security_struct *sbsec;
+	const struct superblock_security_struct *sbsec = selinux_superblock(reference);
 	struct selinux_mnt_opts *opts;
 
+	/*
+	 * Ensure that fc->security remains NULL when no options are set
+	 * as expected by selinux_set_mnt_opts().
+	 */
+	if (!(sbsec->flags & (FSCONTEXT_MNT|CONTEXT_MNT|DEFCONTEXT_MNT)))
+		return 0;
+
 	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)
-- 
2.41.0

Re: [PATCH] selinux: fix handling of empty opts in selinux_fs_context_submount()

[Jeff Layton]

On Mon, 2023-09-11 at 16:23 +0200, Ondrej Mosnacek wrote:
> selinux_set_mnt_opts() relies on the fact that the mount options pointer
> is always NULL when all options are unset (specifically in its
> !selinux_initialized() branch. However, the new
> selinux_fs_context_submount() hook breaks this rule by allocating a new
> structure even if no options are set. That causes any submount created
> before a SELinux policy is loaded to be rejected in
> selinux_set_mnt_opts().
> 
> Fix this by making selinux_fs_context_submount() leave fc->security
> set to NULL when there are no options to be copied from the reference
> superblock.
> 
> Reported-by: Adam Williamson <awilliam@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2236345
> Fixes: d80a8f1b58c2 ("vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/hooks.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 10350534de6d6..2aa0e219d7217 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2775,14 +2775,20 @@ static int selinux_umount(struct vfsmount *mnt, int flags)
>  static int selinux_fs_context_submount(struct fs_context *fc,
>  				   struct super_block *reference)
>  {
> -	const struct superblock_security_struct *sbsec;
> +	const struct superblock_security_struct *sbsec = selinux_superblock(reference);
>  	struct selinux_mnt_opts *opts;
>  
> +	/*
> +	 * Ensure that fc->security remains NULL when no options are set
> +	 * as expected by selinux_set_mnt_opts().
> +	 */
> +	if (!(sbsec->flags & (FSCONTEXT_MNT|CONTEXT_MNT|DEFCONTEXT_MNT)))
> +		return 0;
> +
>  	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)

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] selinux: fix handling of empty opts in selinux_fs_context_submount()

[Paul Moore]

On Sep 11, 2023 Ondrej Mosnacek <omosnace@redhat.com> wrote:
> 
> selinux_set_mnt_opts() relies on the fact that the mount options pointer
> is always NULL when all options are unset (specifically in its
> !selinux_initialized() branch. However, the new
> selinux_fs_context_submount() hook breaks this rule by allocating a new
> structure even if no options are set. That causes any submount created
> before a SELinux policy is loaded to be rejected in
> selinux_set_mnt_opts().
> 
> Fix this by making selinux_fs_context_submount() leave fc->security
> set to NULL when there are no options to be copied from the reference
> superblock.
> 
> Reported-by: Adam Williamson <awilliam@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2236345
> Fixes: d80a8f1b58c2 ("vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> ---
>  security/selinux/hooks.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Thanks Ondrej, this looks good.  I'm going to merge this into
selinux/stable-6.6 and assuming all goes well with the automated
testing (I can't imagine it would catch anything) I'll send this up
to Linus later this week.

I'm also tagging this for the stable kernels even though this patch
is only present in v6.6-rc1 because the original patch has a number
of 'Fixes:' tags which means the stable folks will probably end up
pulling it into their trees.

--
paul-moore.com

Re: [PATCH] selinux: fix handling of empty opts in selinux_fs_context_submount()

[Christian Brauner]

On Mon, Sep 11, 2023 at 04:23:58PM +0200, Ondrej Mosnacek wrote:
> selinux_set_mnt_opts() relies on the fact that the mount options pointer
> is always NULL when all options are unset (specifically in its
> !selinux_initialized() branch. However, the new
> selinux_fs_context_submount() hook breaks this rule by allocating a new
> structure even if no options are set. That causes any submount created
> before a SELinux policy is loaded to be rejected in
> selinux_set_mnt_opts().
> 
> Fix this by making selinux_fs_context_submount() leave fc->security
> set to NULL when there are no options to be copied from the reference
> superblock.
> 
> Reported-by: Adam Williamson <awilliam@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2236345
> Fixes: d80a8f1b58c2 ("vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>