Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1675119451-23180-1-git-send-email-wufan@linux.microsoft.com>
 <1675119451-23180-4-git-send-email-wufan@linux.microsoft.com>
 <CAHC9VhS_EbT7ze4oSHwHfus91VWQfdgGagf=5O7_h+XJ2o79PA@mail.gmail.com> <20230410185334.GA18827@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
In-Reply-To: <20230410185334.GA18827@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
From:   Paul Moore <paul@paul-moore.com>
Date:   Tue, 11 Apr 2023 16:32:41 -0400
Message-ID: <CAHC9VhTRpDK74iL6A6wt2=--5LmrC7pHZY_BLnHDdfqboA2i1A@mail.gmail.com>
Subject: Re: [RFC PATCH v9 03/16] ipe: add evaluation loop and introduce
 'boot_verified' as a trust provider
To:     Fan Wu <wufan@linux.microsoft.com>
Cc:     corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org,
        serge@hallyn.com, tytso@mit.edu, ebiggers@kernel.org,
        axboe@kernel.dk, agk@redhat.com, snitzer@kernel.org,
        eparis@redhat.com, linux-doc@vger.kernel.org,
        linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fscrypt@vger.kernel.org, linux-block@vger.kernel.org,
        dm-devel@redhat.com, linux-audit@redhat.com,
        roberto.sassu@huawei.com, linux-kernel@vger.kernel.org,
        Deven Bowers <deven.desai@linux.microsoft.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Apr 10, 2023 at 2:53=E2=80=AFPM Fan Wu <wufan@linux.microsoft.com> =
wrote:
> On Thu, Mar 02, 2023 at 02:03:11PM -0500, Paul Moore wrote:
> > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wr=
ote:
> > >
> > > From: Deven Bowers <deven.desai@linux.microsoft.com>
> > >
> > > IPE must have a centralized function to evaluate incoming callers
> > > against IPE's policy. This iteration of the policy against the rules
> > > for that specific caller is known as the evaluation loop.
> > >
> > > In addition, IPE is designed to provide system level trust guarantees=
,
> > > this usually implies that trust starts from bootup with a hardware ro=
ot
> > > 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 proper=
ty
> > > that causes the first superblock to have an execution to be "pinned",
> > > which is typically initramfs.
> > >
> > > Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> > > Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
> >
> > ...
> >
> > > ---
> > >  security/ipe/Makefile        |   1 +
> > >  security/ipe/eval.c          | 180 +++++++++++++++++++++++++++++++++=
++
> > >  security/ipe/eval.h          |  28 ++++++
> > >  security/ipe/hooks.c         |  25 +++++
> > >  security/ipe/hooks.h         |  14 +++
> > >  security/ipe/ipe.c           |   1 +
> > >  security/ipe/policy.c        |  20 ++++
> > >  security/ipe/policy.h        |   3 +
> > >  security/ipe/policy_parser.c |   8 +-
> > >  9 files changed, 279 insertions(+), 1 deletion(-)
> > >  create mode 100644 security/ipe/eval.c
> > >  create mode 100644 security/ipe/eval.h
> > >  create mode 100644 security/ipe/hooks.c
> > >  create mode 100644 security/ipe/hooks.h

...

> > > diff --git a/security/ipe/eval.c b/security/ipe/eval.c
> > > new file mode 100644
> > > index 000000000000..48b5104a3463
> > > --- /dev/null
> > > +++ b/security/ipe/eval.c
> > > @@ -0,0 +1,180 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > > + */
> > > +
> > > +#include "ipe.h"
> > > +#include "eval.h"
> > > +#include "hooks.h"
> > > +#include "policy.h"
> > > +
> > > +#include <linux/fs.h>
> > > +#include <linux/types.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/file.h>
> > > +#include <linux/sched.h>
> > > +#include <linux/rcupdate.h>
> > > +#include <linux/spinlock.h>
> > > +
> > > +struct ipe_policy __rcu *ipe_active_policy;
> > > +
> > > +static 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 trust=
ed.
> > > + * @f: Supplies a file structure to source the super_block from.
> > > + */
> > > +static void pin_sb(const struct file *f)
> > > +{
> > > +       if (!f)
> > > +               return;
> > > +       spin_lock(&pin_lock);
> > > +       if (pinned_sb)
> > > +               goto out;
> > > +       pinned_sb =3D FILE_SUPERBLOCK(f);
> > > +out:
> > > +       spin_unlock(&pin_lock);
> > > +}
> >
> > Since you don't actually use @f, just the super_block, you might
> > consider passing the super_block as the parameter and not the
> > associated file.
> >
> > I'd probably also flip the if-then to avoid the 'goto', for example:
> >
> >   static void pin_sb(const struct super_block *sb)
> >   {
> >     if (!sb)
> >       return;
> >     spin_lock(&pin_lock);
> >     if (!pinned_sb)
> >       pinned_sb =3D sb;
> >     spin_unlock(&pin_lock);
> >   }
> >
>
> Sure, I can change the code accordingly.
>
> > Also, do we need to worry about the initramfs' being unmounted and the
> > super_block going away?
>
> If initramfs is being unmounted, the boot_verified property will never be=
 TRUE,
> which is an expected behavior. In an actual use case, we can leverage thi=
s
> property to only enable files in initramfs during the booting stage, and =
later switch
> to another policy without the boot_verified property after unmounting the=
 initramfs.
> This approach helps keep the allowed set of files minimum at each stage.

I think I was worried about not catching when the fs was unmounted and
the superblock disappeared, but you've got a hook defined for that so
it should be okay.  I'm not sure what I was thinking here, sorry for
the noise ...

Regardless of the source of my confusion, your policy/boot_verified
description all sounds good to me.

--=20
paul-moore.com