[v1] LSM: Multiple LSM mount options

[PATCH 2/3] LSM: allocate mnt_opts blobs instead of module specific data

Posted by Casey Schaufler 3 months, 3 weeks ago

Replace allocations of LSM specific mount data with the
shared mnt_opts blob.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 include/linux/lsm_hooks.h  |  1 +
 security/security.c        | 12 ++++++++++++
 security/selinux/hooks.c   | 10 +++++++---
 security/smack/smack_lsm.c |  4 ++--
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9741c76e4654..1871ebc5833b 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -219,4 +219,5 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs,
 	return &xattrs[(*xattr_count)++];
 }
 
+extern void *lsm_mnt_opts_alloc(gfp_t priority);
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/security.c b/security/security.c
index 8a4e0f70e49d..ec61fb7e6492 100644
--- a/security/security.c
+++ b/security/security.c
@@ -904,6 +904,18 @@ void security_sb_free(struct super_block *sb)
 	sb->s_security = NULL;
 }
 
+/**
+ * lsm_mnt_opts_alloc - allocate a mnt_opts blob
+ * @priority: memory allocation priority
+ *
+ * Returns a newly allocated mnt_opts blob or NULL if
+ * memory isn't available.
+ */
+void *lsm_mnt_opts_alloc(gfp_t priority)
+{
+	return kzalloc(blob_sizes.lbs_mnt_opts, priority);
+}
+
 /**
  * security_free_mnt_opts() - Free memory associated with mount options
  * @mnt_opts: LSM processed mount options
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 88cd1d56081a..f7eda0cce68f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2808,7 +2808,7 @@ static int selinux_fs_context_submount(struct fs_context *fc,
 	if (!(sbsec->flags & (FSCONTEXT_MNT|CONTEXT_MNT|DEFCONTEXT_MNT)))
 		return 0;
 
-	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
+	opts = lsm_mnt_opts_alloc(GFP_KERNEL);
 	if (!opts)
 		return -ENOMEM;
 
@@ -2830,8 +2830,12 @@ static int selinux_fs_context_dup(struct fs_context *fc,
 	if (!src)
 		return 0;
 
-	fc->security = kmemdup(src, sizeof(*src), GFP_KERNEL);
-	return fc->security ? 0 : -ENOMEM;
+	fc->security = lsm_mnt_opts_alloc(GFP_KERNEL);
+	if (!fc->security)
+		return -ENOMEM;
+
+	memcpy(fc->security, src, sizeof(*src));
+	return 0;
 }
 
 static const struct fs_parameter_spec selinux_fs_parameters[] = {
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 44bd92410425..1d456df40096 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -622,7 +622,7 @@ static int smack_fs_context_submount(struct fs_context *fc,
 	struct smack_mnt_opts *ctx;
 	struct inode_smack *isp;
 
-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	ctx = lsm_mnt_opts_alloc(GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 	fc->security = ctx;
@@ -673,7 +673,7 @@ static int smack_fs_context_dup(struct fs_context *fc,
 	if (!src)
 		return 0;
 
-	fc->security = kzalloc(sizeof(struct smack_mnt_opts), GFP_KERNEL);
+	fc->security = lsm_mnt_opts_alloc(GFP_KERNEL);
 	if (!fc->security)
 		return -ENOMEM;
 
-- 
2.47.0

Re: [PATCH 2/3] LSM: allocate mnt_opts blobs instead of module specific data

Posted by Paul Moore 2 months ago

On Jun 17, 2025 Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> Replace allocations of LSM specific mount data with the
> shared mnt_opts blob.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  include/linux/lsm_hooks.h  |  1 +
>  security/security.c        | 12 ++++++++++++
>  security/selinux/hooks.c   | 10 +++++++---
>  security/smack/smack_lsm.c |  4 ++--
>  4 files changed, 22 insertions(+), 5 deletions(-)

...

> diff --git a/security/security.c b/security/security.c
> index 8a4e0f70e49d..ec61fb7e6492 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -904,6 +904,18 @@ void security_sb_free(struct super_block *sb)
>  	sb->s_security = NULL;
>  }
>  
> +/**
> + * lsm_mnt_opts_alloc - allocate a mnt_opts blob
> + * @priority: memory allocation priority
> + *
> + * Returns a newly allocated mnt_opts blob or NULL if
> + * memory isn't available.
> + */
> +void *lsm_mnt_opts_alloc(gfp_t priority)
> +{
> +	return kzalloc(blob_sizes.lbs_mnt_opts, priority);
> +}

It's probably better to use lsm_blob_alloc() here so we have some
allocator consistency.

