On Fri, May 31, 2024 at 1:47 PM Eric Biggers <ebiggers@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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
