Message ID | 1696457386-3010-6-git-send-email-wufan@linux.microsoft.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Paul Moore |
Headers | show |
Series | Integrity Policy Enforcement LSM (IPE) | expand |
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> > --- > 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 > --- > 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(). > + return rv; > +} -- paul-moore.com
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
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
On Thu, Nov 2, 2023 at 6:46 PM Fan Wu <wufan@linux.microsoft.com> wrote: > 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(-) ... > >>>> +/** > >>>> + * 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. > > I wasn't able to find a way to let LSM pin initramfs/initrd during mount > time ... I haven't had to look at the kernel init code in a while, and I don't recall ever looking at the initramfs code, but I spent some time digging through the code and I wonder if it would be possible to mark the initramfs superblock in wait_for_initramfs() via a new LSM hook using @current->fs->root.mnt->mnt_sb? Although I'm not completely sure that it's populated. Have you already looked at an approach like this? > 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
On Fri, Nov 3, 2023 at 6:15 PM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Nov 2, 2023 at 6:46 PM Fan Wu <wufan@linux.microsoft.com> wrote: > > 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(-) > > ... > > > >>>> +/** > > >>>> + * 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. > > > > I wasn't able to find a way to let LSM pin initramfs/initrd during mount > > time ... > > I haven't had to look at the kernel init code in a while, and I don't > recall ever looking at the initramfs code, but I spent some time > digging through the code and I wonder if it would be possible to mark > the initramfs superblock in wait_for_initramfs() via a new LSM hook > using @current->fs->root.mnt->mnt_sb? Although I'm not completely > sure that it's populated. Have you already looked at an approach like > this? Thinking about this more, the current IPE approach of treating the first file access as being present in the initramfs is not correct (one could build a system without an initramfs). I think we need to do something like the above where the initramfs is explicitly marked in the initramfs code. > > 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?
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; } /**