diff mbox series

[RFC,27/29] lsm: consolidate all of the LSM framework initcalls

Message ID 20250409185019.238841-58-paul@paul-moore.com (mailing list archive)
State New
Headers show
Series Rework the LSM initialization | expand

Commit Message

Paul Moore April 9, 2025, 6:50 p.m. UTC
The LSM framework itself registers a small number of initcalls, this
patch converts these initcalls into the new initcall mechanism.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/inode.c    |  3 +--
 security/lsm.h      |  4 ++++
 security/lsm_init.c | 14 ++++++++++++--
 security/min_addr.c |  5 +++--
 4 files changed, 20 insertions(+), 6 deletions(-)

Comments

Kees Cook April 9, 2025, 11:52 p.m. UTC | #1
On Wed, Apr 09, 2025 at 02:50:12PM -0400, Paul Moore wrote:
> The LSM framework itself registers a small number of initcalls, this
> patch converts these initcalls into the new initcall mechanism.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  security/inode.c    |  3 +--
>  security/lsm.h      |  4 ++++
>  security/lsm_init.c | 14 ++++++++++++--
>  security/min_addr.c |  5 +++--
>  4 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/security/inode.c b/security/inode.c
> index f687e22e6809..671c66c147bc 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -375,7 +375,7 @@ static const struct file_operations lsm_ops = {
>  };
>  #endif
>  
> -static int __init securityfs_init(void)
> +int __init securityfs_init(void)
>  {
>  	int retval;
>  
> @@ -394,4 +394,3 @@ static int __init securityfs_init(void)
>  #endif
>  	return 0;
>  }
> -core_initcall(securityfs_init);
> diff --git a/security/lsm.h b/security/lsm.h
> index 8ecb66896646..c432dc0c5e30 100644
> --- a/security/lsm.h
> +++ b/security/lsm.h
> @@ -35,4 +35,8 @@ extern struct kmem_cache *lsm_inode_cache;
>  int lsm_cred_alloc(struct cred *cred, gfp_t gfp);
>  int lsm_task_alloc(struct task_struct *task);
>  
> +/* LSM framework initializers */
> +int securityfs_init(void);
> +int min_addr_init(void);
> +
>  #endif /* _LSM_H_ */
> diff --git a/security/lsm_init.c b/security/lsm_init.c
> index 75eb0cc82869..c0881407ca3f 100644
> --- a/security/lsm_init.c
> +++ b/security/lsm_init.c
> @@ -485,7 +485,12 @@ int __init security_init(void)
>   */
>  static int __init security_initcall_pure(void)
>  {
> -	return lsm_initcall(pure);
> +	int rc_adr, rc_lsm;
> +
> +	rc_adr = min_addr_init();
> +	rc_lsm = lsm_initcall(pure);
> +
> +	return (rc_adr ? rc_adr : rc_lsm);
>  }
>  pure_initcall(security_initcall_pure);
>  
> @@ -503,7 +508,12 @@ early_initcall(security_initcall_early);
>   */
>  static int __init security_initcall_core(void)
>  {
> -	return lsm_initcall(core);
> +	int rc_sfs, rc_lsm;
> +
> +	rc_sfs = securityfs_init();
> +	rc_lsm = lsm_initcall(core);
> +
> +	return (rc_sfs ? rc_sfs : rc_lsm);
>  }
>  core_initcall(security_initcall_core);

Hrm. Given these aren't really _lsm_ hooks, maybe just leave this out. I
worry about confusing the lsm inits with the lsm subsystem's core inits.
Or we need a new stacking type for "required"? But that seems ... heavy.

-Kees

>  
> diff --git a/security/min_addr.c b/security/min_addr.c
> index df1bc643d886..40714bdeefbe 100644
> --- a/security/min_addr.c
> +++ b/security/min_addr.c
> @@ -4,6 +4,8 @@
>  #include <linux/security.h>
>  #include <linux/sysctl.h>
>  
> +#include "lsm.h"
> +
>  /* amount of vm to protect from userspace access by both DAC and the LSM*/
>  unsigned long mmap_min_addr;
>  /* amount of vm to protect from userspace using CAP_SYS_RAWIO (DAC) */
> @@ -54,11 +56,10 @@ static const struct ctl_table min_addr_sysctl_table[] = {
>  	},
>  };
>  
> -static int __init init_mmap_min_addr(void)
> +int __init min_addr_init(void)
>  {
>  	register_sysctl_init("vm", min_addr_sysctl_table);
>  	update_mmap_min_addr();
>  
>  	return 0;
>  }
> -pure_initcall(init_mmap_min_addr);
> -- 
> 2.49.0
>
Paul Moore April 11, 2025, 1:21 a.m. UTC | #2
On Wed, Apr 9, 2025 at 7:52 PM Kees Cook <kees@kernel.org> wrote:
> On Wed, Apr 09, 2025 at 02:50:12PM -0400, Paul Moore wrote:
> > The LSM framework itself registers a small number of initcalls, this
> > patch converts these initcalls into the new initcall mechanism.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> >  security/inode.c    |  3 +--
> >  security/lsm.h      |  4 ++++
> >  security/lsm_init.c | 14 ++++++++++++--
> >  security/min_addr.c |  5 +++--
> >  4 files changed, 20 insertions(+), 6 deletions(-)

...

