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 <p...@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