Also, make this private/static as we should just handle the blob
allocation in the LSM framework (see below) just like everything else,
unless you can explain why the mount options need to be handled
differently?

>  /**
>   * security_free_mnt_opts() - Free memory associated with mount options
>   * @mnt_opts: LSM processed mount options
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 88cd1d56081a..f7eda0cce68f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2808,7 +2808,7 @@ static int selinux_fs_context_submount(struct fs_context *fc,
>  	if (!(sbsec->flags & (FSCONTEXT_MNT|CONTEXT_MNT|DEFCONTEXT_MNT)))
>  		return 0;
>  
> -	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> +	opts = lsm_mnt_opts_alloc(GFP_KERNEL);

See above.

>  	if (!opts)
>  		return -ENOMEM;
>  
> @@ -2830,8 +2830,12 @@ static int selinux_fs_context_dup(struct fs_context *fc,
>  	if (!src)
>  		return 0;
>  
> -	fc->security = kmemdup(src, sizeof(*src), GFP_KERNEL);
> -	return fc->security ? 0 : -ENOMEM;
> +	fc->security = lsm_mnt_opts_alloc(GFP_KERNEL);
> +	if (!fc->security)
> +		return -ENOMEM;

Another case where we should do the allocation in the LSM framework.

> +	memcpy(fc->security, src, sizeof(*src));
> +	return 0;
>  }
>  
>  static const struct fs_parameter_spec selinux_fs_parameters[] = {
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 44bd92410425..1d456df40096 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -622,7 +622,7 @@ static int smack_fs_context_submount(struct fs_context *fc,
>  	struct smack_mnt_opts *ctx;
>  	struct inode_smack *isp;
>  
> -	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +	ctx = lsm_mnt_opts_alloc(GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
>  	fc->security = ctx;
> @@ -673,7 +673,7 @@ static int smack_fs_context_dup(struct fs_context *fc,
>  	if (!src)
>  		return 0;
>  
> -	fc->security = kzalloc(sizeof(struct smack_mnt_opts), GFP_KERNEL);
> +	fc->security = lsm_mnt_opts_alloc(GFP_KERNEL);
>  	if (!fc->security)
>  		return -ENOMEM;

Same thing in Smack.

--
paul-moore.com

Re: [PATCH 2/3] LSM: allocate mnt_opts blobs instead of module specific data

Posted by Casey Schaufler 2 months ago

On 8/6/2025 3:06 PM, Paul Moore wrote:
> On Jun 17, 2025 Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Replace allocations of LSM specific mount data with the
>> shared mnt_opts blob.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>>  include/linux/lsm_hooks.h  |  1 +
>>  security/security.c        | 12 ++++++++++++
>>  security/selinux/hooks.c   | 10 +++++++---
>>  security/smack/smack_lsm.c |  4 ++--
>>  4 files changed, 22 insertions(+), 5 deletions(-)
> ..
>
>> diff --git a/security/security.c b/security/security.c
>> index 8a4e0f70e49d..ec61fb7e6492 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -904,6 +904,18 @@ void security_sb_free(struct super_block *sb)
>>  	sb->s_security = NULL;
>>  }
>>  
>> +/**
>> + * lsm_mnt_opts_alloc - allocate a mnt_opts blob
>> + * @priority: memory allocation priority
>> + *
>> + * Returns a newly allocated mnt_opts blob or NULL if
>> + * memory isn't available.
>> + */
>> +void *lsm_mnt_opts_alloc(gfp_t priority)
>> +{
>> +	return kzalloc(blob_sizes.lbs_mnt_opts, priority);
>> +}
> It's probably better to use lsm_blob_alloc() here so we have some
> allocator consistency.
>
> Also, make this private/static as we should just handle the blob
> allocation in the LSM framework (see below) just like everything else,
> unless you can explain why the mount options need to be handled
> differently?

The mount blob is different from the other blobs in that it is
only used if there are LSM specific mount options. If there aren't
LSM specific mount options there is no reason to have a blob.
I know it's not a huge deal, but there is a performance cost in
allocating a blob that isn't used.

If you'd really rather accept the overhead, I can make the blob
always allocated. Let me know. 


