Date: Mon, 5 Feb 2024 14:39:39 -0800
From: Fan Wu <wufan@...ux.microsoft.com>
To: Paul Moore <paul@...l-moore.com>, corbet@....net, zohar@...ux.ibm.com,
 jmorris@...ei.org, serge@...lyn.com, tytso@....edu, ebiggers@...nel.org,
 axboe@...nel.dk, agk@...hat.com, snitzer@...nel.org, eparis@...hat.com
Cc: linux-doc@...r.kernel.org, linux-integrity@...r.kernel.org,
 linux-security-module@...r.kernel.org, linux-fscrypt@...r.kernel.org,
 linux-block@...r.kernel.org, dm-devel@...ts.linux.dev,
 audit@...r.kernel.org, linux-kernel@...r.kernel.org,
 Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH RFC v12 6/20] ipe: introduce 'boot_verified' as a trust
 provider



On 2/3/2024 2:25 PM, Paul Moore wrote:
> On Jan 30, 2024 Fan Wu <wufan@...ux.microsoft.com> wrote:
>>
>> IPE is designed to provide system level trust guarantees, this usually
>> implies that trust starts from bootup with a hardware root of trust,
>> which validates the bootloader. After this, the bootloader verifies
>> the kernel and the initramfs.
>>
>> As there's no currently supported integrity method for initramfs, and
>> it's typically already verified by the bootloader. This patch introduces
>> a new IPE property `boot_verified` which allows author of IPE policy to
>> indicate trust for files from initramfs.
>>
>> The implementation of this feature utilizes the newly added
>> `unpack_initramfs` hook. This hook marks the superblock of the rootfs
>> after the initramfs has been unpacked into it.
>>
>> Since the rootfs is never unmounted during system operation, it is
>> advised to switch to a different policy that doesn't rely on the
>> `boot_verified` property after the real rootfs is in charge.
>> This ensures that the trust policies remain relevant and effective
>> throughout the system's operation.
> 
> To be clear, most (all?) init systems cleanup the initial rootfs and
> mount another filesystem on top of it during the boot process, correct?
> That should probably be mentioned in the commit description, perhaps
> with references to the pivot_root(2) and switch_root(8) docs.
> 
> While I don't disagree with your recommendation above, I do believe
> it is unnecessarily scary sounding.
> 
I agree more details/references will be helpful. I will rewrite the 
commit message.

>> Signed-off-by: Deven Bowers <deven.desai@...ux.microsoft.com>
>> Signed-off-by: Fan Wu <wufan@...ux.microsoft.com>
>> ---
>> v2:
>>    +No Changes
>>
>> v3:
>>    + Remove useless caching system
>>    + Move ipe_load_properties to this match
>>    + Minor changes from checkpatch --strict warnings
>>
>> v4:
>>    + Remove comments from headers that was missed previously.
>>    + Grammatical corrections.
>>
>> v5:
>>    + No significant changes
>>
>> v6:
>>    + No changes
>>
>> v7:
>>    + Reword and refactor patch 04/12 to [09/16], based on changes in
>> the underlying system.
>>    + Add common audit function for boolean values
>>    + Use common audit function as implementation.
>>
>> v8:
>>    + No changes
>>
>> v9:
>>    + No changes
>>
>> v10:
>>    + Replace struct file with struct super_block
>>
>> v11:
>>    + Fix code style issues
>>
>> v12:
>>    + Switch to use unpack_initramfs hook and security blob
>> ---
>>   security/ipe/eval.c          | 68 +++++++++++++++++++++++++++++++++++-
>>   security/ipe/eval.h          |  9 +++++
>>   security/ipe/hooks.c         |  8 +++++
>>   security/ipe/hooks.h         |  4 +++
>>   security/ipe/ipe.c           | 14 ++++++++
>>   security/ipe/ipe.h           |  3 ++
>>   security/ipe/policy.h        |  2 ++
>>   security/ipe/policy_parser.c | 37 +++++++++++++++++++-
>>   8 files changed, 143 insertions(+), 2 deletions(-)
> 
> More comments below, but one thing I wanted to mention at the top is
> that I believe there is too much conditional compilation depending on
> the state of CONFIG_BLK_DEV_INITRD.  While there is noting wrong about
> this from a correctness perspective, I believe the reality is that
> the vast majority of systems these days are built with this enabled,
> and having all these pre-processor checks adds to the complexity of
> the code.  Additional comments about this below ...
> 
Yes, removing the dependency on the switch can significantly simplify 
the code. I will refactor the code accordingly.

