Re: [RFC PATCH 04/29] lsm: simplify ordered_lsm_init() and rename to lsm_init_ordered()

[Kees Cook <kees@kernel.org>]

On Wed, Apr 09, 2025 at 02:49:49PM -0400, Paul Moore wrote:
> One part of a larger effort to cleanup the LSM framework initialization
> code.

Again, needs a better commit log.

> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  security/lsm_init.c | 94 +++++++++++++++++----------------------------
>  1 file changed, 36 insertions(+), 58 deletions(-)
> 
> diff --git a/security/lsm_init.c b/security/lsm_init.c
> index dffa8dc2da36..407429688f1b 100644
> --- a/security/lsm_init.c
> +++ b/security/lsm_init.c
> @@ -32,6 +32,12 @@ static __initdata bool debug;
>  			pr_info(__VA_ARGS__);				\
>  	} while (0)
>  
> +#define lsm_order_for_each(iter)					\
> +	for ((iter) = ordered_lsms; *(iter); (iter)++)
> +#define lsm_early_for_each_raw(iter)					\
> +	for ((iter) = __start_early_lsm_info;				\
> +	     (iter) < __end_early_lsm_info; (iter)++)

The longer I look at this patch the longer I think it needs to be broken
up into a few separate patches, but they would be relatively small, like
this one: replace iter loops with iter macros.

> +
>  static int lsm_append(const char *new, char **result);
>  
>  /* Save user chosen LSM */
> @@ -96,9 +102,10 @@ static bool __init exists_ordered_lsm(struct lsm_info *lsm)
>  {
>  	struct lsm_info **check;
>  
> -	for (check = ordered_lsms; *check; check++)
> +	lsm_order_for_each(check) {
>  		if (*check == lsm)
>  			return true;
> +	}
>  
>  	return false;
>  }
> @@ -279,56 +286,13 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  	kfree(sep);
>  }
>  
> -static void __init report_lsm_order(void)
> -{
> -	struct lsm_info **lsm, *early;
> -	int first = 0;
> -
> -	pr_info("initializing lsm=");
> -
> -	/* Report each enabled LSM name, comma separated. */
> -	for (early = __start_early_lsm_info;
> -	     early < __end_early_lsm_info; early++)
> -		if (is_enabled(early))
> -			pr_cont("%s%s", first++ == 0 ? "" : ",", early->name);
> -	for (lsm = ordered_lsms; *lsm; lsm++)
> -		if (is_enabled(*lsm))
> -			pr_cont("%s%s", first++ == 0 ? "" : ",", (*lsm)->name);
> -
> -	pr_cont("\n");
> -}
> -
>  /**
> - * lsm_early_cred - during initialization allocate a composite cred blob
> - * @cred: the cred that needs a blob
> - *
> - * Allocate the cred blob for all the modules
> + * lsm_init_ordered - Initialize the ordered LSMs
>   */
> -static void __init lsm_early_cred(struct cred *cred)
> -{
> -	int rc = lsm_cred_alloc(cred, GFP_KERNEL);
> -
> -	if (rc)
> -		panic("%s: Early cred alloc failed.\n", __func__);
> -}
> -
> -/**
> - * lsm_early_task - during initialization allocate a composite task blob
> - * @task: the task that needs a blob
> - *
> - * Allocate the task blob for all the modules
> - */
> -static void __init lsm_early_task(struct task_struct *task)
> -{
> -	int rc = lsm_task_alloc(task);
> -
> -	if (rc)
> -		panic("%s: Early task alloc failed.\n", __func__);
> -}
> -
> -static void __init ordered_lsm_init(void)
> +static void __init lsm_init_ordered(void)
>  {
>  	struct lsm_info **lsm;
> +	struct lsm_info *early;
>  
>  	if (chosen_lsm_order) {
>  		if (chosen_major_lsm) {
> @@ -340,10 +304,23 @@ static void __init ordered_lsm_init(void)
>  	} else
>  		ordered_lsm_parse(builtin_lsm_order, "builtin");
>  
> -	for (lsm = ordered_lsms; *lsm; lsm++)
> +	lsm_order_for_each(lsm) {
>  		lsm_prep_single(*lsm);
> +	}
>  
> -	report_lsm_order();
> +	pr_info("initializing lsm=");
> +	lsm_early_for_each_raw(early) {
> +		if (is_enabled(early))
> +			pr_cont("%s%s",
> +				early == __start_early_lsm_info ? "" : ",",
> +				early->name);
> +	}
> +	lsm_order_for_each(lsm) {
> +		if (is_enabled(*lsm))
> +			pr_cont("%s%s",
> +				lsm == ordered_lsms ? "" : ",", (*lsm)->name);
> +	}

report_lsm_order()'s use of "first" needs to stay here or you don't get
the right comma/no-comma behavior. It's not about the lsm, it's about
whether "first" got incremented. Perhaps "count" might be a better name
for "first".

> +	pr_cont("\n");
>  
>  	init_debug("cred blob size       = %d\n", blob_sizes.lbs_cred);
>  	init_debug("file blob size       = %d\n", blob_sizes.lbs_file);
> @@ -362,9 +339,6 @@ static void __init ordered_lsm_init(void)
>  	init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr_count);
>  	init_debug("bdev blob size       = %d\n", blob_sizes.lbs_bdev);
>  
> -	/*
> -	 * Create any kmem_caches needed for blobs
> -	 */
>  	if (blob_sizes.lbs_file)
>  		lsm_file_cache = kmem_cache_create("lsm_file_cache",
>  						   blob_sizes.lbs_file, 0,
> @@ -374,10 +348,14 @@ static void __init ordered_lsm_init(void)
>  						    blob_sizes.lbs_inode, 0,
>  						    SLAB_PANIC, NULL);
>  
> -	lsm_early_cred((struct cred *) current->cred);
> -	lsm_early_task(current);
> -	for (lsm = ordered_lsms; *lsm; lsm++)
> +	if (lsm_cred_alloc((struct cred *)current->cred, GFP_KERNEL))
> +		panic("%s: early cred alloc failed.\n", __func__);
> +	if (lsm_task_alloc(current))
> +		panic("%s: early task alloc failed.\n", __func__);
> +
> +	lsm_order_for_each(lsm) {
>  		initialize_lsm(*lsm);
> +	}
>  }
>  
>  static bool match_last_lsm(const char *list, const char *lsm)
> @@ -479,7 +457,7 @@ int __init early_security_init(void)
>  {
>  	struct lsm_info *lsm;
>  
> -	for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) {
> +	lsm_early_for_each_raw(lsm) {
>  		if (!lsm->enabled)
>  			lsm->enabled = &lsm_enabled_true;
>  		lsm_prep_single(lsm);
> @@ -506,7 +484,7 @@ int __init security_init(void)
>  	 * Append the names of the early LSM modules now that kmalloc() is
>  	 * available
>  	 */
> -	for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) {
> +	lsm_early_for_each_raw(lsm) {
>  		init_debug("  early started: %s (%s)\n", lsm->name,
>  			   is_enabled(lsm) ? "enabled" : "disabled");
>  		if (lsm->enabled)
> @@ -514,7 +492,7 @@ int __init security_init(void)
>  	}
>  
>  	/* Load LSMs in specified order. */
> -	ordered_lsm_init();
> +	lsm_init_ordered();
>  
>  	return 0;
>  }
> -- 
> 2.49.0
> 

Other stuff seems good.

-- 
Kees Cook