| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c40cd6a6-5c32-4e72-8831-f87ee0a09324@linux.microsoft.com>
Date: Thu, 2 Nov 2023 15:46:52 -0700
From: Fan Wu <wufan@...ux.microsoft.com>
To: Paul Moore <paul@...l-moore.com>
Cc: corbet@....net, zohar@...ux.ibm.com, jmorris@...ei.org,
serge@...lyn.com, tytso@....edu, ebiggers@...nel.org,
axboe@...nel.dk, agk@...hat.com, snitzer@...nel.org,
eparis@...hat.com, linux-doc@...r.kernel.org,
linux-integrity@...r.kernel.org,
linux-security-module@...r.kernel.org,
linux-fscrypt@...r.kernel.org, linux-block@...r.kernel.org,
dm-devel@...hat.com, audit@...r.kernel.org,
roberto.sassu@...wei.com, linux-kernel@...r.kernel.org,
Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH RFC v11 5/19] ipe: introduce 'boot_verified' as a trust
provider
On 10/26/2023 3:12 PM, Paul Moore wrote:
> On Thu, Oct 26, 2023 at 5:33 PM Fan Wu <wufan@...ux.microsoft.com> wrote:
>> On 10/23/2023 8:52 PM, Paul Moore wrote:
>>> On Oct 4, 2023 Fan Wu <wufan@...ux.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@...ux.microsoft.com>
>>>> Signed-off-by: Fan Wu <wufan@...ux.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
Powered by blists - more mailing lists