On Wed, Apr 9, 2025 at 8:11 PM Kees Cook <k...@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 <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.
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
