[RFC,v11,05/19] ipe: introduce 'boot_verified' as a trust provider
Commit Message
From: Deven Bowers <deven.desai@linux.microsoft.com>
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(-)
Comments
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?
@@ -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);
+}
@@ -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 */
@@ -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);
+}
@@ -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 */
@@ -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),
};
/**
@@ -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
};
@@ -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;
}
/**