Messages in this thread | | | Date | Thu, 2 Nov 2023 15:46:52 -0700 | Subject | Re: [PATCH RFC v11 5/19] ipe: introduce 'boot_verified' as a trust provider | From | Fan Wu <> |
| |
On 10/26/2023 3:12 PM, Paul Moore wrote: > 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
I wasn't able to find a way to let LSM pin initramfs/initrd during mount time. But I think we could replace the global variable with a flag variable ipe_sb_state so we could use atomic operation to only mark one drive as pinned without any lock. The code will be like:
static void pin_sb(const struct super_block *sb) { if (!sb) return;
if (!test_and_set_bit_lock(IPE_SB_PINNED, &ipe_sb_state)) { ipe_sb(sb)->pinned = true; } }
Would this sound better?
-Fan
| |