> > @@ -503,7 +508,12 @@ early_initcall(security_initcall_early);
> >   */
> >  static int __init security_initcall_core(void)
> >  {
> > -     return lsm_initcall(core);
> > +     int rc_sfs, rc_lsm;
> > +
> > +     rc_sfs = securityfs_init();
> > +     rc_lsm = lsm_initcall(core);
> > +
> > +     return (rc_sfs ? rc_sfs : rc_lsm);
> >  }
> >  core_initcall(security_initcall_core);
>
> Hrm. Given these aren't really _lsm_ hooks, maybe just leave this out. I
> worry about confusing the lsm inits with the lsm subsystem's core inits.

I'm not too concerned about that, and I do prefer it this way.

> Or we need a new stacking type for "required"? But that seems ... heavy.

So I understand the motivation behind that, but that's a big hard "no"
from me at this point in time ;)
Kees Cook April 11, 2025, 2:16 a.m. UTC | #3
On Thu, Apr 10, 2025 at 09:21:46PM -0400, Paul Moore wrote:
> On Wed, Apr 9, 2025 at 7:52 PM Kees Cook <kees@kernel.org> wrote:
> > On Wed, Apr 09, 2025 at 02:50:12PM -0400, Paul Moore wrote:
> > > The LSM framework itself registers a small number of initcalls, this
> > > patch converts these initcalls into the new initcall mechanism.
> > >
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > ---
> > >  security/inode.c    |  3 +--
> > >  security/lsm.h      |  4 ++++
> > >  security/lsm_init.c | 14 ++++++++++++--
> > >  security/min_addr.c |  5 +++--
> > >  4 files changed, 20 insertions(+), 6 deletions(-)
> 
> ...
> 
> > > @@ -503,7 +508,12 @@ early_initcall(security_initcall_early);
> > >   */
> > >  static int __init security_initcall_core(void)
> > >  {
> > > -     return lsm_initcall(core);
> > > +     int rc_sfs, rc_lsm;
> > > +
> > > +     rc_sfs = securityfs_init();
> > > +     rc_lsm = lsm_initcall(core);
> > > +
> > > +     return (rc_sfs ? rc_sfs : rc_lsm);
> > >  }
> > >  core_initcall(security_initcall_core);
> >
> > Hrm. Given these aren't really _lsm_ hooks, maybe just leave this out. I
> > worry about confusing the lsm inits with the lsm subsystem's core inits.
> 
> I'm not too concerned about that, and I do prefer it this way.

Sounds good to me. And with an eye toward trying to minimize which kinds
of init calls we have in the future, I think it'll just get cleaner over
time.
diff mbox series

Patch

diff --git a/security/inode.c b/security/inode.c
index f687e22e6809..671c66c147bc 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -375,7 +375,7 @@  static const struct file_operations lsm_ops = {
 };
 #endif
 
-static int __init securityfs_init(void)
+int __init securityfs_init(void)
 {
 	int retval;
 
@@ -394,4 +394,3 @@  static int __init securityfs_init(void)
 #endif
 	return 0;
 }
-core_initcall(securityfs_init);
diff --git a/security/lsm.h b/security/lsm.h
index 8ecb66896646..c432dc0c5e30 100644
--- a/security/lsm.h
+++ b/security/lsm.h
@@ -35,4 +35,8 @@  extern struct kmem_cache *lsm_inode_cache;
 int lsm_cred_alloc(struct cred *cred, gfp_t gfp);
 int lsm_task_alloc(struct task_struct *task);
 
+/* LSM framework initializers */
+int securityfs_init(void);
+int min_addr_init(void);
+
 #endif /* _LSM_H_ */
diff --git a/security/lsm_init.c b/security/lsm_init.c
index 75eb0cc82869..c0881407ca3f 100644
--- a/security/lsm_init.c
+++ b/security/lsm_init.c
@@ -485,7 +485,12 @@  int __init security_init(void)
  */
 static int __init security_initcall_pure(void)
 {
-	return lsm_initcall(pure);
+	int rc_adr, rc_lsm;
+
+	rc_adr = min_addr_init();
+	rc_lsm = lsm_initcall(pure);
+
+	return (rc_adr ? rc_adr : rc_lsm);
 }
 pure_initcall(security_initcall_pure);
 
@@ -503,7 +508,12 @@  early_initcall(security_initcall_early);
  */
 static int __init security_initcall_core(void)
 {
-	return lsm_initcall(core);
+	int rc_sfs, rc_lsm;
+
+	rc_sfs = securityfs_init();
+	rc_lsm = lsm_initcall(core);
+
+	return (rc_sfs ? rc_sfs : rc_lsm);
 }
 core_initcall(security_initcall_core);
 
diff --git a/security/min_addr.c b/security/min_addr.c
index df1bc643d886..40714bdeefbe 100644
--- a/security/min_addr.c
+++ b/security/min_addr.c
@@ -4,6 +4,8 @@ 
 #include <linux/security.h>
 #include <linux/sysctl.h>
 
+#include "lsm.h"
+
 /* amount of vm to protect from userspace access by both DAC and the LSM*/
 unsigned long mmap_min_addr;
 /* amount of vm to protect from userspace using CAP_SYS_RAWIO (DAC) */
@@ -54,11 +56,10 @@  static const struct ctl_table min_addr_sysctl_table[] = {
 	},
 };
 
-static int __init init_mmap_min_addr(void)
+int __init min_addr_init(void)
 {
 	register_sysctl_init("vm", min_addr_sysctl_table);
 	update_mmap_min_addr();
 
 	return 0;
 }
-pure_initcall(init_mmap_min_addr);