| Message ID | 20250409185019.238841-35-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rework the LSM initialization | expand |
On Wed, Apr 09, 2025 at 02:49:49PM -0400, Paul Moore wrote: > One part of a larger effort to cleanup the LSM framework initialization > code. Again, needs a better commit log. > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/lsm_init.c | 94 +++++++++++++++++---------------------------- > 1 file changed, 36 insertions(+), 58 deletions(-) > > diff --git a/security/lsm_init.c b/security/lsm_init.c > index dffa8dc2da36..407429688f1b 100644 > --- a/security/lsm_init.c > +++ b/security/lsm_init.c > @@ -32,6 +32,12 @@ static __initdata bool debug; > pr_info(__VA_ARGS__); \ > } while (0) > > +#define lsm_order_for_each(iter) \ > + for ((iter) = ordered_lsms; *(iter); (iter)++) > +#define lsm_early_for_each_raw(iter) \ > + for ((iter) = __start_early_lsm_info; \ > + (iter) < __end_early_lsm_info; (iter)++) The longer I look at this patch the longer I think it needs to be broken up into a few separate patches, but they would be relatively small, like this one: replace iter loops with iter macros. > + > static int lsm_append(const char *new, char **result); > > /* Save user chosen LSM */ > @@ -96,9 +102,10 @@ static bool __init exists_ordered_lsm(struct lsm_info *lsm) > { > struct lsm_info **check; > > - for (check = ordered_lsms; *check; check++) > + lsm_order_for_each(check) { > if (*check == lsm) > return true; > + } > > return false; > } > @@ -279,56 +286,13 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) > kfree(sep); > } > > -static void __init report_lsm_order(void) > -{ > - struct lsm_info **lsm, *early; > - int first = 0; > - > - pr_info("initializing lsm="); > - > - /* Report each enabled LSM name, comma separated. */ > - for (early = __start_early_lsm_info; > - early < __end_early_lsm_info; early++) > - if (is_enabled(early)) > - pr_cont("%s%s", first++ == 0 ? "" : ",", early->name); > - for (lsm = ordered_lsms; *lsm; lsm++) > - if (is_enabled(*lsm)) > - pr_cont("%s%s", first++ == 0 ? "" : ",", (*lsm)->name); > - > - pr_cont("\n"); > -} > - > /** > - * lsm_early_cred - during initialization allocate a composite cred blob > - * @cred: the cred that needs a blob > - * > - * Allocate the cred blob for all the modules > + * lsm_init_ordered - Initialize the ordered LSMs > */ > -static void __init lsm_early_cred(struct cred *cred) > -{ > - int rc = lsm_cred_alloc(cred, GFP_KERNEL); > - > - if (rc) > - panic("%s: Early cred alloc failed.\n", __func__); > -} > - > -/** > - * lsm_early_task - during initialization allocate a composite task blob > - * @task: the task that needs a blob > - * > - * Allocate the task blob for all the modules > - */ > -static void __init lsm_early_task(struct task_struct *task) > -{ > - int rc = lsm_task_alloc(task); > - > - if (rc) > - panic("%s: Early task alloc failed.\n", __func__); > -} > - > -static void __init ordered_lsm_init(void) > +static void __init lsm_init_ordered(void) > { > struct lsm_info **lsm; > + struct lsm_info *early; > > if (chosen_lsm_order) { > if (chosen_major_lsm) { > @@ -340,10 +304,23 @@ static void __init ordered_lsm_init(void) > } else > ordered_lsm_parse(builtin_lsm_order, "builtin"); > > - for (lsm = ordered_lsms; *lsm; lsm++) > + lsm_order_for_each(lsm) { > lsm_prep_single(*lsm); > + } > > - report_lsm_order(); > + pr_info("initializing lsm="); > + lsm_early_for_each_raw(early) { > + if (is_enabled(early)) > + pr_cont("%s%s", > + early == __start_early_lsm_info ? "" : ",", > + early->name); > + } > + lsm_order_for_each(lsm) { > + if (is_enabled(*lsm)) > + pr_cont("%s%s", > + lsm == ordered_lsms ? "" : ",", (*lsm)->name); > + } report_lsm_order()'s use of "first" needs to stay here or you don't get the right comma/no-comma behavior. It's not about the lsm, it's about whether "first" got incremented. Perhaps "count" might be a better name for "first". > + pr_cont("\n"); > > init_debug("cred blob size = %d\n", blob_sizes.lbs_cred); > init_debug("file blob size = %d\n", blob_sizes.lbs_file); > @@ -362,9 +339,6 @@ static void __init ordered_lsm_init(void) > init_debug("xattr slots = %d\n", blob_sizes.lbs_xattr_count); > init_debug("bdev blob size = %d\n", blob_sizes.lbs_bdev); > > - /* > - * Create any kmem_caches needed for blobs > - */ > if (blob_sizes.lbs_file) > lsm_file_cache = kmem_cache_create("lsm_file_cache", > blob_sizes.lbs_file, 0, > @@ -374,10 +348,14 @@ static void __init ordered_lsm_init(void) > blob_sizes.lbs_inode, 0, > SLAB_PANIC, NULL); > > - lsm_early_cred((struct cred *) current->cred); > - lsm_early_task(current); > - for (lsm = ordered_lsms; *lsm; lsm++) > + if (lsm_cred_alloc((struct cred *)current->cred, GFP_KERNEL)) > + panic("%s: early cred alloc failed.\n", __func__); > + if (lsm_task_alloc(current)) > + panic("%s: early task alloc failed.\n", __func__); > + > + lsm_order_for_each(lsm) { > initialize_lsm(*lsm); > + } > } > > static bool match_last_lsm(const char *list, const char *lsm) > @@ -479,7 +457,7 @@ int __init early_security_init(void) > { > struct lsm_info *lsm; > > - for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) { > + lsm_early_for_each_raw(lsm) { > if (!lsm->enabled) > lsm->enabled = &lsm_enabled_true; > lsm_prep_single(lsm); > @@ -506,7 +484,7 @@ int __init security_init(void) > * Append the names of the early LSM modules now that kmalloc() is > * available > */ > - for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) { > + lsm_early_for_each_raw(lsm) { > init_debug(" early started: %s (%s)\n", lsm->name, > is_enabled(lsm) ? "enabled" : "disabled"); > if (lsm->enabled) > @@ -514,7 +492,7 @@ int __init security_init(void) > } > > /* Load LSMs in specified order. */ > - ordered_lsm_init(); > + lsm_init_ordered(); > > return 0; > } > -- > 2.49.0 > Other stuff seems good.
On Wed, Apr 9, 2025 at 5:38 PM Kees Cook <kees@kernel.org> wrote: > > On Wed, Apr 09, 2025 at 02:49:49PM -0400, Paul Moore wrote: > > One part of a larger effort to cleanup the LSM framework initialization > > code. > > Again, needs a better commit log. See my previous comments as well as the cover letter for the reason why. > > diff --git a/security/lsm_init.c b/security/lsm_init.c > > index dffa8dc2da36..407429688f1b 100644 > > --- a/security/lsm_init.c > > +++ b/security/lsm_init.c > > @@ -32,6 +32,12 @@ static __initdata bool debug; > > pr_info(__VA_ARGS__); \ > > } while (0) > > > > +#define lsm_order_for_each(iter) \ > > + for ((iter) = ordered_lsms; *(iter); (iter)++) > > +#define lsm_early_for_each_raw(iter) \ > > + for ((iter) = __start_early_lsm_info; \ > > + (iter) < __end_early_lsm_info; (iter)++) > > The longer I look at this patch the longer I think it needs to be broken > up into a few separate patches, but they would be relatively small, like > this one: replace iter loops with iter macros. Fair point, done. > > @@ -340,10 +304,23 @@ static void __init ordered_lsm_init(void) > > } else > > ordered_lsm_parse(builtin_lsm_order, "builtin"); > > > > - for (lsm = ordered_lsms; *lsm; lsm++) > > + lsm_order_for_each(lsm) { > > lsm_prep_single(*lsm); > > + } > > > > - report_lsm_order(); > > + pr_info("initializing lsm="); > > + lsm_early_for_each_raw(early) { > > + if (is_enabled(early)) > > + pr_cont("%s%s", > > + early == __start_early_lsm_info ? "" : ",", > > + early->name); > > + } > > + lsm_order_for_each(lsm) { > > + if (is_enabled(*lsm)) > > + pr_cont("%s%s", > > + lsm == ordered_lsms ? "" : ",", (*lsm)->name); > > + } > > report_lsm_order()'s use of "first" needs to stay here or you don't get > the right comma/no-comma behavior. It's not about the lsm, it's about > whether "first" got incremented. Perhaps "count" might be a better name > for "first". Sure, I'll just put the "first" code back, it all gets changed later in the patchset anyway, no need to worry about long term stuff in this snippet.
diff --git a/security/lsm_init.c b/security/lsm_init.c index dffa8dc2da36..407429688f1b 100644 --- a/security/lsm_init.c +++ b/security/lsm_init.c @@ -32,6 +32,12 @@ static __initdata bool debug; pr_info(__VA_ARGS__); \ } while (0) +#define lsm_order_for_each(iter) \ + for ((iter) = ordered_lsms; *(iter); (iter)++) +#define lsm_early_for_each_raw(iter) \ + for ((iter) = __start_early_lsm_info; \ + (iter) < __end_early_lsm_info; (iter)++) + static int lsm_append(const char *new, char **result); /* Save user chosen LSM */ @@ -96,9 +102,10 @@ static bool __init exists_ordered_lsm(struct lsm_info *lsm) { struct lsm_info **check; - for (check = ordered_lsms; *check; check++) + lsm_order_for_each(check) { if (*check == lsm) return true; + } return false; } @@ -279,56 +286,13 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) kfree(sep); } -static void __init report_lsm_order(void) -{ - struct lsm_info **lsm, *early; - int first = 0; - - pr_info("initializing lsm="); - - /* Report each enabled LSM name, comma separated. */ - for (early = __start_early_lsm_info; - early < __end_early_lsm_info; early++) - if (is_enabled(early)) - pr_cont("%s%s", first++ == 0 ? "" : ",", early->name); - for (lsm = ordered_lsms; *lsm; lsm++) - if (is_enabled(*lsm)) - pr_cont("%s%s", first++ == 0 ? "" : ",", (*lsm)->name); - - pr_cont("\n"); -} - /** - * lsm_early_cred - during initialization allocate a composite cred blob - * @cred: the cred that needs a blob - * - * Allocate the cred blob for all the modules + * lsm_init_ordered - Initialize the ordered LSMs */ -static void __init lsm_early_cred(struct cred *cred) -{ - int rc = lsm_cred_alloc(cred, GFP_KERNEL); - - if (rc) - panic("%s: Early cred alloc failed.\n", __func__); -} - -/** - * lsm_early_task - during initialization allocate a composite task blob - * @task: the task that needs a blob - * - * Allocate the task blob for all the modules - */ -static void __init lsm_early_task(struct task_struct *task) -{ - int rc = lsm_task_alloc(task); - - if (rc) - panic("%s: Early task alloc failed.\n", __func__); -} - -static void __init ordered_lsm_init(void) +static void __init lsm_init_ordered(void) { struct lsm_info **lsm; + struct lsm_info *early; if (chosen_lsm_order) { if (chosen_major_lsm) { @@ -340,10 +304,23 @@ static void __init ordered_lsm_init(void) } else ordered_lsm_parse(builtin_lsm_order, "builtin"); - for (lsm = ordered_lsms; *lsm; lsm++) + lsm_order_for_each(lsm) { lsm_prep_single(*lsm); + } - report_lsm_order(); + pr_info("initializing lsm="); + lsm_early_for_each_raw(early) { + if (is_enabled(early)) + pr_cont("%s%s", + early == __start_early_lsm_info ? "" : ",", + early->name); + } + lsm_order_for_each(lsm) { + if (is_enabled(*lsm)) + pr_cont("%s%s", + lsm == ordered_lsms ? "" : ",", (*lsm)->name); + } + pr_cont("\n"); init_debug("cred blob size = %d\n", blob_sizes.lbs_cred); init_debug("file blob size = %d\n", blob_sizes.lbs_file); @@ -362,9 +339,6 @@ static void __init ordered_lsm_init(void) init_debug("xattr slots = %d\n", blob_sizes.lbs_xattr_count); init_debug("bdev blob size = %d\n", blob_sizes.lbs_bdev); - /* - * Create any kmem_caches needed for blobs - */ if (blob_sizes.lbs_file) lsm_file_cache = kmem_cache_create("lsm_file_cache", blob_sizes.lbs_file, 0, @@ -374,10 +348,14 @@ static void __init ordered_lsm_init(void) blob_sizes.lbs_inode, 0, SLAB_PANIC, NULL); - lsm_early_cred((struct cred *) current->cred); - lsm_early_task(current); - for (lsm = ordered_lsms; *lsm; lsm++) + if (lsm_cred_alloc((struct cred *)current->cred, GFP_KERNEL)) + panic("%s: early cred alloc failed.\n", __func__); + if (lsm_task_alloc(current)) + panic("%s: early task alloc failed.\n", __func__); + + lsm_order_for_each(lsm) { initialize_lsm(*lsm); + } } static bool match_last_lsm(const char *list, const char *lsm) @@ -479,7 +457,7 @@ int __init early_security_init(void) { struct lsm_info *lsm; - for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) { + lsm_early_for_each_raw(lsm) { if (!lsm->enabled) lsm->enabled = &lsm_enabled_true; lsm_prep_single(lsm); @@ -506,7 +484,7 @@ int __init security_init(void) * Append the names of the early LSM modules now that kmalloc() is * available */ - for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) { + lsm_early_for_each_raw(lsm) { init_debug(" early started: %s (%s)\n", lsm->name, is_enabled(lsm) ? "enabled" : "disabled"); if (lsm->enabled) @@ -514,7 +492,7 @@ int __init security_init(void) } /* Load LSMs in specified order. */ - ordered_lsm_init(); + lsm_init_ordered(); return 0; }
One part of a larger effort to cleanup the LSM framework initialization code. Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/lsm_init.c | 94 +++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 58 deletions(-)