>
>>  /**
>>   * security_free_mnt_opts() - Free memory associated with mount options
>>   * @mnt_opts: LSM processed mount options
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 88cd1d56081a..f7eda0cce68f 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -2808,7 +2808,7 @@ static int selinux_fs_context_submount(struct fs_context *fc,
>>  	if (!(sbsec->flags & (FSCONTEXT_MNT|CONTEXT_MNT|DEFCONTEXT_MNT)))
>>  		return 0;
>>  
>> -	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
>> +	opts = lsm_mnt_opts_alloc(GFP_KERNEL);
> See above.
>
>>  	if (!opts)
>>  		return -ENOMEM;
>>  
>> @@ -2830,8 +2830,12 @@ static int selinux_fs_context_dup(struct fs_context *fc,
>>  	if (!src)
>>  		return 0;
>>  
>> -	fc->security = kmemdup(src, sizeof(*src), GFP_KERNEL);
>> -	return fc->security ? 0 : -ENOMEM;
>> +	fc->security = lsm_mnt_opts_alloc(GFP_KERNEL);
>> +	if (!fc->security)
>> +		return -ENOMEM;
> Another case where we should do the allocation in the LSM framework.
>
>> +	memcpy(fc->security, src, sizeof(*src));
>> +	return 0;
>>  }
>>  
>>  static const struct fs_parameter_spec selinux_fs_parameters[] = {
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 44bd92410425..1d456df40096 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -622,7 +622,7 @@ static int smack_fs_context_submount(struct fs_context *fc,
>>  	struct smack_mnt_opts *ctx;
>>  	struct inode_smack *isp;
>>  
>> -	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> +	ctx = lsm_mnt_opts_alloc(GFP_KERNEL);
>>  	if (!ctx)
>>  		return -ENOMEM;
>>  	fc->security = ctx;
>> @@ -673,7 +673,7 @@ static int smack_fs_context_dup(struct fs_context *fc,
>>  	if (!src)
>>  		return 0;
>>  
>> -	fc->security = kzalloc(sizeof(struct smack_mnt_opts), GFP_KERNEL);
>> +	fc->security = lsm_mnt_opts_alloc(GFP_KERNEL);
>>  	if (!fc->security)
>>  		return -ENOMEM;
> Same thing in Smack.
>
> --
> paul-moore.com
>

Re: [PATCH 2/3] LSM: allocate mnt_opts blobs instead of module specific data

Posted by Paul Moore 2 months ago

On Wed, Aug 6, 2025 at 7:16 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/6/2025 3:06 PM, Paul Moore wrote:
> > On Jun 17, 2025 Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> Replace allocations of LSM specific mount data with the
> >> shared mnt_opts blob.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> ---
> >>  include/linux/lsm_hooks.h  |  1 +
> >>  security/security.c        | 12 ++++++++++++
> >>  security/selinux/hooks.c   | 10 +++++++---
> >>  security/smack/smack_lsm.c |  4 ++--
> >>  4 files changed, 22 insertions(+), 5 deletions(-)
> > ..
> >
> >> diff --git a/security/security.c b/security/security.c
> >> index 8a4e0f70e49d..ec61fb7e6492 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >> @@ -904,6 +904,18 @@ void security_sb_free(struct super_block *sb)
> >>      sb->s_security = NULL;
> >>  }
> >>
> >> +/**
> >> + * lsm_mnt_opts_alloc - allocate a mnt_opts blob
> >> + * @priority: memory allocation priority
> >> + *
> >> + * Returns a newly allocated mnt_opts blob or NULL if
> >> + * memory isn't available.
> >> + */
> >> +void *lsm_mnt_opts_alloc(gfp_t priority)
> >> +{
> >> +    return kzalloc(blob_sizes.lbs_mnt_opts, priority);
> >> +}
> > It's probably better to use lsm_blob_alloc() here so we have some
> > allocator consistency.
> >
> > Also, make this private/static as we should just handle the blob
> > allocation in the LSM framework (see below) just like everything else,
> > unless you can explain why the mount options need to be handled
> > differently?
>
> The mount blob is different from the other blobs in that it is
> only used if there are LSM specific mount options. If there aren't
> LSM specific mount options there is no reason to have a blob.
> I know it's not a huge deal, but there is a performance cost in
> allocating a blob that isn't used.
>
> If you'd really rather accept the overhead, I can make the blob
> always allocated. Let me know.

Well, this is happening at mount time, which should already have a
non-trivial amount of overhead (parsing options, doing the filesystem
setup, mount tree addition, etc.) so I'm not sure this will really be
noticeable in practice.  I guess one could also make an argument about
additional memory pressure, but the mount options blob should have a
fairly short lifetime so I don't see that as a significant issue
either.  If one, or both, of these becomes an issue we can look into
ways of mitigating them, but right now I'd just assume keep with the
existing LSM blob allocation pattern to simplify the code and make
life easier.

-- 
paul-moore.com

[PATCH 1/3] LSM: Add mount opts blob size tracking
[PATCH 2/3] LSM: allocate mnt_opts blobs instead of module specific data
[PATCH 3/3] LSM: Infrastructure management of the mnt_opts security blob