[RFC PATCH 03/29] lsm: simplify prepare_lsm() and rename to lsm_prep_single()

[Paul Moore]

On Wed, Apr 9, 2025 at 5:30 PM Kees Cook <kees at kernel.org> wrote:
> On Wed, Apr 09, 2025 at 02:49:48PM -0400, Paul Moore wrote:
> > One part of a larger effort to cleanup the LSM framework initialization
> > code.
>
> This commit log needs improvement. i.e. explain what and why:

Yeah, it's bad, see the cover letter, the commit descriptions were
explicitly mentioned as needing improvement.  You'll likely see a few
other patches with let's just say "less than good" commit
descriptions.  I basically go through and add/edit/fix-up commit
descriptions while things are compiling, tests are running, etc. and
with the number of patches in this patchset I didn't make enough
passes through the list to get beyond some generic text in a lot of
the patches.

I simply wanted to get this patchset out as a RFC as it had been
mentioned to a few people offline and I wanted to get some feedback
from the individual LSM maintainers who might be affected by moving
the initcalls out of the individual LSMs and into the framework.

> The execution flow through lsm_allowed(), prepare_lsm(), and
> lsm_set_blob_sizes() is a bit convoluted. Merge the logic of all three
> into a single new function, lsm_prep_single().
>
> >
> > Signed-off-by: Paul Moore <paul at paul-moore.com>
> > ---
> >  security/lsm_init.c | 103 ++++++++++++++++++--------------------------
> >  1 file changed, 43 insertions(+), 60 deletions(-)

...

> > @@ -151,51 +135,50 @@ static void __init lsm_set_blob_size(int *need, int *lbs)
> >       *need = offset;
> >  }
> >
> > -static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
> > +/**
> > + * lsm_prep_single - Prepare the LSM framework for a new LSM
> > + * @lsm: LSM definition
> > + */
> > +static void __init lsm_prep_single(struct lsm_info *lsm)
>
> Nit-pick on naming: why shorten "prepare"?

My fingers are lazy?

> > +     /* Register the LSM blob sizes. */
> > +     blobs = lsm->blobs;
> > +     lsm_set_blob_size(&blobs->lbs_cred, &blob_sizes.lbs_cred);
> > +     lsm_set_blob_size(&blobs->lbs_file, &blob_sizes.lbs_file);
> > +     lsm_set_blob_size(&blobs->lbs_ib, &blob_sizes.lbs_ib);
> > +     /* inode blob gets an rcu_head in addition to LSM blobs. */
> > +     if (blobs->lbs_inode && blob_sizes.lbs_inode == 0)
> > +             blob_sizes.lbs_inode = sizeof(struct rcu_head);
> > +     lsm_set_blob_size(&blobs->lbs_inode, &blob_sizes.lbs_inode);
> > +     lsm_set_blob_size(&blobs->lbs_ipc, &blob_sizes.lbs_ipc);
> > +     lsm_set_blob_size(&blobs->lbs_key, &blob_sizes.lbs_key);
> > +     lsm_set_blob_size(&blobs->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> > +     lsm_set_blob_size(&blobs->lbs_perf_event, &blob_sizes.lbs_perf_event);
> > +     lsm_set_blob_size(&blobs->lbs_sock, &blob_sizes.lbs_sock);
> > +     lsm_set_blob_size(&blobs->lbs_superblock, &blob_sizes.lbs_superblock);
> > +     lsm_set_blob_size(&blobs->lbs_task, &blob_sizes.lbs_task);
> > +     lsm_set_blob_size(&blobs->lbs_tun_dev, &blob_sizes.lbs_tun_dev);
> > +     lsm_set_blob_size(&blobs->lbs_xattr_count,
> > +                       &blob_sizes.lbs_xattr_count);
> > +     lsm_set_blob_size(&blobs->lbs_bdev, &blob_sizes.lbs_bdev);
>
> Another refactoring idea I saw recently from the sysctl subsystem was
> turning these named "same things" into an array with enum names, so
> instead of &blobs->lbs_ipc, &blobs->lbs_key, they can still have useful
> names but also be iterated in a loop:
>
> enum lsm_blob_types {
>         LSM_BLOB_IPC,
>         LSM_BLOB_KEY,
>         ...
>         LSM_BLOB_MAX
> };
> ...
>         for (i = 0; i < ARRAY_SIZE(blobs->lbs); i++) {
>                 lsm_set_blob_size(&blobs->lbs[i], &blob_sizes[i]);

Cool idea, but let's leave that as future work since it is going to
start stretching across security.c for the allocators (and in xattr
count stuff) and this patchset is already more expansive than I would
like.

> > @@ -499,7 +482,7 @@ int __init early_security_init(void)
> >       for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) {
> >               if (!lsm->enabled)
> >                       lsm->enabled = &lsm_enabled_true;
> > -             prepare_lsm(lsm);
> > +             lsm_prep_single(lsm);
> >               initialize_lsm(lsm);
> >       }
>
> Regardless, this looks correct to me. With or without renaming the
> function:

Thanks, I appreciate the review.

> Reviewed-by: Kees Cook <kees at kernel.org>
>
> --
> Kees Cook

--
paul-moore.com