Message-ID: <CAHC9VhQDPnq3X0fFOXf5zY38K_STffmZJMqHp50sVqWW6pcabw@mail.gmail.com>
Date: Sun, 2 Jun 2024 21:40:34 -0400
From: Paul Moore <paul@...l-moore.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Fan Wu <wufan@...ux.microsoft.com>, corbet@....net, zohar@...ux.ibm.com, 
	jmorris@...ei.org, serge@...lyn.com, tytso@....edu, axboe@...nel.dk, 
	agk@...hat.com, snitzer@...nel.org, mpatocka@...hat.com, eparis@...hat.com, 
	linux-doc@...r.kernel.org, linux-integrity@...r.kernel.org, 
	linux-security-module@...r.kernel.org, fsverity@...ts.linux.dev, 
	linux-block@...r.kernel.org, dm-devel@...ts.linux.dev, audit@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH v19 15/20] fsverity: expose verified fsverity built-in
 signatures to LSMs

On Fri, May 31, 2024 at 1:47 PM Eric Biggers <ebiggers@...nel.org> wrote:
> On Fri, May 31, 2024 at 11:51:47AM -0400, Paul Moore wrote:
> > On Thu, May 30, 2024 at 8:43 PM Eric Biggers <ebiggers@...nel.org> wrote:
> > > On Thu, May 30, 2024 at 04:54:37PM -0400, Paul Moore wrote:
> > > > On Wed, May 29, 2024 at 11:06 PM Eric Biggers <ebiggers@...nel.org> wrote:
> > > > > On Wed, May 29, 2024 at 09:46:57PM -0400, Paul Moore wrote:
> > > > > > On Fri, May 24, 2024 at 4:46 PM Fan Wu <wufan@...ux.microsoft.com> wrote:
> > > > > > >
> > > > > > > This patch enhances fsverity's capabilities to support both integrity and
> > > > > > > authenticity protection by introducing the exposure of built-in
> > > > > > > signatures through a new LSM hook. This functionality allows LSMs,
> > > > > > > e.g. IPE, to enforce policies based on the authenticity and integrity of
> > > > > > > files, specifically focusing on built-in fsverity signatures. It enables
> > > > > > > a policy enforcement layer within LSMs for fsverity, offering granular
> > > > > > > control over the usage of authenticity claims. For instance, a policy
> > > > > > > could be established to permit the execution of all files with verified
> > > > > > > built-in fsverity signatures while restricting kernel module loading
> > > > > > > from specified fsverity files via fsverity digests.
> > > >
> > > > ...
> > > >
> > > > > > Eric, can you give this patch in particular a look to make sure you
> > > > > > are okay with everything?  I believe Fan has addressed all of your
> > > > > > previous comments and it would be nice to have your Ack/Review tag if
> > > > > > you are okay with the current revision.
> > > > >
> > > > > Sorry, I've just gotten a bit tired of finding so many basic issues in this
> > > > > patchset even after years of revisions.
> > > > >
> > > > > This patch in particular is finally looking better.  There are a couple issues
> > > > > that I still see.  (BTW, you're welcome to review it too to help find these
> > > > > things, given that you seem to have an interest in getting this landed...):
> > > >
> > > > I too have been reviewing this patchset across multiple years and have
> > > > worked with Fan to fix locking issues, parsing issues, the initramfs
> > > > approach, etc.
> > >
> > > Sure, but none of the patches actually have your Reviewed-by.
> >
> > As a general rule I don't post Acked-by/Reviewed-by tags for patches
> > that are targeting a subsystem that I maintain.  The logic being that
> > I'm going to be adding my Signed-off-by tag to the patches and arguing
> > these in front of Linus, so adding a Acked-by/Reviewed-by simply
> > creates more work later on where I have to strip them off and replace
> > them with my sign-off.
> >
> > If the lack of a Reviewed-by tag is *really* what is preventing you
> > from reviewing the fs-verity patch, I can post that starting with the
> > next revision, but I'm guessing the lack of my tag isn't your core
> > issue (or at least I would argue it shouldn't be).
> >
> > > > My interest in getting this landed is simply a
> > > > combination of fulfilling my role as LSM maintainer as well as being
> > > > Fan's coworker.  While I realize you don't work with Fan, you are
> > > > listed as the fs-verity maintainer and as such I've been looking to
> > > > you to help review and authorize the fs-verity related code.  If you
> > > > are too busy, frustrated, or <fill in the blank> to continue reviewing
> > > > this patchset it would be helpful if you could identify an authorized
> > > > fs-verity reviewer.  I don't see any besides you and Ted listed in the
> > > > MAINTAINERS file, but perhaps the fs-verity entry is dated.
> > > >
> > > > Regardless, I appreciate your time and feedback thus far and I'm sure
> > > > Fan does as well.
> > >
> > > Maintainers are expected to do reviews and acks, but not to the extent of
> > > extensive hand-holding of a half-baked submission.
> >
> > Considering the current state of this patchset I don't believe that
> > verdict to be fair, or very considerate.
> >
> > We clearly have different styles and approaches towards subsystem
> > maintainer roles.  I've had the good fortune to work with both hostile
> > and helpful senior developers during the early years of my time
> > working in the Linux kernel, and it helped reinforce the impact
> > patience and mentoring can have on contributors who are new to the
> > Linux kernel or perhaps system programming in general.  While I'm far
> > from perfect in this regard, I do hope and recommend that all of us in
> > maintainer, or senior developer, roles remember to exercise some
> > additional patience and education when working with new contributors.
> >
>
> It's not clear to me that you've done a close review of the verity related
> patches, including not just this one but the dm-verity related ones and the
> fsverity and dm-verity support in IPE itself, given the issues that I've been
> finding in them in the last couple months.

I have not been able to give the fs-verify or dm-verity patches the
same level of scrutiny as the associated subsystem maintainers simply
because I lack the deep history and background; I rely on the
associated maintainers to catch the important "gotchas" as we've seen
in the patchset.

> As I said before, I'm not too
> enthusiastic about IPE myself, for various reasons I've explained, so I've
> really been looking to the people who actually want it to help drive it forward.

Once again, that is what I have been doing in my effort to get this to
a point where it can be merged and sent to Linus.  I've spent numerous
hours reviewing patches on-list (and catching quite a few issues), and
working with Fan off-list, to ensure these patches continue to
improve.  I'm asking you to review the fs-verity patch(es) for two
main reasons: 1) to identify any fs-verity interaction problems, 2) as
a courtesy since you are the fs-verity maintainer and I want you to be
aware of this and accepting of the code being introduced in the
subsystem you are responsible for maintaining.

> Anyway, as I also said, the fsverity and dm-verity support does seem to be
> improved now after all the rounds of feedback, and I think it's close to the
> finish line.

I agree.  I appreciate your help in reviewing this patchset, and those
that came before it.  I've seen Fan voice his appreciation too
on-list.

>  I just hope you can understand that I'm also a bit burnt out now,

I can understand that, and I'm sympathetic.  I've been doing this long
enough to have gone through my own cycles of burnout and rejuvenation
and I know how disheartening it can be at times.

> and getting asked for an ack on this patch again and then seeing a bug in it
> (despite it having been simplified to only a few lines now) and also still
> misleading information in the commit message that I asked to be fixed before, is
> a bit frustrating.  I think it's reasonable to expect a bit better, especially
> for a security oriented feature.

I firmly believe that no one writes perfect code, and no one performs
a perfect review.  As far as I'm concerned, the important bit is that
you respond, learn, and strive to do better next time.

-- 
paul-moore.com

Powered by blists - more mailing lists