>> diff --git a/security/ipe/eval.c b/security/ipe/eval.c
>> index 4f425afffcad..546bbc52a071 100644
>> --- a/security/ipe/eval.c
>> +++ b/security/ipe/eval.c
>> @@ -16,6 +16,24 @@
>>   
>>   struct ipe_policy __rcu *ipe_active_policy;
>>   
>> +#define FILE_SUPERBLOCK(f) ((f)->f_path.mnt->mnt_sb)
>> +
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +/**
>> + * build_ipe_sb_ctx - Build from_initramfs field of an evaluation context.
>> + * @ctx: Supplies a pointer to the context to be populated.
>> + * @file: Supplies the file struct of the file triggered IPE event.
>> + */
>> +static void build_ipe_sb_ctx(struct ipe_eval_ctx *ctx, const struct file *const file)
>> +{
>> +	ctx->from_initramfs = ipe_sb(FILE_SUPERBLOCK(file))->is_initramfs;
>> +}
>> +#else
>> +static void build_ipe_sb_ctx(struct ipe_eval_ctx *ctx, const struct file *const file)
>> +{
>> +}
>> +#endif /* CONFIG_BLK_DEV_INITRD */
> 
> If you move the @file NULL check into build_ipe_sb_ctx() you can save
> a comparison in the !CONFIG_BLK_DEV_INITRD case:
> 
> #ifdef CONFIG_BLK_DEV_INITRD
> void build(...)
> {
>    if (file)
>      ctx->initramfs = ipe_sb(file);
>    else
>      ctx->initramfs = false;
> }
> #else
> void build(...)
> {
>    ctx->initramfs = false;
> }
> #endif
> 
I agree this can save a comparision if we only have sb_ctx. But there 
are bdev_ctx(for dm-verity) and inode_ctx(for fsverity) will be 
introduced later. So I still think the NULL check should be done at the 
current place. (also the save won't exist if we are always enabling the 
initramfs boolean)

