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:
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 <p...@paul-moore.com>
> ---
> security/lsm_init.c | 103 ++++++++++++++++++--------------------------
> 1 file changed, 43 insertions(+), 60 deletions(-)
>
> diff --git a/security/lsm_init.c b/security/lsm_init.c
> index 70e7d4207dae..dffa8dc2da36 100644
> --- a/security/lsm_init.c
> +++ b/security/lsm_init.c
> @@ -123,22 +123,6 @@ static void __init append_ordered_lsm(struct lsm_info
> *lsm, const char *from)
> is_enabled(lsm) ? "enabled" : "disabled");
> }
>
> -/* Is an LSM allowed to be initialized? */
> -static bool __init lsm_allowed(struct lsm_info *lsm)
> -{
> - /* Skip if the LSM is disabled. */
> - if (!is_enabled(lsm))
> - return false;
> -
> - /* Not allowed if another exclusive LSM already initialized. */
> - if ((lsm->flags & LSM_FLAG_EXCLUSIVE) && exclusive) {
> - init_debug("exclusive disabled: %s\n", lsm->name);
> - return false;
> - }
> -
> - return true;
> -}
> -
> static void __init lsm_set_blob_size(int *need, int *lbs)
> {
> int offset;
> @@ -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"?
> {
> - if (!needed)
> + struct lsm_blob_sizes *blobs;
> +
> + if (!is_enabled(lsm)) {
> + set_enabled(lsm, false);
> + return;
> + } else if ((lsm->flags & LSM_FLAG_EXCLUSIVE) && exclusive) {
> + init_debug("exclusive disabled: %s\n", lsm->name);
> + set_enabled(lsm, false);
> return;
> -
> - lsm_set_blob_size(&needed->lbs_cred, &blob_sizes.lbs_cred);
> - lsm_set_blob_size(&needed->lbs_file, &blob_sizes.lbs_file);
> - lsm_set_blob_size(&needed->lbs_ib, &blob_sizes.lbs_ib);
> - /*
> - * The inode blob gets an rcu_head in addition to
> - * what the modules might need.
> - */
> - if (needed->lbs_inode && blob_sizes.lbs_inode == 0)
> - blob_sizes.lbs_inode = sizeof(struct rcu_head);
> - lsm_set_blob_size(&needed->lbs_inode, &blob_sizes.lbs_inode);
> - lsm_set_blob_size(&needed->lbs_ipc, &blob_sizes.lbs_ipc);
> - lsm_set_blob_size(&needed->lbs_key, &blob_sizes.lbs_key);
> - lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> - lsm_set_blob_size(&needed->lbs_perf_event, &blob_sizes.lbs_perf_event);
> - lsm_set_blob_size(&needed->lbs_sock, &blob_sizes.lbs_sock);
> - lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
> - lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
> - lsm_set_blob_size(&needed->lbs_tun_dev, &blob_sizes.lbs_tun_dev);
> - lsm_set_blob_size(&needed->lbs_xattr_count,
> - &blob_sizes.lbs_xattr_count);
> - lsm_set_blob_size(&needed->lbs_bdev, &blob_sizes.lbs_bdev);
> -}
> -
> -/* Prepare LSM for initialization. */
> -static void __init prepare_lsm(struct lsm_info *lsm)
> -{
> - int enabled = lsm_allowed(lsm);
> -
> - /* Record enablement (to handle any following exclusive LSMs). */
> - set_enabled(lsm, enabled);
> -
> - /* If enabled, do pre-initialization work. */
> - if (enabled) {
> - if ((lsm->flags & LSM_FLAG_EXCLUSIVE) && !exclusive) {
> - exclusive = lsm;
> - init_debug("exclusive chosen: %s\n", lsm->name);
> - }
> -
> - lsm_set_blob_sizes(lsm->blobs);
> }
> +
> + /* Mark the LSM as enabled. */
> + set_enabled(lsm, true);
> + if ((lsm->flags & LSM_FLAG_EXCLUSIVE) && !exclusive) {
> + init_debug("exclusive chosen: %s\n", lsm->name);
> + exclusive = lsm;
> + }
> +
> + /* 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]);
> }
>
> /* Initialize a given LSM, if it is enabled. */
> @@ -358,7 +341,7 @@ static void __init ordered_lsm_init(void)
> ordered_lsm_parse(builtin_lsm_order, "builtin");
>
> for (lsm = ordered_lsms; *lsm; lsm++)
> - prepare_lsm(*lsm);
> + lsm_prep_single(*lsm);
>
> report_lsm_order();
>
> @@ -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:
Reviewed-by: Kees Cook <k...@kernel.org>
--
Kees Cook
