On Thu, Jul 4, 2024 at 2:24 AM KP Singh [off-list ref] wrote:

On Thu, Jul 4, 2024 at 2:04 AM Casey Schaufler [off-list ref] wrote:

quoted

On 7/3/2024 4:08 PM, KP Singh wrote:

quoted

On Thu, Jul 4, 2024 at 12:52 AM Paul Moore [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 6:22 PM KP Singh [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 10:56 PM Paul Moore [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 12:55 PM KP Singh [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 2:07 AM Paul Moore [off-list ref] wrote:

quoted

On Jun 29, 2024 KP Singh [off-list ref] wrote:

quoted

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:

[...]

quoted

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".

quoted

quoted

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/ (local)

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.

Now if you folks really don't want any changes to the core LSM layer,
that too is doable, the patch below accomplishes that.

So, here's where we are at, while the LSM framework is comfortable
with saying that default values and empty callbacks are fine, that's
not what BPF LSM is comfortable doing. Your concerns around changes to
the LSM layer should be addressed by the propose patch below:

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;
 }

- KP

- KP

quoted

quoted

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/ (local)

- KP

quoted

--
paul-moore.com