Message-ID: <CAHC9VhR9scT7V7dvN5zhAYdExORB9arWaR7Gbix1AUtAMDPHcg@mail.gmail.com>
Date:   Fri, 3 Nov 2023 18:15:42 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Fan Wu <wufan@...ux.microsoft.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 Thu, Nov 2, 2023 at 6:46 PM Fan Wu <wufan@...ux.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@...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(-)

...

> >>>> +/**
> >>>> + * 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

-- 
paul-moore.com

Powered by blists - more mailing lists