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

[Paul Moore]

On Wed, Apr 9, 2025 at 8:11 PM Kees Cook <kees at kernel.org> wrote:
> 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 at 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.

Yeah, I noticed this when I was reverting that
/lsm_active_cnt/lsm_count/ change and fixed it up to use lsm_idlist[]
which should address that problem.  Later patches convert over to
lsm_idlist[] anyway, which is likely why I didn't catch this in the
preliminary testing.

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

Good point.  I'm not sure what happened here, it may have been a
victim of an earlier change which I dropped.

> > +/**
> > + * 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;

Sure.

> > -/* 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?

It's a little confusing as lsm_order_append() gets heavily reworked a
couple of patches later in "lsm: cleanup the LSM ordered parsing",
which is essentially this function's end state from a logic
perspective.  I think the best thing to do might be to squash those
two patches ... lemme see how ugly that ends up ...

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

I can look into doing that, let me try the squashing first.

-- 
paul-moore.com