Re: [RFC PATCH 09/29] lsm: cleanup and normalize the LSM enabled functions

[Kees Cook <kees@kernel.org>]

On Wed, Apr 09, 2025 at 02:49:54PM -0400, Paul Moore wrote:
> One part of a larger effort to cleanup the LSM framework initialization
> code.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  security/inode.c    |   9 ++--
>  security/lsm_init.c | 110 ++++++++++++++++++++++++--------------------
>  2 files changed, 63 insertions(+), 56 deletions(-)
> 
> diff --git a/security/inode.c b/security/inode.c
> index 49bc3578bd23..f687e22e6809 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -351,18 +351,17 @@ static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count,
>  
>  	for (i = 0; i < lsm_count; i++)
>  		/* the '+ 1' accounts for either a comma or a NUL terminator */
> -		len += strlen(lsm_order[i]->id->name) + 1;
> +		len += strlen(lsm_idlist[i]->name) + 1;
>  
>  	str = kmalloc(len, GFP_KERNEL);
>  	if (!str)
>  		return -ENOMEM;
>  	str[0] = '\0';
>  
> -	i = 0;
> -	while (i < lsm_count) {
> -		strcat(str, lsm_order[i]->id->name);
> -		if (++i < lsm_count)
> +	for (i = 0; i < lsm_count; i++) {
> +		if (i > 0)
>  			strcat(str, ",");
> +		strcat(str, lsm_idlist[i]->name);
>  	}
>  
>  	rc = simple_read_from_buffer(buf, count, ppos, str, len);

This chunk needs to be folded into the lsm_names changing patch, I
think. I missed this on the first pass, but lsm_order can never be used
here because lsm_order is initdata -- it will be thrown away after init
is done.

> diff --git a/security/lsm_init.c b/security/lsm_init.c
> index 978bb81b58fa..7f2bc8c22ce9 100644
> --- a/security/lsm_init.c
> +++ b/security/lsm_init.c
> @@ -10,6 +10,10 @@
>  
>  #include "lsm.h"
>  
> +/* LSM enabled constants. */
> +int lsm_enabled_true = 1;
> +int lsm_enabled_false = 0;

Why are these losing static and __initdata? It looks like they're
staying assigned to the __init-marked lsm_info instances.

> +
>  /* Pointers to LSM sections defined in include/asm-generic/vmlinux.lds.h */
>  extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
>  extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
> @@ -72,41 +76,42 @@ static int __init lsm_debug_enable(char *str)
>  }
>  __setup("lsm.debug", lsm_debug_enable);
>  
> -/* Mark an LSM's enabled flag. */
> -static int lsm_enabled_true __initdata = 1;
> -static int lsm_enabled_false __initdata = 0;
> -static void __init set_enabled(struct lsm_info *lsm, bool enabled)
> +/**
> + * lsm_enabled_set - Mark a LSM as enabled
> + * @lsm: LSM definition
> + * @enabled: enabled flag
> + */
> +static void __init lsm_enabled_set(struct lsm_info *lsm, bool enabled)
>  {
>  	/*
>  	 * When an LSM hasn't configured an enable variable, we can use
>  	 * a hard-coded location for storing the default enabled state.
>  	 */
> -	if (!lsm->enabled) {
> -		if (enabled)
> -			lsm->enabled = &lsm_enabled_true;
> -		else
> -			lsm->enabled = &lsm_enabled_false;
> -	} else if (lsm->enabled == &lsm_enabled_true) {
> -		if (!enabled)
> -			lsm->enabled = &lsm_enabled_false;
> -	} else if (lsm->enabled == &lsm_enabled_false) {
> -		if (enabled)
> -			lsm->enabled = &lsm_enabled_true;
> +	if (!lsm->enabled ||
> +	    lsm->enabled == &lsm_enabled_true ||
> +	    lsm->enabled == &lsm_enabled_false) {
> +		lsm->enabled = enabled ? &lsm_enabled_true : &lsm_enabled_false;
>  	} else {
>  		*lsm->enabled = enabled;
>  	}
>  }

Good logic folding.

>  
> -static inline bool is_enabled(struct lsm_info *lsm)
> +/**
> + * lsm_is_enabled - Determine if a LSM is enabled
> + * @lsm: LSM definition
> + */
> +static inline bool lsm_is_enabled(struct lsm_info *lsm)
>  {
>  	if (!lsm->enabled)
>  		return false;
> -
>  	return *lsm->enabled;
>  }

This could be one-lined, actually:

	return lsm->enabled ? *lsm->enabled : false;

>  
> -/* Is an LSM already listed in the ordered LSMs list? */
> -static bool __init exists_ordered_lsm(struct lsm_info *lsm)
> +/**
> + * lsm_order_exists - Determine if a LSM exists in the ordered list
> + * @lsm: LSM definition
> + */
> +static bool __init lsm_order_exists(struct lsm_info *lsm)
>  {
>  	struct lsm_info **check;
>  
> @@ -118,25 +123,29 @@ static bool __init exists_ordered_lsm(struct lsm_info *lsm)
>  	return false;
>  }
>  
> -/* Append an LSM to the list of ordered LSMs to initialize. */
> -static int last_lsm __initdata;
> -static void __init append_ordered_lsm(struct lsm_info *lsm, const char *from)
> +/**
> + * lsm_order_append - Append a LSM to the ordered list
> + * @lsm: LSM definition
> + * @src: source of the addition
> + */
> +static void __init lsm_order_append(struct lsm_info *lsm, const char *src)
>  {
>  	/* Ignore duplicate selections. */
> -	if (exists_ordered_lsm(lsm))
> +	if (lsm_order_exists(lsm))
>  		return;
>  
> -	if (WARN(last_lsm == MAX_LSM_COUNT, "%s: out of LSM static calls!?\n", from))
> -		return;
> +	/* Skip explicitly disabled LSMs. */
> +	if (lsm->enabled && !lsm_is_enabled(lsm)) {
> +		if (WARN(lsm_count == MAX_LSM_COUNT,
> +			 "%s: out of LSM static calls!?\n", src))
> +			return;
> +		lsm_enabled_set(lsm, true);
> +		lsm_order[lsm_count] = lsm;
> +		lsm_idlist[lsm_count++] = lsm->id;
> +	}
>  
> -	/* Enable this LSM, if it is not already set. */
> -	if (!lsm->enabled)
> -		lsm->enabled = &lsm_enabled_true;
> -	lsm_order[last_lsm] = lsm;
> -	lsm_idlist[last_lsm++] = lsm->id;

I don't understand the logic change here. I may be missing something (it
feels like a lot of logic changes mixed together again), but this logic:

     /* Enable this LSM, if it is not already set. */
     if (!lsm->enabled)
             lsm->enabled = &lsm_enabled_true;

seems like it has gone missing now? And I think the last_lsm/lsm_count
changes need to be in the "lsm: rework lsm_active_cnt and lsm_idlist[]"
patch? I'm really struggling to follow this patch, but maybe I am EOD.
:P


> -
> -	init_debug("%s ordered: %s (%s)\n", from, lsm->id->name,
> -		   is_enabled(lsm) ? "enabled" : "disabled");
> +	init_debug("%s ordered: %s (%s)\n", src, lsm->id->name,
> +		   lsm_is_enabled(lsm) ? "enabled" : "disabled");
>  }
>  
>  static void __init lsm_set_blob_size(int *need, int *lbs)
> @@ -159,17 +168,17 @@ static void __init lsm_prep_single(struct lsm_info *lsm)
>  {
>  	struct lsm_blob_sizes *blobs;
>  
> -	if (!is_enabled(lsm)) {
> -		set_enabled(lsm, false);
> +	if (!lsm_is_enabled(lsm)) {
> +		lsm_enabled_set(lsm, false);
>  		return;
>  	} else if ((lsm->flags & LSM_FLAG_EXCLUSIVE) && lsm_exclusive) {
>  		init_debug("exclusive disabled: %s\n", lsm->id->name);
> -		set_enabled(lsm, false);
> +		lsm_enabled_set(lsm, false);
>  		return;
>  	}
>  
>  	/* Mark the LSM as enabled. */
> -	set_enabled(lsm, true);
> +	lsm_enabled_set(lsm, true);
>  	if ((lsm->flags & LSM_FLAG_EXCLUSIVE) && !lsm_exclusive) {
>  		init_debug("exclusive chosen:   %s\n", lsm->id->name);
>  		lsm_exclusive = lsm;
> @@ -200,7 +209,7 @@ static void __init lsm_prep_single(struct lsm_info *lsm)
>  /* Initialize a given LSM, if it is enabled. */
>  static void __init initialize_lsm(struct lsm_info *lsm)
>  {
> -	if (is_enabled(lsm)) {
> +	if (lsm_is_enabled(lsm)) {
>  		int ret;
>  
>  		init_debug("initializing %s\n", lsm->id->name);
> @@ -218,7 +227,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  	/* LSM_ORDER_FIRST is always first. */
>  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
>  		if (lsm->order == LSM_ORDER_FIRST)
> -			append_ordered_lsm(lsm, "  first");
> +			lsm_order_append(lsm, "  first");
>  	}
>  
>  	/* Process "security=", if given. */
> @@ -235,7 +244,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  		     major++) {
>  			if ((major->flags & LSM_FLAG_LEGACY_MAJOR) &&
>  			    strcmp(major->id->name, lsm_order_legacy) != 0) {
> -				set_enabled(major, false);
> +				lsm_enabled_set(major, false);
>  				init_debug("security=%s disabled: %s (only one legacy major LSM)\n",
>  					   lsm_order_legacy, major->id->name);
>  			}
> @@ -251,7 +260,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  		for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
>  			if (strcmp(lsm->id->name, name) == 0) {
>  				if (lsm->order == LSM_ORDER_MUTABLE)
> -					append_ordered_lsm(lsm, origin);
> +					lsm_order_append(lsm, origin);
>  				found = true;
>  			}
>  		}
> @@ -264,24 +273,24 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  	/* Process "security=", if given. */
>  	if (lsm_order_legacy) {
>  		for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
> -			if (exists_ordered_lsm(lsm))
> +			if (lsm_order_exists(lsm))
>  				continue;
>  			if (strcmp(lsm->id->name, lsm_order_legacy) == 0)
> -				append_ordered_lsm(lsm, "security=");
> +				lsm_order_append(lsm, "security=");
>  		}
>  	}
>  
>  	/* LSM_ORDER_LAST is always last. */
>  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
>  		if (lsm->order == LSM_ORDER_LAST)
> -			append_ordered_lsm(lsm, "   last");
> +			lsm_order_append(lsm, "   last");
>  	}
>  
>  	/* Disable all LSMs not in the ordered list. */
>  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
> -		if (exists_ordered_lsm(lsm))
> +		if (lsm_order_exists(lsm))
>  			continue;
> -		set_enabled(lsm, false);
> +		lsm_enabled_set(lsm, false);
>  		init_debug("%s skipped: %s (not in requested order)\n",
>  			   origin, lsm->id->name);
>  	}
> @@ -313,13 +322,13 @@ static void __init lsm_init_ordered(void)
>  
>  	pr_info("initializing lsm=");
>  	lsm_early_for_each_raw(early) {
> -		if (is_enabled(early))
> +		if (lsm_is_enabled(early))
>  			pr_cont("%s%s",
>  				early == __start_early_lsm_info ? "" : ",",
>  				early->id->name);
>  	}
>  	lsm_order_for_each(lsm) {
> -		if (is_enabled(*lsm))
> +		if (lsm_is_enabled(*lsm))
>  			pr_cont("%s%s",
>  				lsm == lsm_order ? "" : ",", (*lsm)->id->name);
>  	}
> @@ -404,8 +413,7 @@ int __init early_security_init(void)
>  	struct lsm_info *lsm;
>  
>  	lsm_early_for_each_raw(lsm) {
> -		if (!lsm->enabled)
> -			lsm->enabled = &lsm_enabled_true;
> +		lsm_enabled_set(lsm, true);
>  		lsm_prep_single(lsm);
>  		initialize_lsm(lsm);
>  	}
> @@ -432,7 +440,7 @@ int __init security_init(void)
>  	 */
>  	lsm_early_for_each_raw(lsm) {
>  		init_debug("  early started: %s (%s)\n", lsm->id->name,
> -			   is_enabled(lsm) ? "enabled" : "disabled");
> +			   lsm_is_enabled(lsm) ? "enabled" : "disabled");
>  	}
>  
>  	/* Load LSMs in specified order. */
> -- 
> 2.49.0

The simple renamings looks fine, but would be nicer if they got split
out.

-- 
Kees Cook