| Message ID | 1696457386-3010-6-git-send-email-wufan@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Integrity Policy Enforcement LSM (IPE) | expand |
On 10/23/2023 8:52 PM, Paul Moore wrote: > On Oct 4, 2023 Fan Wu <wufan@linux.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, introduce a property >> that causes the first superblock to have an execution to be "pinned", >> which is typically initramfs. >> >> When the "pinned" device is unmounted, it will be "unpinned" and >> `boot_verified` property will always evaluate to false afterward. >> >> We use a pointer with a spin_lock to "pin" the device instead of rcu >> because rcu synchronization may sleep, which is not allowed when >> unmounting a device. >> >> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com> >> Signed-off-by: Fan Wu <wufan@linux.microsoft.com> ... >> --- >> security/ipe/eval.c | 72 +++++++++++++++++++++++++++++++++++- >> security/ipe/eval.h | 2 + >> security/ipe/hooks.c | 12 ++++++ >> security/ipe/hooks.h | 2 + >> security/ipe/ipe.c | 1 + >> security/ipe/policy.h | 2 + >> security/ipe/policy_parser.c | 35 +++++++++++++++++- >> 7 files changed, 124 insertions(+), 2 deletions(-) >> >> diff --git a/security/ipe/eval.c b/security/ipe/eval.c >> index 8a8bcc5c7d7f..bdac4abc0ddb 100644 >> --- a/security/ipe/eval.c >> +++ b/security/ipe/eval.c >> @@ -9,6 +9,7 @@ >> #include <linux/file.h> >> #include <linux/sched.h> >> #include <linux/rcupdate.h> >> +#include <linux/spinlock.h> >> >> #include "ipe.h" >> #include "eval.h" >> @@ -16,6 +17,44 @@ >> >> struct ipe_policy __rcu *ipe_active_policy; >> >> +static const struct super_block *pinned_sb; >> +static DEFINE_SPINLOCK(pin_lock); >> +#define FILE_SUPERBLOCK(f) ((f)->f_path.mnt->mnt_sb) >> + >> +/** >> + * pin_sb - Pin the underlying superblock of @f, marking it as trusted. >> + * @sb: Supplies a super_block structure to be pinned. >> + */ >> +static void pin_sb(const struct super_block *sb) >> +{ >> + if (!sb) >> + return; >> + spin_lock(&pin_lock); >> + if (!pinned_sb) >> + pinned_sb = sb; >> + spin_unlock(&pin_lock); >> +} >> + >> +/** >> + * from_pinned - Determine whether @sb is the pinned super_block. >> + * @sb: Supplies a super_block to check against the pinned super_block. >> + * >> + * Return: >> + * * true - @sb is the pinned super_block >> + * * false - @sb is not the pinned super_block >> + */ >> +static bool from_pinned(const struct super_block *sb) >> +{ >> + bool rv; >> + >> + if (!sb) >> + return false; >> + spin_lock(&pin_lock); >> + rv = !IS_ERR_OR_NULL(pinned_sb) && pinned_sb == sb; >> + spin_unlock(&pin_lock); > > It's okay for an initial version, but I still think you need to get > away from this spinlock in from_pinned() as quickly as possible. > Maybe I'm wrong, but this looks like a major source of lock contention. > > I understand the issue around RCU and the potential for matching on > a reused buffer/address, but if you modified IPE to have its own LSM > security blob in super_block::security you could mark the superblock > when it was mounted and do a lockless lookup here in from_pinned(). > Thank you for the suggestion. After some testing, I discovered that switching to RCU to pin the super block and using a security blob to mark a pinned super block works. This approach do avoid many spinlock operations. I'll incorporate these changes in the next version of the patch. -Fan >> + return rv; >> +} > > -- > paul-moore.com
On Thu, Oct 26, 2023 at 5:33 PM Fan Wu <wufan@linux.microsoft.com> wrote: > On 10/23/2023 8:52 PM, Paul Moore wrote: > > On Oct 4, 2023 Fan Wu <wufan@linux.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, introduce a property > >> that causes the first superblock to have an execution to be "pinned", > >> which is typically initramfs. > >> > >> When the "pinned" device is unmounted, it will be "unpinned" and > >> `boot_verified` property will always evaluate to false afterward. > >> > >> We use a pointer with a spin_lock to "pin" the device instead of rcu > >> because rcu synchronization may sleep, which is not allowed when > >> unmounting a device. > >> > >> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com> > >> Signed-off-by: Fan Wu <wufan@linux.microsoft.com> > ... > >> --- > >> security/ipe/eval.c | 72 +++++++++++++++++++++++++++++++++++- > >> security/ipe/eval.h | 2 + > >> security/ipe/hooks.c | 12 ++++++ > >> security/ipe/hooks.h | 2 + > >> security/ipe/ipe.c | 1 + > >> security/ipe/policy.h | 2 + > >> security/ipe/policy_parser.c | 35 +++++++++++++++++- > >> 7 files changed, 124 insertions(+), 2 deletions(-) > >> > >> diff --git a/security/ipe/eval.c b/security/ipe/eval.c > >> index 8a8bcc5c7d7f..bdac4abc0ddb 100644 > >> --- a/security/ipe/eval.c > >> +++ b/security/ipe/eval.c > >> @@ -9,6 +9,7 @@ > >> #include <linux/file.h> > >> #include <linux/sched.h> > >> #include <linux/rcupdate.h> > >> +#include <linux/spinlock.h> > >> > >> #include "ipe.h" > >> #include "eval.h" > >> @@ -16,6 +17,44 @@ > >> > >> struct ipe_policy __rcu *ipe_active_policy; > >> > >> +static const struct super_block *pinned_sb; > >> +static DEFINE_SPINLOCK(pin_lock); > >> +#define FILE_SUPERBLOCK(f) ((f)->f_path.mnt->mnt_sb) > >> + > >> +/** > >> + * pin_sb - Pin the underlying superblock of @f, marking it as trusted. > >> + * @sb: Supplies a super_block structure to be pinned. > >> + */ > >> +static void pin_sb(const struct super_block *sb) > >> +{ > >> + if (!sb) > >> + return; > >> + spin_lock(&pin_lock); > >> + if (!pinned_sb) > >> + pinned_sb = sb; > >> + spin_unlock(&pin_lock); > >> +} > >> + > >> +/** > >> + * from_pinned - Determine whether @sb is the pinned super_block. > >> + * @sb: Supplies a super_block to check against the pinned super_block. > >> + * > >> + * Return: > >> + * * true - @sb is the pinned super_block > >> + * * false - @sb is not the pinned super_block > >> + */ > >> +static bool from_pinned(const struct super_block *sb) > >> +{ > >> + bool rv; > >> + > >> + if (!sb) > >> + return false; > >> + spin_lock(&pin_lock); > >> + rv = !IS_ERR_OR_NULL(pinned_sb) && pinned_sb == sb; > >> + spin_unlock(&pin_lock); > > > > It's okay for an initial version, but I still think you need to get > > away from this spinlock in from_pinned() as quickly as possible. > > Maybe I'm wrong, but this looks like a major source of lock contention. > > > > I understand the issue around RCU and the potential for matching on > > a reused buffer/address, but if you modified IPE to have its own LSM > > security blob in super_block::security you could mark the superblock > > when it was mounted and do a lockless lookup here in from_pinned(). > > Thank you for the suggestion. After some testing, I discovered that > switching to RCU to pin the super block and using a security blob to > mark a pinned super block works. This approach do avoid many spinlock > operations. I'll incorporate these changes in the next version of the patch. I probably wasn't as clear as I should have been, I was thinking of doing away with the @pinned_sb global variable entirely, as well as its associated lock problems and simply marking the initramfs/initrd superblock when it was mounted. I will admit that I haven't fully thought about all the implementation details, but I think you could leverage the security_sb_mount() hook to set a flag in IPE's superblock metadata when the initramfs was mounted. -- paul-moore.com
diff --git a/security/ipe/eval.c b/security/ipe/eval.c index 8a8bcc5c7d7f..bdac4abc0ddb 100644 --- a/security/ipe/eval.c +++ b/security/ipe/eval.c @@ -9,6 +9,7 @@ #include <linux/file.h> #include <linux/sched.h> #include <linux/rcupdate.h> +#include <linux/spinlock.h> #include "ipe.h" #include "eval.h" @@ -16,6 +17,44 @@ struct ipe_policy __rcu *ipe_active_policy; +static const struct super_block *pinned_sb; +static DEFINE_SPINLOCK(pin_lock); +#define FILE_SUPERBLOCK(f) ((f)->f_path.mnt->mnt_sb) + +/** + * pin_sb - Pin the underlying superblock of @f, marking it as trusted. + * @sb: Supplies a super_block structure to be pinned. + */ +static void pin_sb(const struct super_block *sb) +{ + if (!sb) + return; + spin_lock(&pin_lock); + if (!pinned_sb) + pinned_sb = sb; + spin_unlock(&pin_lock); +} + +/** + * from_pinned - Determine whether @sb is the pinned super_block. + * @sb: Supplies a super_block to check against the pinned super_block. + * + * Return: + * * true - @sb is the pinned super_block + * * false - @sb is not the pinned super_block + */ +static bool from_pinned(const struct super_block *sb) +{ + bool rv; + + if (!sb) + return false; + spin_lock(&pin_lock); + rv = !IS_ERR_OR_NULL(pinned_sb) && pinned_sb == sb; + spin_unlock(&pin_lock); + return rv; +} + /** * build_eval_ctx - Build an evaluation context. * @ctx: Supplies a pointer to the context to be populdated. @@ -26,8 +65,14 @@ void build_eval_ctx(struct ipe_eval_ctx *ctx, const struct file *file, enum ipe_op_type op) { + if (op == IPE_OP_EXEC && file) + pin_sb(FILE_SUPERBLOCK(file)); + ctx->file = file; ctx->op = op; + + if (file) + ctx->from_init_sb = from_pinned(FILE_SUPERBLOCK(file)); } /** @@ -42,7 +87,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 !ctx->from_init_sb; + case IPE_PROP_BOOT_VERIFIED_TRUE: + return ctx->from_init_sb; + default: + return false; + } } /** @@ -108,3 +160,21 @@ int ipe_evaluate_event(const struct ipe_eval_ctx *const ctx) return 0; } + +/** + * ipe_invalidate_pinned_sb - invalidate the ipe pinned super_block. + * @mnt_sb: super_block to check against the pinned super_block. + * + * This function is called a super_block like the initramfs's is freed, + * if the super_block is currently pinned by ipe it will be invalided, + * so ipe won't consider the block device is boot verified afterward. + */ +void ipe_invalidate_pinned_sb(const struct super_block *mnt_sb) +{ + spin_lock(&pin_lock); + + if (mnt_sb == pinned_sb) + pinned_sb = ERR_PTR(-EIO); + + spin_unlock(&pin_lock); +} diff --git a/security/ipe/eval.h b/security/ipe/eval.h index cfdf3c8dfe8a..9769da42c65f 100644 --- a/security/ipe/eval.h +++ b/security/ipe/eval.h @@ -19,9 +19,11 @@ struct ipe_eval_ctx { enum ipe_op_type op; const struct file *file; + bool from_init_sb; }; void build_eval_ctx(struct ipe_eval_ctx *ctx, const struct file *file, enum ipe_op_type op); int ipe_evaluate_event(const struct ipe_eval_ctx *const ctx); +void ipe_invalidate_pinned_sb(const struct super_block *mnt_sb); #endif /* _IPE_EVAL_H */ diff --git a/security/ipe/hooks.c b/security/ipe/hooks.c index 6164a9b53361..e9386762a597 100644 --- a/security/ipe/hooks.c +++ b/security/ipe/hooks.c @@ -181,3 +181,15 @@ int ipe_kernel_load_data(enum kernel_load_data_id id, bool contents) build_eval_ctx(&ctx, NULL, op); return ipe_evaluate_event(&ctx); } + +/** + * ipe_sb_free_security - ipe security hook function for super_block. + * @mnt_sb: Supplies a pointer to a super_block is about to be freed. + * + * IPE does not have any structures with mnt_sb, but uses this hook to + * invalidate a pinned super_block. + */ +void ipe_sb_free_security(struct super_block *mnt_sb) +{ + ipe_invalidate_pinned_sb(mnt_sb); +} diff --git a/security/ipe/hooks.h b/security/ipe/hooks.h index 23205452f758..ac0cdfd9877f 100644 --- a/security/ipe/hooks.h +++ b/security/ipe/hooks.h @@ -22,4 +22,6 @@ 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); +void ipe_sb_free_security(struct super_block *mnt_sb); + #endif /* _IPE_HOOKS_H */ diff --git a/security/ipe/ipe.c b/security/ipe/ipe.c index 77c950459810..06da94a58aba 100644 --- a/security/ipe/ipe.c +++ b/security/ipe/ipe.c @@ -15,6 +15,7 @@ static struct security_hook_list ipe_hooks[] __ro_after_init = { 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), + LSM_HOOK_INIT(sb_free_security, ipe_sb_free_security), }; /** diff --git a/security/ipe/policy.h b/security/ipe/policy.h index 77aa91f2b953..45704465dc01 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 c09458bd348d..ee7646de72ed 100644 --- a/security/ipe/policy_parser.c +++ b/security/ipe/policy_parser.c @@ -265,6 +265,12 @@ static enum ipe_action_type parse_action(char *t) return match_token(t, action_tokens, args); } +static const match_table_t property_tokens = { + {IPE_PROP_BOOT_VERIFIED_FALSE, "boot_verified=FALSE"}, + {IPE_PROP_BOOT_VERIFIED_TRUE, "boot_verified=TRUE"}, + {IPE_PROP_INVALID, NULL} +}; + /** * parse_property - Parse the property type given a token string. * @t: Supplies the token string to be parsed. @@ -277,7 +283,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; } /**