> NOTE: see my comment below about always enabling the initramfs boolean
> in @ipe_eval_ctx and other structs.
> 
>>   /**
>>    * build_eval_ctx - Build an evaluation context.
>>    * @ctx: Supplies a pointer to the context to be populated.
>> @@ -28,8 +46,49 @@ void build_eval_ctx(struct ipe_eval_ctx *ctx,
>>   {
>>   	ctx->file = file;
>>   	ctx->op = op;
>> +
>> +	if (file)
>> +		build_ipe_sb_ctx(ctx, file);
>> +}
> 
> See my comment above regarding the @file NULL check.
> 
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +/**
>> + * evaluate_boot_verified_true - Evaluate @ctx for the boot verified property.
>> + * @ctx: Supplies a pointer to the context being evaluated.
>> + *
>> + * Return:
>> + * * true	- The current @ctx match the @p
>> + * * false	- The current @ctx doesn't match the @p
>> + */
>> +static bool evaluate_boot_verified_true(const struct ipe_eval_ctx *const ctx)
>> +{
>> +	return ctx->from_initramfs;
>>   }
>>   
>> +/**
>> + * evaluate_boot_verified_false - Evaluate @ctx for the boot verified property.
>> + * @ctx: Supplies a pointer to the context being evaluated.
>> + *
>> + * Return:
>> + * * true	- The current @ctx match the @p
>> + * * false	- The current @ctx doesn't match the @p
>> + */
>> +static bool evaluate_boot_verified_false(const struct ipe_eval_ctx *const ctx)
>> +{
>> +	return !evaluate_boot_verified_true(ctx);
>> +}
>> +#else
>> +static bool evaluate_boot_verified_true(const struct ipe_eval_ctx *const ctx)
>> +{
>> +	return false;
>> +}
>> +
>> +static bool evaluate_boot_verified_false(const struct ipe_eval_ctx *const ctx)
>> +{
>> +	return false;
>> +}
>> +#endif /* CONFIG_BLK_DEV_INITRD */
> 
> That is a lot of lines of code just to check a single boolean value.
> I understand the layers of abstraction, but this looks a bit excessive
> to me.  Assuming you agree with the other comments in this email
> regarding always including an initramfs flag in @ipe_eval_ctx, I think
> you could reduce all of the above into one single line function as shown
> below, and just negate it as needed in evaluate_property().
> 
> static bool evaluate_boot_verified(ctx)
> {
>    return ctx->initramfs;
> }
> 
>>   /**
>>    * evaluate_property - Analyze @ctx against a property.
>>    * @ctx: Supplies a pointer to the context to be evaluated.
>> @@ -42,7 +101,14 @@ void build_eval_ctx(struct ipe_eval_ctx *ctx,
>>   static bool evaluate_property(const struct ipe_eval_ctx *const ctx,
>>   			      struct ipe_prop *p)
>>   {
>> -	return false;
>> +	switch (p->type) {
>> +	case IPE_PROP_BOOT_VERIFIED_FALSE:
>> +		return evaluate_boot_verified_false(ctx);
>> +	case IPE_PROP_BOOT_VERIFIED_TRUE:
>> +		return evaluate_boot_verified_true(ctx);
> 
> According to my comment above:
> 
>    case IPE_PROP_BOOT_VERIFIED_FALSE:
>      return !evaludate_boot_verified(ctx);
>    case IPE_PROP_BOOT_VERIFIED_TRUE:
>      return evaludate_boot_verified(ctx);
> 
This looks better, I will update this part accordingly.

>> +	default:
>> +		return false;
>> +	}
>>   }
>>   
>>   /**
>> diff --git a/security/ipe/eval.h b/security/ipe/eval.h
>> index cfdf3c8dfe8a..7d79fdb63bbf 100644
>> --- a/security/ipe/eval.h
>> +++ b/security/ipe/eval.h
>> @@ -15,10 +15,19 @@
>>   
>>   extern struct ipe_policy __rcu *ipe_active_policy;
>>   
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +struct ipe_sb {
>> +	bool is_initramfs;
>> +}
> 
> You've already got a function named "ipe_sb()", I would suggest saving
> us all some headaches by renaming the above to "ipe_superblock" or
> something similar.  The point is to not have a struct and a function
> with the same name.
> 
> I also think you can shorten the field name to "initramfs", it's
> defined as a boolean flag so I'm not sure the "is_" prefix lends any
> useful hints but does make for longer lines, more typing, etc.
>
>> +#endif /* CONFIG_BLK_DEV_INITRD */
>> +
>>   struct ipe_eval_ctx {
>>   	enum ipe_op_type op;
>>   
>>   	const struct file *file;
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +	bool from_initramfs;
>> +#endif /* CONFIG_BLK_DEV_INITRD */
>>   };
> 
> I suppose I understand the desire to make the @from_initramfs
> conditional to potentially reduce the size of @ipe_eval_ctx when it is
> not needed, however, I believe in the vast majority of systems we are
> going to see CONFIG_BLK_DEV_INITRD enabled so I believe this adds a lot
> extra code noise for little practical benefit.
> 
> Similarly to ipe_sb::is_initramfs, I think you can rename this field to
> "initramfs" and we would all be better for the change.
> 

