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

[Fan Wu]

On 10/26/2023 3:12 PM, Paul Moore wrote:
> On Thu, Oct 26, 2023 at 5:33 PM Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
> > On 10/23/2023 8:52 PM, Paul Moore wrote:
> > > On Oct  4, 2023 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>
> > ...
> > > > ---
> > > >    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