Re: [PATCH RFC v11 5/19] ipe: introduce 'boot_verified' as a trust provider

[Fan Wu <wufan@linux.microsoft.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