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

[KP Singh]

On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul at paul-moore.com> wrote:
>
> On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh at kernel.org> 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:
>
> ...
>
> > > > > 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.

> > 2. Performance, no extra function call.
>
> Convince me the bug still exists first and then we can discuss the
> merits of whatever solutions are proposed.

This is independent of the bug!

The extra function calls have performance overhead and as the BPF LSM
maintainer I am not okay with these extraneous calls when I have a
clear way of solving it.

As I said, If you don't want to modify the core LSM layer, it's okay,
I still want to go with changes local to the BPF LSM, If you really
don't agree with the changes local to the BPF LSM, we can have it go
via the BPF tree and seek Linus' help to resolve the conflict.

diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 1de7ece5d36d..5a2ab1067095 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -29,6 +29,7 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,

 bool bpf_lsm_is_sleepable_hook(u32 btf_id);
 bool bpf_lsm_is_trusted(const struct bpf_prog *prog);
+void bpf_lsm_toggle_hook(void *addr, bool value);

 static inline struct bpf_storage_blob *bpf_inode(
        const struct inode *inode)
@@ -52,6 +53,10 @@ static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id)
        return false;
 }

+static inline void bpf_lsm_toggle_hook(void *addr, bool value)
+{
+}
+
 static inline bool bpf_lsm_is_trusted(const struct bpf_prog *prog)
 {
        return false;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index f8302a5ca400..bc59025b3d46 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -523,6 +523,22 @@ static enum bpf_tramp_prog_type
bpf_attach_type_to_tramp(struct bpf_prog *prog)
        }
 }

+static void bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr,
+                                     enum bpf_tramp_prog_type kind)
+{
+       struct bpf_tramp_link *link;
+       bool found = false;
+
+       hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) {
+               if (link->link.prog->type == BPF_PROG_TYPE_LSM) {
+                       found  = true;
+                       break;
+               }
+       }
+
+       bpf_lsm_toggle_hook(tr->func.addr, found);
+}
+
 static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
struct bpf_trampoline *tr)
 {
        enum bpf_tramp_prog_type kind;
@@ -562,6 +578,10 @@ static int __bpf_trampoline_link_prog(struct
bpf_tramp_link *link, struct bpf_tr

        hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
        tr->progs_cnt[kind]++;
+
+       if (link->link.prog->type == BPF_PROG_TYPE_LSM)
+               bpf_trampoline_toggle_lsm(tr, kind);
+
        err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
        if (err) {
                hlist_del_init(&link->tramp_hlist);
@@ -595,6 +615,10 @@ static int __bpf_trampoline_unlink_prog(struct
bpf_tramp_link *link, struct bpf_
        }
        hlist_del_init(&link->tramp_hlist);
        tr->progs_cnt[kind]--;
+
+       if (link->link.prog->type == BPF_PROG_TYPE_LSM)
+               bpf_trampoline_toggle_lsm(tr, kind);
+
        return bpf_trampoline_update(tr, true /* lock_direct_mutex */);
 }

diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index 57b9ffd53c98..9ca3db6d2b07 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -16,6 +16,29 @@ static struct security_hook_list bpf_lsm_hooks[]
__ro_after_init = {
        LSM_HOOK_INIT(task_free, bpf_task_storage_free),
 };

+void bpf_lsm_toggle_hook(void *addr, bool enable)
+{
+       struct lsm_static_call *scalls;
+       struct security_hook_list *h;
+       int i, j;
+
+       for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) {
+               h = &bpf_lsm_hooks[i];
+               if (h->hook.lsm_func_addr != addr)
+                       continue;
+
+               for (j = 0; j < MAX_LSM_COUNT; j++) {
+                       scalls = &h->scalls[j];
+                       if (scalls->hl != &bpf_lsm_hooks[i])
+                               continue;
+                       if (enable)
+                               static_branch_enable(scalls->active);
+                       else
+                               static_branch_disable(scalls->active);
+               }
+       }
+}
+
 static const struct lsm_id bpf_lsmid = {
        .name = "bpf",
        .id = LSM_ID_BPF,
@@ -23,8 +46,14 @@ static const struct lsm_id bpf_lsmid = {

 static int __init bpf_lsm_init(void)
 {
+       int i;
+
        security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks),
                           &bpf_lsmid);
+
+       for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++)
+               bpf_lsm_toggle_hook(bpf_lsm_hooks[i].hook.lsm_func_addr, false);
+
        pr_info("LSM support for eBPF active\n");
        return 0;
 }



>
> --
> paul-moore.com