Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 13 May 2022 09:53:04 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Mike Snitzer <snitzer@redhat.com>
Cc:     Alasdair Kergon <agk@redhat.com>,
        Mike Snitzer <snitzer@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        James Morris <jmorris@namei.org>,
        "Serge E . Hallyn" <serge@hallyn.com>, dm-devel@redhat.com,
        linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org,
        Song Liu <song@kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        linux-security-module@vger.kernel.org
Subject: Re: [PATCH v3 1/3] dm: Add verity helpers for LoadPin
Message-ID: <Yn6M8J1lfBpH8rW0@google.com>
References: <20220504195419.1143099-1-mka@chromium.org>
 <20220504125404.v3.1.I3e928575a23481121e73286874c4c2bdb403355d@changeid>
 <YnwioaRiVmYevo8i@google.com>
 <Yn1BkO4t+CXR0nzk@redhat.com>
 <Yn1xyVmPJGQz31lN@google.com>
 <Yn6HaVbfBxmmWkvI@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Yn6HaVbfBxmmWkvI@redhat.com>
Precedence: bulk

On Fri, May 13, 2022 at 12:29:29PM -0400, Mike Snitzer wrote:
> On Thu, May 12 2022 at  4:44P -0400,
> Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > On Thu, May 12, 2022 at 01:19:12PM -0400, Mike Snitzer wrote:
> > > On Wed, May 11 2022 at  4:54P -0400,
> > > Matthias Kaehlcke <mka@chromium.org> wrote:
> > > 
> > > > Alasdar/Mike, I'd be interested in your take on adding these functions
> > > > to verity/DM, to get an idea whether this series has a path forward to
> > > > landing upstream.
> > > 
> > > I'll be reviewing your patchset now. Comments inlined below.
> > > 
> > > > On Wed, May 04, 2022 at 12:54:17PM -0700, Matthias Kaehlcke wrote:
> > > > > LoadPin limits loading of kernel modules, firmware and certain
> > > > > other files to a 'pinned' file system (typically a read-only
> > > > > rootfs). To provide more flexibility LoadPin is being extended
> > > > > to also allow loading these files from trusted dm-verity
> > > > > devices. For that purpose LoadPin can be provided with a list
> > > > > of verity root digests that it should consider as trusted.
> > > > > 
> > > > > Add a bunch of helpers to allow LoadPin to check whether a DM
> > > > > device is a trusted verity device. The new functions broadly
> > > > > fall in two categories: those that need access to verity
> > > > > internals (like the root digest), and the 'glue' between
> > > > > LoadPin and verity. The new file dm-verity-loadpin.c contains
> > > > > the glue functions.
> > > > > 
> > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > ---
> > > > > 
> > > > > Changes in v3:
> > > > > - none
> > > > > 
> > > > > Changes in v2:
> > > > > - none
> > > > > 
> > > > >  drivers/md/Makefile               |  6 +++
> > > > >  drivers/md/dm-verity-loadpin.c    | 80 +++++++++++++++++++++++++++++++
> > > > >  drivers/md/dm-verity-target.c     | 33 +++++++++++++
> > > > >  drivers/md/dm-verity.h            |  4 ++
> > > > >  include/linux/dm-verity-loadpin.h | 27 +++++++++++
> > > > >  5 files changed, 150 insertions(+)
> > > > >  create mode 100644 drivers/md/dm-verity-loadpin.c
> > > > >  create mode 100644 include/linux/dm-verity-loadpin.h
> > > > > 
> > > > > diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> > > > > index 0454b0885b01..e12cd004d375 100644
> > > > > --- a/drivers/md/Makefile
> > > > > +++ b/drivers/md/Makefile
> > > > > @@ -100,6 +100,12 @@ ifeq ($(CONFIG_IMA),y)
> > > > >  dm-mod-objs			+= dm-ima.o
> > > > >  endif
> > > > >  
> > > > > +ifeq ($(CONFIG_DM_VERITY),y)
> > > > > +ifeq ($(CONFIG_SECURITY_LOADPIN),y)
> > > > > +dm-mod-objs			+= dm-verity-loadpin.o
> > > > > +endif
> > > > > +endif
> > > > > +
> > > 
> > > Why are you extending dm-mod-objs?  Why not dm-verity-objs?
> > 
> > Sorry, I missed to address this comment in my earlier reply.
> > 
> > I don't recall why I chose dm-mod-objs initially, agreed that
> > dm-verity-objs seems a better fit.
> 
> Yes, should be fixed even though the 3rd patch removes this change.

Sure

> BTW, looking at the 2nd patch's loadpin_is_fs_trusted().  Seems to me
> you'd do well to pass a 'struct block_device *' to a DM helper rather
> than force security/loadpin/loadpin.c to mess around with DM device
> refcounting, etc.

Sounds good to me. Thanks for the suggestion!