Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls

[Kees Cook <kees@kernel.org>]

On Fri, Jul 05, 2024 at 09:34:20PM +0200, KP Singh wrote:
> On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
> > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > LSM hooks are currently invoked from a linked list as indirect calls
> > > > > > > > > which are invoked using retpolines as a mitigation for speculative
> > > > > > > > > attacks (Branch History / Target injection) and add extra overhead which
> > > > > > > > > is especially bad in kernel hot paths:
> >
> > ...
> >
> > > > > > I'm not aware of any other existing problems relating to the LSM hook
> > > > > > default values, if there are any, we need to fix them independent of
> > > > > > this patchset.  The LSM framework should function properly if the
> > > > > > "default" values are used.
> > > > >
> > > > > Patch 5 eliminates the possibilities of errors and subtle bugs all
> > > > > together. The problem with subtle bugs is, well, they are subtle, if
> > > > > you and I knew of the bugs, we would fix all of them, but we don't. I
> > > > > really feel we ought to eliminate the class of issues and not just
> > > > > whack-a-mole when we see the bugs.
> > > >
> > > > Here's the thing, I don't really like patch 5/5.  To be honest, I
> > > > don't really like a lot of this patchset.  From my perspective, the
> > > > complexity of the code is likely going to mean more maintenance
> > > > headaches down the road, but Linus hath spoken so we're doing this
> > > > (although "this" is still a bit undefined as far as I'm concerned).
> > > > If you want me to merge patch 5/5 you've got to give me something real
> > > > and convincing that can't be fixed by any other means.  My current
> > > > opinion is that you're trying to use a previously fixed bug to scare
> > > > and/or coerce the merging of some changes I don't really want to
> > > > merge.  If you want me to take patch 5/5, you've got to give me a
> > > > reason that is far more compelling that what you've written thus far.
> > >
> > > Paul, I am not scaring you, I am providing a solution that saves us
> > > from headaches with side-effects and bugs in the future. It's safer by
> > > design.
> >
> > Perhaps I wasn't clear enough in my previous emails; instead of trying
> > to convince me that your solution is literally the best possible thing
> > to ever touch the kernel, convince me that there is a problem we need
> > to fix.  Right now, I'm not convinced there is a bug that requires all
> > of the extra code in patch 5/5 (all of which have the potential to
> > introduce new bugs).  As mentioned previously, the bugs that typically
> > have been used as examples of unwanted side effects with the LSM hooks
> > have been resolved, both in the specific and general case.  If you
> > want me to add more code/functionality to fix a bug, you must first
> > demonstrate the bug exists and the risk is real; you have not done
> > that as far as I'm concerned.
> >
> > > You say you have not reviewed it carefully ...
> >
> > That may have been true of previous versions of this patchset, but I
> > did not say that about this current patchset.
> >
> > > ... but you did ask me to move
> > > the function from the BPF LSM layer to an LSM API, and we had a bunch
> > > of discussion around naming in the subsequent revisions.
> > >
> > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
> >
> > That discussion predates commit 61df7b828204 ("lsm: fixup the inode
> > xattr capability handling") which is currently in the lsm/dev branch,
> > marked for stable, and will go up to Linus during the upcoming merge
> > window.
> >
> > > My reasons are:
> > >
> > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither
> > > you, nor me, can guarantee that a default value will be safe in the
> > > LSM layer.
> >
> > In the first sentence above you "guarantee" that your code is not
> > buggy and then follow that up with a second sentence discussing how no
> > one can guarantee source code safety.  Regardless of whatever point
> > you were trying to make here, I maintain that *all* patches have the
> > potential for bugs, even those that are attempting to fix bugs.  WithD
> > that in mind, if you want me to merge more code to fix a bug (class),
> > a bug that I've mentioned several times now that I believe we've
> > already fixed, you first MUST convince me that the bug (class) still
> > exists.  You have not done that.
> >
> 
> Paul, I am talking about eliminating a class of bugs, but you don't
> seem to get the point and you are fixated on the very instance of this
> bug class.

Let's take this one step at a time. I think patches 1-4 are fine and
stand alone and solve a specific problem without creating any new
immediate problems.

After 1-4 is accepted, we can come back around to what patch 5 is trying
to do, and work on whatever issues may remain at that time.

-- 
Kees Cook