Sure, I can change the name and remove the KCONFIG dependency.
>>   void build_eval_ctx(struct ipe_eval_ctx *ctx, const struct file *file, enum ipe_op_type op);
>> diff --git a/security/ipe/hooks.c b/security/ipe/hooks.c
>> index 3aec88c074e1..8ee105bf7bad 100644
>> --- a/security/ipe/hooks.c
>> +++ b/security/ipe/hooks.c
>> @@ -4,6 +4,7 @@
>>    */
>>   
>>   #include <linux/fs.h>
>> +#include <linux/fs_struct.h>
>>   #include <linux/types.h>
>>   #include <linux/binfmts.h>
>>   #include <linux/mman.h>
>> @@ -181,3 +182,10 @@ int ipe_kernel_load_data(enum kernel_load_data_id id, bool contents)
>>   	build_eval_ctx(&ctx, NULL, op);
>>   	return ipe_evaluate_event(&ctx);
>>   }
>> +
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +void ipe_unpack_initramfs(void)
>> +{
>> +	ipe_sb(current->fs->root.mnt->mnt_sb)->is_initramfs = true;
>> +}
>> +#endif /* CONFIG_BLK_DEV_INITRD */
>> diff --git a/security/ipe/hooks.h b/security/ipe/hooks.h
>> index 23205452f758..3b1bb0a6e89c 100644
>> --- a/security/ipe/hooks.h
>> +++ b/security/ipe/hooks.h
>> @@ -22,4 +22,8 @@ int ipe_kernel_read_file(struct file *file, enum kernel_read_file_id id,
>>   
>>   int ipe_kernel_load_data(enum kernel_load_data_id id, bool contents);
>>   
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +void ipe_unpack_initramfs(void);
>> +#endif /* CONFIG_BLK_DEV_INITRD */
>> +
>>   #endif /* _IPE_HOOKS_H */
>> diff --git a/security/ipe/ipe.c b/security/ipe/ipe.c
>> index 22bd95116087..ed3acf6174d8 100644
>> --- a/security/ipe/ipe.c
>> +++ b/security/ipe/ipe.c
>> @@ -5,9 +5,13 @@
>>   #include <uapi/linux/lsm.h>
>>   
>>   #include "ipe.h"
>> +#include "eval.h"
>>   #include "hooks.h"
>>   
>>   static struct lsm_blob_sizes ipe_blobs __ro_after_init = {
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +	.lbs_superblock = sizeof(struct ipe_sb),
>> +#endif /* CONFIG_BLK_DEV_INITRD */
>>   };
> 
> I would drop the CONFIG_BLK_DEV_INITRD conditional above for reasons
> already mentioned, it's also not like a running system has that many
> superblocks allocated.  The increase in memory usage should be
> trivial.
> 
>>   static const struct lsm_id ipe_lsmid = {
>> @@ -15,12 +19,22 @@ static const struct lsm_id ipe_lsmid = {
>>   	.id = LSM_ID_IPE,
>>   };
>>   
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +struct ipe_sb *ipe_sb(const struct super_block *sb)
>> +{
>> +	return sb->s_security + ipe_blobs.lbs_superblock;
>> +}
>> +#endif /* CONFIG_BLK_DEV_INITRD */
> 
> If we always have an IPE slot in the superblock's security blob, there
> is not need to make the above conditional.
> 
>>   static struct security_hook_list ipe_hooks[] __ro_after_init = {
>>   	LSM_HOOK_INIT(bprm_check_security, ipe_bprm_check_security),
>>   	LSM_HOOK_INIT(mmap_file, ipe_mmap_file),
>>   	LSM_HOOK_INIT(file_mprotect, ipe_file_mprotect),
>>   	LSM_HOOK_INIT(kernel_read_file, ipe_kernel_read_file),
>>   	LSM_HOOK_INIT(kernel_load_data, ipe_kernel_load_data),
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +	LSM_HOOK_INIT(unpack_initramfs_security, ipe_unpack_initramfs),
>> +#endif /* CONFIG_BLK_DEV_INITRD */
>>   };
>>   
>>   /**
>> diff --git a/security/ipe/ipe.h b/security/ipe/ipe.h
>> index a1c68d0fc2e0..f1e7c3222b6d 100644
>> --- a/security/ipe/ipe.h
>> +++ b/security/ipe/ipe.h
>> @@ -12,5 +12,8 @@
>>   #define pr_fmt(fmt) "IPE: " fmt
>>   
>>   #include <linux/lsm_hooks.h>
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +struct ipe_sb *ipe_sb(const struct super_block *sb);
>> +#endif /* CONFIG_BLK_DEV_INITRD */
>>   
>>   #endif /* _IPE_H */
>> diff --git a/security/ipe/policy.h b/security/ipe/policy.h
>> index fb906f41522b..fb48024bb63e 100644
>> --- a/security/ipe/policy.h
>> +++ b/security/ipe/policy.h
>> @@ -30,6 +30,8 @@ enum ipe_action_type {
>>   #define IPE_ACTION_INVALID __IPE_ACTION_MAX
>>   
>>   enum ipe_prop_type {
>> +	IPE_PROP_BOOT_VERIFIED_FALSE,
>> +	IPE_PROP_BOOT_VERIFIED_TRUE,
>>   	__IPE_PROP_MAX
>>   };
>>   
>> diff --git a/security/ipe/policy_parser.c b/security/ipe/policy_parser.c
>> index 612839b405f4..cce15f0eb645 100644
>> --- a/security/ipe/policy_parser.c
>> +++ b/security/ipe/policy_parser.c
>> @@ -265,6 +265,14 @@ static enum ipe_action_type parse_action(char *t)
>>   	return match_token(t, action_tokens, args);
>>   }
>>   
>> +static const match_table_t property_tokens = {
>> +#ifdef CONFIG_BLK_DEV_INITRD
>> +	{IPE_PROP_BOOT_VERIFIED_FALSE,	"boot_verified=FALSE"},
>> +	{IPE_PROP_BOOT_VERIFIED_TRUE,	"boot_verified=TRUE"},
>> +#endif /* CONFIG_BLK_DEV_INITRD */
> 
> You want the "boot_verified" field to be part of the IPE policy
> regardless of the state of CONFIG_BLK_DEV_INITRD, yes?  On a system
> without _INITRD the TRUE case would never trigger, only the FALSE
> case, which seems like the Right Thing.
> 
> It just seems wrong to me to have the policy grammar change depending
> on what the kernel supports, it seems like IPE should parse and enforce
> the policy regardless of the kernel's config, with the understanding
> that some rules might never be satisfied as it would be impossible
> for a given kernel config, e.g. "boot_verified=TRUE" on a non-initramfs
> system.
> 
> I probably should have thought of this sooner, but I believe the same
> applies to the dm-verity and fs-verity policy tokens.
> 
This sounds reasonable to me. I will change the parser to make the 
policy grammar not depending on any kernel config.

Thanks,
Fan

>> +	{IPE_PROP_INVALID,		NULL}
>> +};
>> +
>>   /**
>>    * parse_property - Parse the property type given a token string.
>>    * @t: Supplies the token string to be parsed.
>> @@ -277,7 +285,34 @@ static enum ipe_action_type parse_action(char *t)
>>    */
>>   static int parse_property(char *t, struct ipe_rule *r)
>>   {
>> -	return -EBADMSG;
>> +	substring_t args[MAX_OPT_ARGS];
>> +	struct ipe_prop *p = NULL;
>> +	int rc = 0;
>> +	int token;
>> +
>> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
>> +	if (!p)
>> +		return -ENOMEM;
>> +
>> +	token = match_token(t, property_tokens, args);
>> +
>> +	switch (token) {
>> +	case IPE_PROP_BOOT_VERIFIED_FALSE:
>> +	case IPE_PROP_BOOT_VERIFIED_TRUE:
>> +		p->type = token;
>> +		break;
>> +	default:
>> +		rc = -EBADMSG;
>> +		break;
>> +	}
>> +	if (rc)
>> +		goto err;
>> +	list_add_tail(&p->next, &r->props);
>> +
>> +	return rc;
>> +err:
>> +	kfree(p);
>> +	return rc;
>>   }
>>   
>>   /**
>> -- 
>> 2.43.0
> 
> --
> paul-moore.com

Powered by blists - more mailing lists