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

[KP Singh]

On Thu, Jul 4, 2024 at 2:04 AM Casey Schaufler <casey at schaufler-ca.com> wrote:
>
> On 7/3/2024 4:08 PM, KP Singh wrote:
> > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul at paul-moore.com> wrote:
> >> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh at kernel.org> wrote:
> >>> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul at paul-moore.com> wrote:
> >>>> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh at kernel.org> wrote:
> >>>>> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul at paul-moore.com> wrote:
> >>>>>> On Jun 29, 2024 KP Singh <kpsingh at 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:
> >>>>> [...]
> >>>>>
> >>>>>> should fix the more obvious problems.  I'd like to know if you are
> >>>>>> aware of any others?  If not, the text above should be adjusted and
> >>>>>> we should reconsider patch 5/5.  If there are other problems I'd
> >>>>>> like to better understand them as there may be an independent
> >>>>>> solution for those particular problems.
> >>>>> We did have problems with some other hooks but I was unable to dig up
> >>>>> specific examples though, it's been a while. More broadly speaking, a
> >>>>> default decision is still a decision. Whereas the intent from the BPF
> >>>>> LSM is not to make a default decision unless a BPF program is loaded.
> >>>>> I am quite worried about the holes this leaves open, subtle bugs
> >>>>> (security or crashes) we have not caught yet and PATCH 5/5 engineers away
> >>>>>  the problem of the "default decision".
> >>>> The inode/xattr problem you originally mentioned wasn't really rooted
> >>>> in a "bad" default return value, it was really an issue with how the
> >>>> LSM hook was structured due to some legacy design assumptions made
> >>>> well before the initial stacking patches were merged.  That should be
> >>>> fixed now[1] and given that the inode/xattr set/remove hooks were
> >>>> unique in this regard (individual LSMs were responsible for performing
> >>>> the capabilities checks) I don't expect this to be a general problem.
> >>>>
> >>>> There were also some issues caused by the fact that we were defining
> >>>> the default return value in multiple places and these values had gone
> >>>> out of sync in a number of hooks.  We've also fixed this problem by
> >>>> only defining the default return value once for each hook, solving all
> >>>> of those problems.
> >>> I don't see how this solves problems or prevents any future problems
> >>> with side-effects. I have always been uncomfortable with an extraneous
> >>> function being called with a side effect ever since we merged BPF LSM
> >>> with default callback. We have found one bug due to this, not all the
> >>> bugs.
> >> You've got to give me something more concrete than that.  If you can't
> >> provide any concrete examples, start with providing a basic concept
> >> with far more detail than just "side-effects".
> >>
> >>>> 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.
> >
> > You say you have not reviewed it carefully, 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/
> >
> > 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. I request others (Casey, Kees) for their opinion here too.
> > 2. Performance, no extra function call.
>
> I want to be very careful about the comments I make on this patch set.
> I can't say that I trust any fix for the BPF LSM layer. My natural
> inclination is to isolate the fix to the area that has the problem,
> that is, BPF. I have a hard time accepting the notion that the implementation
> will really fix all possible bugs in the future. The pace at which eBPF
> is evolving gives me the heebee geebees when I think of it as a mechanism
> for implementing security modules.
>
> My biggest concern is that we may be trying too hard for perfection.
> I see a situation where we're not moving forward because there are two
> reasonable solutions and rather than running with either we're desperately
> looking for a compelling reason to pick one over the other.

I am fine with either, if you folks prefer security_toggle_hook to be
in BPF only / limited to BPF. I can revert back to what we had in v9,
the changes to the LSM layer are then very minimal.

- KP

>
> >
> > If you still don't like it, it's your call, I would still like to keep
> > most of the logic local to the BPF LSM as proposed in
> > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
> >
> > - KP
> >
> >> --
> >> paul-moore.com