Message-ID: <20240531174710.GA1199@sol.localdomain>
Date: Fri, 31 May 2024 10:47:10 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Paul Moore <paul@...l-moore.com>
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 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.  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.

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 just hope you can understand that I'm also a bit burnt out now,
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.

Thanks,

- Eric

Powered by blists - more mailing lists